Issue 5042 - Adds handling for requests which are not associated with browser tab

lib/devtools.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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 14 matching lines @@
 const {defaultMatcher} = require("matcher");
 const {FilterNotifier} = require("filterNotifier");
 const {extractHostFromFrame} = require("url");
 const {port} = require("messaging");
 
 const nonRequestTypes = ["DOCUMENT", "ELEMHIDE", "GENERICBLOCK", "GENERICHIDE"];
 
 // Mapping of inspected tabs to their devpanel page
 // and recorded items. We can't use a PageMap here,
 // because data must persist after navigation/reload.
-let panels = Object.create(null);
-
-function hasPanels()
-{
-  return Object.keys(panels).length > 0;
+let panels = new Map();
+
+function isActivePanel(panel)
+{
+  return panel && !panel.reload && !panel.reloading;
 }
 
 function getActivePanel(page)
 {
-  let panel = panels[page.id];
-  if (panel && !panel.reload && !panel.reloading)
+  let panel = panels.get(page.id);
+  if (isActivePanel(panel))
     return panel;
   return null;
 }
 
 function getFilterInfo(filter)
 {
   if (!filter)
     return null;
 
   let userDefined = false;
@@ skipping 58 matching lines @@
     request.docDomain,
     request.thirdParty,
     request.sitekey,
     request.specificOnly
   );
 }
 
 /**
  * Logs a request to the devtools panel.
  *
- * @param {Page}     page          The page the request occured on
+ * @param {?Page}    page          The page the request occured on or null if
+ *                                 the request isn't associated with a page
  * @param {string}   url           The URL of the request
  * @param {string}   type          The request type
  * @param {string}   docDomain     The IDN-decoded hostname of the document
  * @param {boolean}  thirdParty    Whether the origin of the request and
  *                                 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(page, url, type, docDomain,
                               thirdParty, sitekey,
                               specificOnly, filter)
 {
+  if (panels.size == 0)
+    return;
+
   let request = {url, type, docDomain, thirdParty, sitekey, specificOnly};

[Sebastian Noack, 2017/04/21 08:39:53]

Perhaps, we should bail if there are no panels, before we create new objects and
do other stuff, to make sure to keep the overhead as small as possible when the
devtools panel isn't used.

[Jon Sonesen, 2017/04/24 08:40:56]

Acknowledged.

-  if (page)
-  {
-    let panel = getActivePanel(page);
-    if (panel)
+  for (let [tabId, panel] of panels)
+    if ((!page || page.id == tabId) && isActivePanel(panel))
       addRecord(panel, request, filter);
-    return;
-  }
-  for (let panelId in panels)

[Sebastian Noack, 2017/04/21 08:39:53]

Some of the code added here, and in this file in general, could be simplified by
turning panels into a Map object, which is also in line with our coding
practices, but back when this code was originally written, the Map object wasn't
supported yet in Chrome. This might be a good opportunity to clean this up.

[Jon Sonesen, 2017/04/24 08:40:56]

Awesome, I agree here. I am wondering though, should I adapt the whole file for
this change or just convert to a map here in the function? I am happy to adapt
the initialization of the panels and the rest of the code path but perhaps it is
better to then open a separate issue?

-    addRecord(panels[panelId], request, filter);

[Sebastian Noack, 2017/04/21 08:39:53]

getActivePanel(), called when logging for a specific page, is excluding panels
of pages that are reloading, we should do the same here.

[Jon Sonesen, 2017/04/24 08:40:56]

Acknowledged.

 };
 
 /**
  * Logs active element hiding filters to the devtools panel.
  *
  * @param {Page}     page       The page the elements were hidden on
  * @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 51 matching lines @@
 };
 
 /**
  * Checks whether a page is inspected by the devtools panel.
  *
  * @param {Page} page
  * @return {boolean}
  */
 exports.hasPanel = function(page)
 {
-  return page.id in panels;
+  return panels.has(page.id);
 };
 
 function onBeforeRequest(details)
 {
-  let panel = panels[details.tabId];
+  let panel = panels.get(details.tabId);
 
   // Clear the devtools panel and reload the inspected tab without caching
   // when a new request is issued. However, make sure that we don't end up
   // in an infinite recursion if we already triggered a reload.
   if (panel.reloading)
   {
     panel.reloading = false;
   }
   else
   {
     panel.records = [];
     panel.port.postMessage({type: "reset"});
 
     // We can't repeat the request if it isn't a GET request. Chrome would
     // prompt the user to confirm reloading the page, and POST requests are
     // known to cause issues on many websites if repeated.
     if (details.method == "GET")
       panel.reload = true;
   }
 }
 
 function onLoading(page)
 {
   let tabId = page.id;
-  let panel = panels[tabId];
+  let panel = panels.get(tabId);
 
   // Reloading the tab is the only way that allows bypassing all caches, in
   // order to see all requests in the devtools panel. Reloading must not be
   // performed before the tab changes to "loading", otherwise it will load the
   // previous URL.
   if (panel && panel.reload)
   {
     chrome.tabs.reload(tabId, {bypassCache: true});
 
     panel.reload = false;
     panel.reloading = true;
   }
 }
 
 function updateFilters(filters, added)
 {
-  for (let tabId in panels)
-  {
-    let panel = panels[tabId];
-
+  for (let panel of panels.values())
+  {
     for (let i = 0; i < panel.records.length; i++)
     {
       let record = panel.records[i];
 
       // If an added filter matches a request shown in the devtools panel,
       // update that record to show the new filter. Ignore filters that aren't
       // associated with any sub-resource request. There is no record for these
       // if they don't already match. In particular, in case of element hiding
       // filters, we also wouldn't know if any new element matches.
       if (added)
@@ skipping 68 matching lines @@
 
   chrome.webRequest.onBeforeRequest.addListener(
     localOnBeforeRequest,
     {
       urls: ["http://*/*", "https://*/*"],
       types: ["main_frame"],
       tabId: inspectedTabId
     }
   );
 
-  if (!hasPanels())
+  if (panels.size == 0)
   {
     ext.pages.onLoading.addListener(onLoading);
     FilterNotifier.on("filter.added", onFilterAdded);
     FilterNotifier.on("filter.removed", onFilterRemoved);
     FilterNotifier.on("subscription.added", onSubscriptionAdded);
   }
 
   newPort.onDisconnect.addListener(() =>
   {
-    delete panels[inspectedTabId];
+    panels.delete(inspectedTabId);
     chrome.webRequest.onBeforeRequest.removeListener(localOnBeforeRequest);
 
-    if (!hasPanels())
+    if (panels.size == 0)
     {
       ext.pages.onLoading.removeListener(onLoading);
       FilterNotifier.off("filter.added", onFilterAdded);
       FilterNotifier.off("filter.removed", onFilterRemoved);
       FilterNotifier.off("subscription.added", onSubscriptionAdded);
     }
   });
 
-  panels[inspectedTabId] = {port: newPort, records: []};
+  panels.set(inspectedTabId, {port: newPort, records: []});
 });
 
 port.on("devtools.traceElemHide", (message, sender) =>
 {
   logHiddenElements(
     sender.page, message.selectors, message.filters,
     extractHostFromFrame(sender.frame)
   );
 });