Issue 4580 - Make filter/request logging use plain tabIds, prepare for multi-tab requests

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 13 matching lines @@
 const {FilterStorage} = require("filterStorage");
 const {defaultMatcher} = require("matcher");
 const {FilterNotifier} = require("filterNotifier");
 const {extractHostFromFrame} = require("url");
 const {port} = require("messaging");
 
 const nonRequestTypes = ["DOCUMENT", "ELEMHIDE",
                          "GENERICBLOCK", "GENERICHIDE", "CSP"];
 
 // Mapping of inspected tabs to their devpanel page
 // and recorded items. We can't use a PageMap here,

[kzar, 2018/04/03 17:14:03]

Maybe remove/change this comment since it wouldn't be consistent to use a
PageMap anymore anyway?

[Sebastian Noack, 2018/04/04 01:50:33]

Well spotted. Done.

 // because data must persist after navigation/reload.
 let panels = new Map();
 
 function isActivePanel(panel)
 {
   return panel && !panel.reload && !panel.reloading;
 }
 
-function getActivePanel(page)
+function getActivePanel(tabId)
 {
-  let panel = panels.get(page.id);
+  let panel = panels.get(tabId);
   if (isActivePanel(panel))
     return panel;
   return null;
 }
 
 function getFilterInfo(filter)
 {
   if (!filter)
     return null;
 
@@ skipping 59 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 or null if
- *                                 the request isn't associated with a page
+ * @param {number[]} tabIds        The tabIds associated with the request
  * @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,
+exports.logRequest = function(tabIds, url, type, docDomain,
                               thirdParty, sitekey,
                               specificOnly, filter)
 {
   if (panels.size == 0)
     return;
 
   let request = {url, type, docDomain, thirdParty, sitekey, specificOnly};
   for (let [tabId, panel] of panels)
-    if ((!page || page.id == tabId) && isActivePanel(panel))
+    if ((tabIds.length == 0 || tabIds.includes(tabId)) && isActivePanel(panel))
       addRecord(panel, request, filter);
 };
 
 /**
  * Logs active element hiding filters to the devtools panel.
  *
- * @param {Page}     page       The page the elements were hidden on
+ * @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
  */
-function logHiddenElements(page, selectors, filters, docDomain)
+function logHiddenElements(tabId, selectors, filters, docDomain)
 {
-  let panel = getActivePanel(page);
+  let panel = getActivePanel(tabId);
   if (panel)
   {
     for (let subscription of FilterStorage.subscriptions)
     {
       if (subscription.disabled)
         continue;
 
       for (let filter of subscription.filters)
       {
         // We only know the exact filter in case of element hiding emulation.
         // For regular element hiding filters, the content script only knows
         // the selector, so we have to find a filter that has an identical
         // selector and is active on the domain the match was reported from.
         let isActiveElemHideFilter = filter instanceof ElemHideFilter &&
                                      selectors.includes(filter.selector) &&
                                      filter.isActiveOnDomain(docDomain);
 
         if (isActiveElemHideFilter || filters.includes(filter.text))
           addRecord(panel, {type: "ELEMHIDE", docDomain}, filter);
       }
     }
   }
 }
 
 /**
  * Logs a whitelisting filter, that disables (some kind of)
  * blocking for a particular document, to the devtools panel.
  *
- * @param {Page}         page      The page the whitelisting is active on
+ * @param {number}       tabId     The tabId the whitelisting is active for
  * @param {string}       url       The url of the whitelisted document
  * @param {number}       typeMask  The bit mask of whitelisting types checked
  *                                 for
  * @param {string}       docDomain The IDN-decoded hostname of the parent
  *                                 document
  * @param {WhitelistFilter} filter The matched whitelisting filter
  */
-exports.logWhitelistedDocument = function(page, url, typeMask, docDomain,
+exports.logWhitelistedDocument = function(tabId, url, typeMask, docDomain,
                                           filter)
 {
-  let panel = getActivePanel(page);
+  let panel = getActivePanel(tabId);
   if (panel)
   {
     for (let type of nonRequestTypes)
     {
       if (typeMask & filter.contentType & RegExpFilter.typeMap[type])
         addRecord(panel, {url, type, docDomain}, filter);
     }
   }
 };
 
 /**
- * Checks whether a page is inspected by the devtools panel.
+ * Checks whether a tab is inspected by the devtools panel.
  *
- * @param {Page} page
+ * @param {number} tabId
  * @return {boolean}
  */
-exports.hasPanel = function(page)
+exports.hasPanel = function(tabId)
 {
-  return panels.has(page.id);
+  return panels.has(tabId);
 };
 
 function onBeforeRequest(details)
 {
   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)
@@ skipping 118 matching lines @@
     localOnBeforeRequest,
     {
       urls: ["http://*/*", "https://*/*"],
       types: ["main_frame"],
       tabId: inspectedTabId
     }
   );
 
   if (panels.size == 0)
   {
     ext.pages.onLoading.addListener(onLoading);

[kzar, 2018/04/03 17:14:03]

What about these? Well I don't mind if we remove them later, but in which case I
guess the codereview title (commit message) should be tweaked?

[Sebastian Noack, 2018/04/04 01:50:33]

There is quite some magic around the ext.pages.onLoading event. Hence there is
no trivial replacement using the plain browser.* API. It might be possible
though that not all of that magic is necessary for the devtools panel, but I
would have to verify that. So I'd rather get rid of ext.page.onLoading
(potentially not only here) with a separate change. But I updated the review
title (and commit message) to be more specific, as you requested.

     FilterNotifier.on("filter.added", onFilterAdded);
     FilterNotifier.on("filter.removed", onFilterRemoved);
     FilterNotifier.on("subscription.added", onSubscriptionAdded);
   }
 
   newPort.onDisconnect.addListener(() =>
   {
     panels.delete(inspectedTabId);
     browser.webRequest.onBeforeRequest.removeListener(localOnBeforeRequest);
 
     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.set(inspectedTabId, {port: newPort, records: []});
 });
 
 port.on("devtools.traceElemHide", (message, sender) =>
 {
   logHiddenElements(
-    sender.page, message.selectors, message.filters,
+    sender.page.id, message.selectors, message.filters,
     extractHostFromFrame(sender.frame)
   );
 });