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 310 matching lines @@
 
   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;
 
-  function handlerBehaviorChanged()

[Wladimir Palant, 2015/04/09 18:45:02]

That's a confusing name. We have three things all called handlerBehaviorChanged:
Chrome's API function, our API function and this private function. While the
former two have similar meaning, the latter doesn't.

Maybe rename it into something like propagateChange()?

[Sebastian Noack, 2015/04/10 06:45:26]

Good point. I went for propagateHandlerBehaviorChange().

+  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.
     if (handlerBehaviorChangedQuota > 0)
     {
-      chrome.webNavigation.onBeforeNavigate.removeListener(handlerBehaviorChanged);
+      chrome.webNavigation.onBeforeNavigate.removeListener(propagateHandlerBehaviorChange);
       chrome.webRequest.handlerBehaviorChanged();
 
       handlerBehaviorChangedQuota--;
       setTimeout(function() { handlerBehaviorChangedQuota++; }, 600000);

[Wladimir Palant, 2015/04/09 18:45:02]

This will "recover" the quota way too slowly. I guess the timeout should be
600000/chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES?

[Sebastian Noack, 2015/04/10 06:45:26]

I think it is correct. This makes sure to not call handlerBehaviorChanged() more
ofthen than MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES within 10 minutes.
This would recover the quota way to quickly (every 30 seconds), potentially
calling handlerBehaviorChanged() way more frequently than
MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES within 10 minutes.

[Wladimir Palant, 2015/04/10 06:58:14]

You are right, the current approach is correct.

     }
   }
 
   ext.webRequest = {
     onBeforeRequest: new ext._EventTarget(),
     handlerBehaviorChanged: function()
     {
       // 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(handlerBehaviorChanged))
-        onBeforeNavigate.addListener(handlerBehaviorChanged);

[Wladimir Palant, 2015/04/09 18:45:02]

My initial reaction was: "cannot be that the same listener will be added twice."
Well, it can...

[Sebastian Noack, 2015/04/10 06:45:26]

Yep, it can. ;)

+      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"]
@@ skipping 189 matching lines @@
             callback(new Page(tab));
         }
         else
         {
           ext.pages.open(optionsUrl, callback);
         }
       });
     });
   };
 })();