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 308 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--;
+
+    // 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);
+  }

[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: chrome.webRequest.handlerBehaviorChanged
+    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;
+      }
+    }
   };
 
   chrome.tabs.query({}, function(tabs)
   {
     tabs.forEach(function(tab)
     {
       chrome.webNavigation.getAllFrames({tabId: tab.id}, function(details)
       {
         if (details && details.length > 0)
         {
@@ skipping 172 matching lines @@
             callback(new Page(tab));
         }
         else
         {
           ext.pages.open(optionsUrl, callback);
         }
       });
     });
   };
 })();