Issue 3860 - Move request blocking logic to a seperate module

lib/requestBlocker.js

Index: lib/requestBlocker.js
===================================================================
rename from webrequest.js
rename to lib/requestBlocker.js
--- a/webrequest.js
+++ b/lib/requestBlocker.js
@@ -15,38 +15,25 @@
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-var FilterNotifier = require("filterNotifier").FilterNotifier;
-var RegExpFilter = require("filterClasses").RegExpFilter;
-var platform = require("info").platform;
-var devtools = require("devtools");
+/** @module requestBlocker */
 
-ext.webRequest.getIndistinguishableTypes().forEach(function(types)
+"use strict";
+
+let {RegExpFilter, BlockingFilter} = require("filterClasses");
+let {defaultMatcher} = require("matcher");
+let {FilterNotifier} = require("filterNotifier");
+let {Prefs} = require("prefs");
+let {checkWhitelisted, getKey} = require("whitelisting");
+let {stringifyURL, extractHostFromFrame, isThirdParty} = require("url");
+let {port} = require("messaging");
+let devtools = require("devtools");
+
+ext.webRequest.getIndistinguishableTypes().forEach(types =>
 {
-  for (var i = 1; i < types.length; i++)
+  for (let i = 1; i < types.length; i++)
     RegExpFilter.typeMap[types[i]] = RegExpFilter.typeMap[types[0]];
 });
 
-FilterNotifier.addListener(function(action, arg)
-{
-  switch (action)
-  {
-    case "filter.added":
-    case "filter.removed":
-    case "filter.disabled":
-      // Only request blocking/whitelisting filters have
-      // an effect on the webRequest handler behavior.
-      if (!(arg instanceof RegExpFilter))
-        break;
-    case "subscription.added":
-    case "subscription.removed":
-    case "subscription.disabled":
-    case "subscription.updated":
-    case "load":
-      ext.webRequest.handlerBehaviorChanged();
-      break;
-  }
-});
-
 function onBeforeRequestAsync(page, url, type, docDomain,
                               thirdParty, key, specificOnly,
                               filter)
@@ -62,21 +49,21 @@
     );
 }
 
-function onBeforeRequest(url, type, page, frame)
+ext.webRequest.onBeforeRequest.addListener((url, type, page, frame) =>
 {
   if (checkWhitelisted(page, frame))
     return true;
 
-  var urlString = stringifyURL(url);
-  var docDomain = extractHostFromFrame(frame);
-  var thirdParty = isThirdParty(url, docDomain);
-  var key = getKey(page, frame);
+  let urlString = stringifyURL(url);
+  let docDomain = extractHostFromFrame(frame);
+  let thirdParty = isThirdParty(url, docDomain);
+  let key = getKey(page, frame);
 
-  var specificOnly = !!checkWhitelisted(
+  let specificOnly = !!checkWhitelisted(
     page, frame, RegExpFilter.typeMap.GENERICBLOCK
   );
 
-  var filter = defaultMatcher.matchesAny(
+  let filter = defaultMatcher.matchesAny(
     urlString, RegExpFilter.typeMap[type],
     docDomain, thirdParty, key, specificOnly
   );
@@ -87,34 +74,95 @@
                                       specificOnly, filter);
 
   return !(filter instanceof BlockingFilter);
-}
+});
 
