Issue 6543 - Match requests without tabId/frameId in their originating context

lib/requestBlocker.js

Index: lib/requestBlocker.js
===================================================================
--- a/lib/requestBlocker.js
+++ b/lib/requestBlocker.js
@@ -73,20 +73,43 @@
   yield "CSP";
 }());
 
-function onBeforeRequestAsync(tabId, url, type, docDomain,
-                              thirdParty, sitekey,
-                              specificOnly, filter)
+function getRelatedTabIds(details)
 {
-  let tabIds = tabId != -1 ? [tabId] : [];
+  // This is the common case, the request is associated with a single tab.
+  // If tabId is -1, its not (e.g. the request was sent by
+  // a Service/Shared Worker) and we have to identify the related tabs.
+  if (details.tabId != -1)
+    return Promise.resolve([details.tabId]);
 
-  if (filter)
-    FilterNotifier.emit("filter.hitCount", filter, 0, 0, tabIds);
+  let url;                    // Firefox provides "originUrl" indicating the
+  if (details.originUrl)      // URL of the tab that caused this request.
+    url = details.originUrl;  // In case of Service/Shared Worker, this is the
+                              // URL of the tab that caused the worker to spawn.
 
-  devtools.logRequest(
-    tabIds, url, type, docDomain,
-    thirdParty, sitekey,
-    specificOnly, filter
-  );
+  else if (details.initiator)        // Chromium >=63 provides "intiator" which
+    url = details.initiator + "/*";  // is equivalent to "originUrl" on Firefox
+                                     // except that its not a full URL but just
+                                     // an origin (proto + host).
+  else
+    return Promise.resolve([]);

[kzar, 2018/04/05 10:31:51]

I wonder if there is a situation where details.initiator isn't provided either,
in which case we will no longer log those requests / hits. Problem is it's not
documented too well. Perhaps do some testing (if you didn't already) to confirm?

[Sebastian Noack, 2018/04/05 16:56:33]

There are two scenarios (I have seen in my testing) where this code path is hit
(i.e. there is neither originUrl nor initiator):

* Always on Chromium <63 (same for Microsoft Edge, which however also
  doesn't support devtools panels)
* On Chromium >=63 if the request is sent by the browser

As of this change (if we resolve with an empty array here), these requests still
get logged (to all tabs), just like before. However, with the next patch under
review, we deliberately stop processing requests sent by the browser, while
other requests we don't find any tabId for won't be logged either anymore. IMO
it's not worth to bother about them if it's just for outdated Chrome versions.

[kzar, 2018/04/09 14:39:18]

Sounds reasonable to me, maybe add a note to the issue to explain that know
limitation?

[Sebastian Noack, 2018/04/09 21:08:20]

There you go: https://issues.adblockplus.org/ticket/6544?action=diff&version=3

+
+  return browser.tabs.query({url}).then(tabs => tabs.map(tab => tab.id));
+}
+
+function logRequest(details, url, type, docDomain, thirdParty,
+                    sitekey, specificOnly, filter)
+{
+  getRelatedTabIds(details).then(tabIds =>
+  {
+    if (filter)
+      FilterNotifier.emit("filter.hitCount", filter, 0, 0, tabIds);
+
+    devtools.logRequest(
+      tabIds, url, type, docDomain,
+      thirdParty, sitekey,
+      specificOnly, filter
+    );
+  });
 }
 
 browser.webRequest.onBeforeRequest.addListener(details =>
@@ -105,55 +128,57 @@
       url.protocol != "ws:" && url.protocol != "wss:")
     return;
 
-  // Firefox (only) allows to intercept requests sent by the browser
-  // and other extensions. We don't want to block these.
+  let originUrl = null;
   if (details.originUrl)
   {
-    let originUrl = new URL(details.originUrl);
+    originUrl = new URL(details.originUrl);
+
+    // Firefox (only) allows to intercept requests sent by the browser
+    // and other extensions. We don't want to block these.
     if (originUrl.protocol == "chrome:" ||
         originUrl.protocol == "moz-extension:")
       return;
   }
-
-  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
-  );
+  // Fallback to "initiator" on Chrome >=63. It doesn't include the
+  // path (unlike "originUrl" on Firefox), but is still good enough
+  // (in case the tab/frame is unknown) for the $domain filter option
+  // and most document exception rules which only match the domain part.
+  else if (details.initiator)
+    originUrl = new URL(details.initiator);
 
-  let docDomain = null;
-  let sitekey = null;
-  let thirdParty = false;
-  let specificOnly = false;
-
-  if (frame)
+  let page = null;
+  let frame = null;
+  if (details.tabId != -1)
   {
-    let page = new ext.Page({id: details.tabId});
+    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
+    );
+  }
 
-    if (checkWhitelisted(page, frame))
-      return;
-
-    docDomain = extractHostFromFrame(frame);
-    sitekey = getKey(page, frame);
-    thirdParty = isThirdParty(url, docDomain);
-    specificOnly = !!checkWhitelisted(page, frame,
-                                      RegExpFilter.typeMap.GENERICBLOCK);
-  }
+  if (checkWhitelisted(page, frame, originUrl))
+    return;
 
   let urlString = stringifyURL(url);
   let type = resourceTypes.get(details.type) || "OTHER";
+  let docDomain = extractHostFromFrame(frame, originUrl);
+  let thirdParty = isThirdParty(url, docDomain);
+  let sitekey = getKey(page, frame, originUrl);
+  let specificOnly = !!checkWhitelisted(page, frame, originUrl,
+                                        RegExpFilter.typeMap.GENERICBLOCK);
+
   let filter = defaultMatcher.matchesAny(
     urlString, RegExpFilter.typeMap[type],
     docDomain, thirdParty, sitekey, specificOnly
   );
 
-  setTimeout(onBeforeRequestAsync, 0, details.tabId, urlString,
-                                      type, docDomain,
-                                      thirdParty, sitekey,
-                                      specificOnly, filter);
+  logRequest(details, urlString, type, docDomain,
+             thirdParty, sitekey, specificOnly, filter);
 
   if (filter instanceof BlockingFilter)
     return {cancel: true};
@@ -170,7 +195,7 @@
   let blocked = false;
 
   let specificOnly = checkWhitelisted(
-    sender.page, sender.frame,
+    sender.page, sender.frame, null,
     RegExpFilter.typeMap.GENERICBLOCK
   );