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

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 63 matching lines @@
 }());
 
 function getRelatedTabIds(details)
 {
   // 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]);
 
-  // Firefox >=48 provides "originUrl" indicating the URL of the tab
-  // that caused this request. In case of Service/Shared Worker,
-  // this is URL of the tab that caused the worker to be spawned.
-  if (details.originUrl)

[kzar, 2018/04/03 12:49:30]

Nit: Please use braces since the body spans multiple lines. (same below)

[Sebastian Noack, 2018/04/04 01:04:45]

No longer relevant (the block has only one line now, same below).

-    return browser.tabs.query({url: details.originUrl}).then(
-      tabs => tabs.map(tab => tab.id)
-    );
-
-  // Chromium >=63 provides "intiator" which is equivalent to "originUrl" on
-  // Firefox except that its not a full URL but just an origin (proto + host).
-  // So we have to find each tab with an URL of the same origin.
-  if (details.initiator)
-    return browser.tabs.query({}).then(tabs =>

[kzar, 2018/04/03 12:49:30]

Couldn't we pass something like `details.initiator + "*" ` for the url part of
the query? I figure we could avoid filtering the URLs manually later that way.

https://developer.chrome.com/extensions/tabs#method-query
https://developer.chrome.com/extensions/match_patterns

[Sebastian Noack, 2018/04/04 01:04:45]

In fact we can. I wasn't aware that tab.query() accepts URL patterns. Done.

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

Cool, I assumed I was wrong somehow :p. This looks much cleaner now.

-    {
-      let tabIds = [];
-      for (let tab of tabs)
-      {
-        if (new URL(tab.url).origin == details.initiator)
-          tabIds.push(tab.id);

[kzar, 2018/04/03 12:49:30]

Wouldn't this mean we sometimes log a request for a tab incorrectly, say if the
user has two tabs open for the same domain?

[Sebastian Noack, 2018/04/04 01:04:45]

This usually means that the request was sent by a Service Worker controlling
multiple tabs, in which case logging the request for all those tabs is correct.
Theoretically we can get false positives, e.g. if the request was sent by a
Service Worker controlling https://example.com/foo while there is another tab
for https://example.com/bar which isn't controlled by that Service Worker.
However, by default ServiceWorkers have a domain-wide scope and limiting the
scope requires to jump through some hoops. Also given the limited information,
Chrome provides us with, this seems to be the best we can do. Also FWIW,
currently, those requests are logged to all tabs. So we don't degrade here.

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

Acknowledged.

-      }
-      return tabIds;
-    });
-
-  return Promise.resolve([]);
+  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.
+
+  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);
 
@@ skipping 157 matching lines @@
 
 port.on("request.blockedByRTCWrapper", (msg, sender) =>
 {
   return ext.webRequest.onBeforeRequest._dispatch(
      new URL(msg.url),
      "webrtc",
      sender.page,
      sender.frame
   ).includes(false);
 });