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

lib/devtools.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 116 matching lines @@
  *                                 document differs
  * @param {?string}  sitekey       The active sitekey if there is any
  * @param {?boolean} specificOnly  Whether generic filters should be ignored
  * @param {?BlockingFilter} filter The matched filter or null if there is no
  *                                 match
  */
 exports.logRequest = function(tabIds, url, type, docDomain,
                               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.

 };
 
 /**
  * Logs active element hiding filters to the devtools panel.
  *
  * @param {number}   tabId      The ID of the tab, the elements were hidden in
  * @param {string[]} selectors  The selectors of applied ElemHideFilters
  * @param {string[]} filters    The text of applied ElemHideEmulationFilters
  * @param {string}   docDomain  The IDN-decoded hostname of the document
  */
@@ skipping 220 matching lines @@
   panels.set(inspectedTabId, {port: newPort, records: []});
 });
 
 port.on("devtools.traceElemHide", (message, sender) =>
 {
   logHiddenElements(
     sender.page.id, message.selectors, message.filters,
     extractHostFromFrame(sender.frame)
   );
 });