Issue 6565 - Ignore requests sent by Chrome's new tab page

lib/requestBlocker.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 139 matching lines @@
   if (url.protocol != "http:" && url.protocol != "https:" &&
       url.protocol != "ws:" && url.protocol != "wss:")
     return;
 
   // Firefox provides us with the full origin URL, while Chromium (>=63)
   // provides only the protocol + host of the (top-level) document which
   // the request originates from through the "initiator" property.
   let originUrl = details.originUrl ? new URL(details.originUrl) :
                   details.initiator ? new URL(details.initiator) : null;
 
-  // Ignore requests sent by extensions or by the browser itself:
+  // Ignore requests sent by extensions or by Firefox itself:

[kzar, 2018/04/16 15:53:38]

I feel it's kind of confusing how we both make the logic generic (e.g. originUrl
could be details.originUrl or details.initiatorUrl), but then talk about the
behavior as being specific to browsers at the same time.

I wonder if we'd be better off to make the different code paths for Firefox VS
Chrome more explicit instead:

let {originUrl} = details;
if (originUrl && originUrl.protocol == "mozilla:" ...

[Sebastian Noack, 2018/04/16 16:42:21]

Well, the logic is generic (and covers more than just Chrome and Firefox). Here,
we want to ignore any request that originates from an extension, and that is
what the code expresses. Below, we ignore any request of unknown origin. Also
unless you want to add similar redundancy to checkWhitelisted(),
extractHostFromFrame() and getKey() we need a single URL object anyway. Also
what is about Microsoft Edge? The same logic applies there.

If the comments would match the logic they would be redundant. They exist to
provide additional context, and to explain why this logic is necessary.

   // * Firefox intercepts requests sent by any extensions, indicated with
   //   an "originURL" starting with "moz-extension:".
   // * Chromium intercepts requests sent by this extension only, indicated
   //   on Chromium >=63 with an "initiator" starting with "chrome-extension:".
   // * On Firefox, requests that don't relate to any document or extension are
   //   indicated with an "originUrl" starting with "chrome:".
-  // * On Chromium >=63, requests that don't relate to any document or extension
-  //   have no "initiator". But since on older Chromium versions, no request
-  //   has an "initiator", we have to check for the tabId as well.
-  if (originUrl)
-  {
-    if (originUrl.protocol == extensionProtocol ||
-        originUrl.protocol == "chrome:")
-      return;
-  }
-  else if (details.tabId == -1)
+  if (originUrl && (originUrl.protocol == extensionProtocol ||
+                    originUrl.protocol == "chrome:"))
     return;
 
-  let page = null;
-  let frame = null;
-  if (details.tabId != -1)
-  {
-    page = new ext.Page({id: details.tabId});
-    frame = ext.getFrame(
-      details.tabId,
-      // We are looking for the frame that contains the element which
-      // has triggered this request. For most requests (e.g. images) we
-      // can just use the request's frame ID, but for subdocument requests
-      // (e.g. iframes) we must instead use the request's parent frame ID.
-      details.type == "sub_frame" ? details.parentFrameId : details.frameId
-    );
-  }
+  let page = new ext.Page({id: details.tabId});
+  let frame = ext.getFrame(
+    details.tabId,
+    // We are looking for the frame that contains the element which
+    // has triggered this request. For most requests (e.g. images) we
+    // can just use the request's frame ID, but for subdocument requests
+    // (e.g. iframes) we must instead use the request's parent frame ID.
+    details.type == "sub_frame" ? details.parentFrameId : details.frameId
+  );
 
-  if (checkWhitelisted(page, frame, originUrl))
+  // On Chromium >= 63, if both the frame is unknown and we haven't get
+  // an "initator", this implies a request sent by the browser itself
+  // (on older versions of Chromium, due to the lack of "initator",
+  // this can also indicate a request sent by a Shared/Service Worker).
+  if ((!frame && !originUrl) || checkWhitelisted(page, frame, originUrl))

[kzar, 2018/04/16 15:53:38]

I'd rather not mix new check with the whitelisting check. I think they should be
separate.

[Sebastian Noack, 2018/04/16 16:42:21]

Done.

     return;
 
   let type = resourceTypes.get(details.type) || "OTHER";
   let [docDomain, sitekey, specificOnly] = getDocumentInfo(page, frame,
                                                            originUrl);
   let [filter, urlString, thirdParty] = matchRequest(url, type, docDomain,
                                                      sitekey, specificOnly);
 
   getRelatedTabIds(details).then(tabIds =>
   {
@@ skipping 110 matching lines @@
 }
 
 FilterNotifier.on("subscription.added", onFilterChange);
 FilterNotifier.on("subscription.removed", onFilterChange);
 FilterNotifier.on("subscription.updated", onFilterChange);
 FilterNotifier.on("subscription.disabled", arg => onFilterChange(arg, true));
 FilterNotifier.on("filter.added", onFilterChange);
 FilterNotifier.on("filter.removed", onFilterChange);
 FilterNotifier.on("filter.disabled", arg => onFilterChange(arg, true));
 FilterNotifier.on("load", onFilterChange);