Issue 6544 - Prevent requests sent by Chrome or Adblock Plus from being blocked

lib/devtools.js

Index: lib/devtools.js
===================================================================
--- a/lib/devtools.js
+++ b/lib/devtools.js
@@ -134,13 +134,15 @@
                               thirdParty, sitekey,
                               specificOnly, filter)
 {
-  if (panels.size == 0)

[kzar, 2018/04/05 10:56:37]

How come you removed this check? I agreed with it, why bother iterate through
all the tabs to find listeners if there's no listeners anyway?

[Sebastian Noack, 2018/04/05 17:38:59]

Why bother checking the size if we would only do one lookup anyway? I'm thinking
about the common case, in which tabIds is an array with a single element.

-    return;
-
-  let request = {url, type, docDomain, thirdParty, sitekey, specificOnly};
-  for (let [tabId, panel] of panels)
-    if ((tabIds.length == 0 || tabIds.includes(tabId)) && isActivePanel(panel))
+  for (let tabId of tabIds)
+  {
+    let panel = getActivePanel(tabId);
+    if (panel)
+    {
+      let request = {url, type, docDomain, thirdParty, sitekey, specificOnly};

[kzar, 2018/04/05 10:56:37]

How come we're now assigning request inside the loop? I figure it made more
sense before when we do it outside.

[Sebastian Noack, 2018/04/05 17:38:59]

Most of the time we have only one tabId which most of the time doesn't even have
an active panel. So by creating the object here we avoid creating it
unnecessarily. Though this means if the request is sent by Service Worker
controlling multiple tabs which all have an active devtools panel, we would
create the object redundantly, now. But this is an extremely rare scenario, and
its better to avoid creating the object at all in other (more common) scenarios.

[kzar, 2018/04/06 14:48:10]

Sure, that's why we had the extra check above which I commented about.

[Sebastian Noack, 2018/04/06 17:55:46]

The object was still created redundantly if there was any devtools panel but not
for any of the given tabs. Sure, this would be acceptable, but I don't get why
we should make the logic here more complex than necessary if its already
slightly better optimized for the common if we leave it as simple as it is now.

[kzar, 2018/04/09 11:08:43]

Well I guess we disagree on this but whatever, I won't insist.

       addRecord(panel, request, filter);
+    }
+  }

[Sebastian Noack, 2018/04/05 05:42:45]

I figured now where we don't log requests out of context (to all tabs) anymore,
it makes sense to streamline the logic here.

[Sebastian Noack, 2018/04/05 06:03:58]

For reference, the case of `tabIds.length == 0` right before this change meant
almost all of the time, a request sent by Chrome or Adblock Plus themselves.
The only scenario left where this condition is met are extremely rare scenarios
where oringUrl/intiator cannot be mapped to an open tab.

 };
 
 /**