-ext.webRequest.onBeforeRequest.addListener(onBeforeRequest);
+port.on("filters.collapse", (message, sender) =>
+{
+  if (checkWhitelisted(sender.page, sender.frame))
+    return false;
 
-if (platform == "chromium")
+  let typeMask = RegExpFilter.typeMap[message.mediatype];
+  let documentHost = extractHostFromFrame(sender.frame);
+  let sitekey = getKey(sender.page, sender.frame);

[kzar, 2016/03/23 15:36:00]

Nit: the name `sitekey` seems inconsistent with the name `key` used above in the
request listener. 

[Sebastian Noack, 2016/03/23 16:44:48]

Well, it was inconsistent before. But fine with me using sitekey (like in other
modules) everywhere. 

+  let blocked = false;
+
+  let specificOnly = checkWhitelisted(
+    sender.page, sender.frame,
+    RegExpFilter.typeMap.GENERICBLOCK
+  );
+
+  for (let url of message.urls)
+  {
+    let urlObj = new URL(url, message.baseURL);
+    let filter = defaultMatcher.matchesAny(
+      stringifyURL(urlObj),
+      typeMask, documentHost,
+      isThirdParty(urlObj, documentHost),
+      sitekey, specificOnly
+    );
+
+    if (!(filter instanceof BlockingFilter))

[kzar, 2016/03/23 15:35:59]

I think it would be clearer to check if the filter is an instance of
BlockingFilter, removing the need for the continue. Something like this

if (filter instanceof BlockingFilter)
{
  blocked = true;
  if (filter.collapse != null)
    return filter.collapse;
}

[Sebastian Noack, 2016/03/23 16:44:47]

I'm not sure whether I agree, that this is any easier to understand as it
requires another block level, but done.

+      continue;
+    if (filter.collapse != null)
+      return filter.collapse;
+
+    blocked = true;
+  }
+
+  return blocked && Prefs.hidePlaceholders;
+});
+
+let ignoreFilterNotifications = false;
+FilterNotifier.addListener((action, arg) =>

[Sebastian Noack, 2016/03/23 14:02:06]

I hope you don't mind that I revisited the logic to detect relevant filter
changes, since it turned out that both the logic in webrequest.js as well in
whitelisting.js didn't consider some cases, and even worse it was inconsistent.
So I used the chance to fix that.

[kzar, 2016/03/23 15:35:59]

Acknowledged.

 {
-  function onHeadersReceived(details)
+  // Handler behavior is already going to be changed. We do so asynchronously, to

[kzar, 2016/03/23 15:35:59]

Nit: Mind wrapping this long line?

[kzar, 2016/03/23 15:35:59]

I don't really understand this comment. Maybe something like this instead?

"Avoid triggering filters.behaviorChanged multiple times when multiple filter
changes happen at the same time."

[Sebastian Noack, 2016/03/23 16:44:47]

Done.

+  // not trigger multiple times if multiple filter changes occur simultaneously.
+  if (ignoreFilterNotifications)
+    return;
+
+  if (action != "load")
   {
-    var page = new ext.Page({id: details.tabId});
-    var frame = ext.getFrame(details.tabId, details.frameId);
+    let parts = action.split(".");
+    let [category, event] = parts;
+    if (category == "subscription")
+    {
+      if (event != "added"  &&
+          event != "removed" &&
+          event != "updated" &&
+          event != "disabled")
+        return;
 
-    if (!frame || frame.url.href != details.url)
+      // Ignore empty subscriptions. This includes subscriptions
+      // that have just been added, but not downloaded yet.
+      if (arg.filters.length == 0)
+        return;
+    }
+    else if (category == "filter")
+    {
+      if (event != "added" &&
+          event != "removed" &&
+          event != "disabled")
+        return;
+
+      // Ignore all types of filters but request filters,
+      // only these have an effect on the handler behavior.
+      if (!(arg instanceof RegExpFilter))
+        return;
+    }
+    else
       return;
 
-    for (var i = 0; i < details.responseHeaders.length; i++)
-    {
-      var header = details.responseHeaders[i];
-      if (header.name.toLowerCase() == "x-adblock-key" && header.value)
-        processKey(header.value, page, frame);
-    }
+    // Ignore disabled subscriptions and filters, unless they just got
+    // disabled, otherwise they have no effect on the handler behavior.
+    if (arg.disabled && event != "disabled")
+      return;
   }
 
-  chrome.webRequest.onHeadersReceived.addListener(
-    onHeadersReceived,
-    {
-      urls: ["http://*/*", "https://*/*"],
-      types: ["main_frame", "sub_frame"]
-    },
-    ["responseHeaders"]
-  );
-}
+  ignoreFilterNotifications = true;
+  setTimeout(() =>
+  {
+    ignoreFilterNotifications = false;
+    ext.webRequest.handlerBehaviorChanged();
+    FilterNotifier.triggerListeners("filter.behaviorChanged");
+  });
+});