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

lib/requestBlocker.js

Index: lib/requestBlocker.js
===================================================================
--- a/lib/requestBlocker.js
+++ b/lib/requestBlocker.js
@@ -157,41 +157,32 @@
   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";