Issue 2034 - Respect chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES

chrome/ext/background.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 309 matching lines @@
   /* Web requests */
 
   var framesOfTabs = Object.create(null);
 
   ext.getFrame = function(tabId, frameId)
   {
     return (framesOfTabs[tabId] || {})[frameId];
   };
 
   var handlerBehaviorChangedQuota = chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES;
-  var handlerBehaviorChangedQuotaExceeded = false;
-  var handlerBehaviorChangedPending = false;
-
-  function handlerBehaviorChanged()
-  {
-    chrome.webRequest.handlerBehaviorChanged();
-    handlerBehaviorChangedQuota--;
-
+
+  function propagateHandlerBehaviorChange()
+  {
     // Make sure to not call handlerBehaviorChanged() more often than allowed
     // by chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES.
     // Otherwise Chrome notifies the user that this extension is causing issues.
-    setTimeout(function()
-    {
-      handlerBehaviorChangedQuota++;
-
-      // If the quota were exceeded, call handlerBehaviorChanged()
-      // delayed, when 10 minutes since the first call within the
-      // past 10 minutes passed.
-      if (handlerBehaviorChangedQuotaExceeded)
-      {
-        handlerBehaviorChangedQuotaExceeded = false;
-        handlerBehaviorChanged();
-      }
-    }, 600000);
+    if (handlerBehaviorChangedQuota > 0)
+    {
+      chrome.webNavigation.onBeforeNavigate.removeListener(propagateHandlerBehaviorChange);
+      chrome.webRequest.handlerBehaviorChanged();
+
+      handlerBehaviorChangedQuota--;
+      setTimeout(function() { handlerBehaviorChangedQuota++; }, 600000);
+    }
   }

[Wladimir Palant, 2015/02/23 18:25:26]

Is this really a good approach? I think that we should leave quota measuring up
to Chrome. The goal isn't calling this function exactly the number of times that
won't trigger the warning - the point is rather that it shouldn't be called too
often.

How about the following approach:

* handlerBehaviorChanged can be called every 600 /
chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES seconds
(quotaInterval).
* If handlerBehaviorChanged is called, check for an existent timeout. If there
is one - ignore that call.
* If there is no timeout, calculate currentInterval as the difference between
current time and time of previous handlerBehaviorChanged call.
* If currentInterval is larger than quotaInterval - pass the call to Chrome and
remember the current time as time of previous call.
* If currentInterval is smaller than quotaInterval: create a timeout to call
handlerBehaviorChanged after |quotaInterval - currentInterval| seconds (clear
timeout before doing that, to make sure something actually happens).

This will make sure that the quota is never exceeded, and should be much simpler
than the code here. More importantly, it will make sure we don't overuse CPU
resources even for smaller time intervals - and refreshing will never be delayed
by more than 30 seconds.

[Sebastian Noack, 2015/02/23 19:12:08]

We call handlerBehaviorChanged() only if it did. That is once a day for most
users. However, sometimes users perform subsequent filter changes. And if you
add/remove filters you care more about seeing the effect of those change, but
not how fast you can reload or navigate to a similar a page (this is all
handlerBehaviorChanged is about, it is merely flushing the in-memory-per-tab
cache). So this quota is stupid to have in the first place. It doesn't matter
whether the in-memory cache isn't flushed for 2 or for 9 minutes. It results
into buggy/unexpected behavior either way. So my strategy here is to maintain
user experience as long as possible by immediately flushing the in-memory cache,
until the silly quota is exceeded. So most users won't even note that there is a
quota.

[Wladimir Palant, 2015/02/23 19:22:42]

If users are changing filters frequently (more often than every 30 seconds),
then it doesn't sound like they want to see immediate results - they are rather
working off a batch of changes, and we shouldn't call handlerBehaviorChanged()
after each individual change.

[Sebastian Noack, 2015/04/09 07:04:55]

How about this: The new patch defers handlerBehaviorChanged() until
onBeforeNavigate, that means until the user reloads a page or loads a new one.
This way we won't call handlerBehaviorChanged() while the user might work off a
batch of changes regardless how long it takes. Plus the implementation is
simpler than what you and I initially suggested.

 
   ext.webRequest = {
     onBeforeRequest: new ext._EventTarget(),
     handlerBehaviorChanged: function()
     {
-      if (handlerBehaviorChangedQuota > 0)
-      {
-        // Call handlerBehaviorChanged() asynchronously, and only if it
-        // hasn't been scheduled yet. That way we avoid to call it multiple
-        // times, if multiple filters were added/removed simultanously.
-        if (!handlerBehaviorChangedPending)
-        {
-          handlerBehaviorChangedPending = true;
-
-          setTimeout(function()
-          {
-            handlerBehaviorChanged();
-            handlerBehaviorChangedPending = false;
-          }, 0);
-        }
-      }
-      else
-      {
-        handlerBehaviorChangedQuotaExceeded = true;
-      }
-    }
-  };
+      // Defer handlerBehaviorChanged() until navigation occurs.
+      // There wouldn't be any visible effect when calling it earlier,
+      // but it's an expensive operation and that way we avoid to call
+      // it multiple times, if multiple filters are added/removed.
+      var onBeforeNavigate = chrome.webNavigation.onBeforeNavigate;
+      if (!onBeforeNavigate.hasListener(propagateHandlerBehaviorChange))
+        onBeforeNavigate.addListener(propagateHandlerBehaviorChange);
+    }
+  };
+
+  // Since Chrome 38 requests of type 'object' (e.g. requests
+  // initiated by Flash) are mistakenly reported with the type 'other'.
+  // https://code.google.com/p/chromium/issues/detail?id=410382
+  if (parseInt(navigator.userAgent.match(/\bChrome\/(\d+)/)[1], 10) >= 38)
+  {
+    ext.webRequest.indistinguishableTypes = [
+      ["OTHER", "OBJECT", "OBJECT_SUBREQUEST"]
+    ];
+  }
+  else
+  {
+    ext.webRequest.indistinguishableTypes = [
+      ["OBJECT", "OBJECT_SUBREQUEST"],
+      ["OTHER", "MEDIA", "FONT"]
+    ];
+  }
 
   chrome.tabs.query({}, function(tabs)
   {
     tabs.forEach(function(tab)
     {
       chrome.webNavigation.getAllFrames({tabId: tab.id}, function(details)
       {
         if (details && details.length > 0)
         {
           var frames = framesOfTabs[tab.id] = Object.create(null);
@@ skipping 16 matching lines @@
   chrome.webRequest.onBeforeRequest.addListener(function(details)
   {
     try
     {
       // the high-level code isn't interested in requests that aren't related
       // to a tab and since those can only be handled in Chrome, we ignore
       // them here instead of in the browser independent high-level code.
       if (details.tabId == -1)
         return;
 
-      var requestType = details.type;
-      var isMainFrame = requestType == "main_frame" || (
+      var isMainFrame = details.type == "main_frame" || (
 
         // assume that the first request belongs to the top frame. Chrome
         // may give the top frame the type "object" instead of "main_frame".
         // https://code.google.com/p/chromium/issues/detail?id=281711
         details.frameId == 0 && !(details.tabId in framesOfTabs)
       );
 
       var frames = null;
       if (!isMainFrame)
         frames = framesOfTabs[details.tabId];
       if (!frames)
         frames = framesOfTabs[details.tabId] = Object.create(null);
 
       var frame = null;
       var url = new URL(details.url);
       if (!isMainFrame)
       {
         // we are looking for the frame that contains the element that
         // is about to load, however if a frame is loading the surrounding
         // frame is indicated by parentFrameId instead of frameId
         var frameId;
-        if (requestType == "sub_frame")
+        var requestType;
+        if (details.type == "sub_frame")
+        {
           frameId = details.parentFrameId;
+          requestType = "SUBDOCUMENT";
+        }
         else
+        {
           frameId = details.frameId;
+          requestType = details.type.toUpperCase();
+        }
 
         frame = frames[frameId] || frames[Object.keys(frames)[0]];
 
         if (frame)
         {
-          // Since Chrome 38 requests of type 'object' (e.g. requests
-          // initiated by Flash) are mistakenly reported with the type 'other'.
-          // https://code.google.com/p/chromium/issues/detail?id=410382
-          if (requestType == "other" && parseInt(navigator.userAgent.match(/\bChrome\/(\d+)/)[1], 10) >= 38)
-            requestType = "object";
-
           var results = ext.webRequest.onBeforeRequest._dispatch(
             url,
             requestType,
             new Page({id: details.tabId}),
             frame
           );
 
           if (results.indexOf(false) != -1)
             return {cancel: true};
         }
       }
 
       if (isMainFrame || details.type == "sub_frame")
         frames[details.frameId] = {url: url, parent: frame};
     }
     catch (e)
     {
       // recent versions of Chrome cancel the request when an error occurs in
       // the onBeforeRequest listener. However in our case it is preferred, to
       // let potentially some ads through, rather than blocking legit requests.
       console.error(e);
     }
   }, {urls: ["http://*/*", "https://*/*"]}, ["blocking"]);
 
 
   /* Message passing */
 
   chrome.runtime.onMessage.addListener(function(message, rawSender, sendResponse)
   {
-    var sender = {
-      page: new Page(rawSender.tab),
-      frame: {
+    var sender = {};
+
+    // Add "page" and "frame" if the message was sent by a content script.
+    // If sent by popup or the background page itself, there is no "tab".
+    if ("tab" in rawSender)
+    {
+      sender.page = new Page(rawSender.tab);
+      sender.frame = {
         url: new URL(rawSender.url),
         get parent()
         {
           var frames = framesOfTabs[rawSender.tab.id];
 
           if (!frames)
             return null;
 
           if ("frameId" in rawSender)
           {
             // Chrome 41+
             var frame = frames[rawSender.frameId];
             if (frame)
               return frame.parent;
           }
           else
           {
             // Chrome 28-40
             for (var frameId in frames)
             {
               if (frames[frameId].url.href == this.url.href)
                 return frames[frameId].parent;
             }
           }
 
           return frames[0];
         }
-      }
-    };
+      };
+    }
 
     return ext.onMessage._dispatch(message, sender, sendResponse).indexOf(true) != -1;
   });
 
   // We have to ensure there is at least one listener for the onConnect event.
   // Otherwise we can't connect a port later, which we need to do in order to
   // detect when the extension is reloaded, disabled or uninstalled.
   chrome.runtime.onConnect.addListener(function() {});
 
 
@@ skipping 17 matching lines @@
       if (!win.incognito)
         queryInfo.windowId = win.id;
 
       chrome.tabs.query(queryInfo, function(tabs)
       {
         if (tabs.length > 0)
         {
           var tab = tabs[0];
 
           chrome.windows.update(tab.windowId, {focused: true});
-          chrome.tabs.highlight(
-            {
-              windowId: tab.windowId,
-              tabs: [tab.index]
-            },
-            function() {}
-          );
+          chrome.tabs.update(tab.id, {active: true});
 
           if (callback)
             callback(new Page(tab));
         }
         else
         {
           ext.pages.open(optionsUrl, callback);
         }
       });
     });
   };
 })();