Issue 6402 - Split filter hit / request logging out into own API

lib/devLogger.js

Index: lib/devLogger.js
diff --git a/lib/devtools.js b/lib/devLogger.js
similarity index 60%
copy from lib/devtools.js
copy to lib/devLogger.js
index 167a7650e990cb3819b50d719a3e25501caf9e5f..6e13cafe9a7c6557b3c14e498578b0b3ed30a270 100644
--- a/lib/devtools.js
+++ b/lib/devLogger.js
@@ -15,6 +15,8 @@
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+/** @module devLogger */

[Sebastian Noack, 2018/03/20 01:28:26]

I'm not too happy with the name "devLogger". It is meant to be an abstraction of
the devtools use case, but yet it relates to it in the name. More accurate would
be "filterAndRequestLogger", this however looks quite ugly and long. So the best
I can come up with, at the moment, would be just "logger". What do you think?

[kzar, 2018/03/21 15:18:45]

I agree the name sucked but I'm also struggling to do much better. How about
hitLogger? logger seems a bit vague to me.

+
 "use strict";
 
 const {RegExpFilter,
@@ -29,21 +31,21 @@ const {port} = require("messaging");
 
 const nonRequestTypes = ["DOCUMENT", "ELEMHIDE", "GENERICBLOCK", "GENERICHIDE"];
 
-// Mapping of inspected tabs to their devpanel page
+// Mapping of inspected tabs to their listeners
 // and recorded items. We can't use a PageMap here,
 // because data must persist after navigation/reload.
-let panels = new Map();
+let pageLoggers = new Map();
 
-function isActivePanel(panel)
+function isActivePageLogger(pageLogger)
 {
-  return panel && !panel.reload && !panel.reloading;
+  return pageLogger && !pageLogger.reload && !pageLogger.reloading;
 }
 
-function getActivePanel(page)
+function getActivePageLogger(pageId)
 {
-  let panel = panels.get(page.id);
-  if (isActivePanel(panel))
-    return panel;
+  let pageLogger = pageLoggers.get(pageId);
+  if (isActivePageLogger(pageLogger))
+    return pageLogger;
   return null;
 }
 
@@ -74,9 +76,9 @@ function getFilterInfo(filter)
   };
 }
 
-function hasRecord(panel, request, filter)
+function hasRecord(pageLogger, request, filter)
 {
-  return panel.records.some(record =>
+  return pageLogger.records.some(record =>
     record.request.url == request.url &&
     record.request.docDomain == request.docDomain &&
 
@@ -92,17 +94,23 @@ function hasRecord(panel, request, filter)
   );
 }
 
-function addRecord(panel, request, filter)
+function emit(pageLogger, ...args)
 {
-  if (!hasRecord(panel, request, filter))
+  for (let listener of pageLogger.listeners.slice())
+    listener(...args);
+}
+
+function addRecord(pageLogger, request, filter)
+{
+  if (!hasRecord(pageLogger, request, filter))
   {
-    panel.port.postMessage({
+    emit(pageLogger, {
       type: "add-record",

[Sebastian Noack, 2018/03/20 01:28:25]

This abstraction is specific to the devtools panel.

1. Filters are merely serialized so that they can be send to another process.
2. Responding to filter changes (i.e. the "update-record" event) and page reload
(the "reset" event) would make the implementation of the issue reporter just
unnecessarily complicated, without any benefit.
3. The hasRecord() check exposes a leaky abstraction if there is more than one
listener. One listener might not get a record if the same record has already
been reported to another listener that has been registered earlier.

I think the listeners registered through this module should simply be called for
each filter hit and request reported though the log*() functions. Higher-level
logic specific to the devtools panel should stay in the devtools module.

       request,
       filter: getFilterInfo(filter)
     });
 
-    panel.records.push({request, filter});
+    pageLogger.records.push({request, filter});
   }
 }
 
@@ -119,7 +127,8 @@ function matchRequest(request)
 }
 
 /**
- * Logs a request to the devtools panel.
+ * Logs a request for a page if there's something (e.g. devtools panel)
+ * listening.
  *
  * @param {?Page}    page          The page the request occured on or null if

[Sebastian Noack, 2018/03/20 01:28:25]

It seems you turned all other "page" parameters into "pageId" (which I
absolutely support, in regard of getting rid of ext.* foo), but why not here?

Furthermore, I would rather call it tabId instead of pageId to match the
browser.* API terminology.

[kzar, 2018/03/21 15:18:45]

Done.

  *                                 the request isn't associated with a page
@@ -133,31 +142,32 @@ function matchRequest(request)
  * @param {?BlockingFilter} filter The matched filter or null if there is no
  *                                 match
  */
-exports.logRequest = function(page, url, type, docDomain,
-                              thirdParty, sitekey,
+exports.logRequest = function(page, url, type, docDomain, thirdParty, sitekey,
                               specificOnly, filter)
 {
-  if (panels.size == 0)
+  if (pageLoggers.size == 0)
     return;
 
-  let request = {url, type, docDomain, thirdParty, sitekey, specificOnly};
-  for (let [tabId, panel] of panels)
-    if ((!page || page.id == tabId) && isActivePanel(panel))
-      addRecord(panel, request, filter);
+  let request = {url, type, docDomain, thirdParty, sitekey, specificOnly,
+                 pageId: page ? page.id : -1};
+  for (let [pageId, pageLogger] of pageLoggers)
+    if ((!page || page.id == pageId) && isActivePageLogger(pageLogger))
+      addRecord(pageLogger, request, filter);
 };
 
 /**
- * Logs active element hiding filters to the devtools panel.
+ * Logs active element hiding filters for a page if there's something's
+ * (e.g. devtools panel) listening.
  *
- * @param {Page}     page       The page the elements were hidden on
+ * @param {int}      pageId     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
  */
-function logHiddenElements(page, selectors, filters, docDomain)
+exports.logHiddenElements = function(pageId, selectors, filters, docDomain)
 {
-  let panel = getActivePanel(page);
-  if (panel)
+  let pageLogger = getActivePageLogger(pageId);
+  if (pageLogger)
   {
     for (let subscription of FilterStorage.subscriptions)
     {
@@ -175,17 +185,17 @@ function logHiddenElements(page, selectors, filters, docDomain)
                                      filter.isActiveOnDomain(docDomain);
 
         if (isActiveElemHideFilter || filters.includes(filter.text))
-          addRecord(panel, {type: "ELEMHIDE", docDomain}, filter);
+          addRecord(pageLogger, {type: "ELEMHIDE", docDomain}, filter);
       }
     }
   }
-}
+};
 
 /**
- * Logs a whitelisting filter, that disables (some kind of)
- * blocking for a particular document, to the devtools panel.
+ * Logs a whitelisting filter, that disables (some kind of) blocking
+ * for a particular document if a devtools panel or similiar is listening.
  *
- * @param {Page}         page      The page the whitelisting is active on
+ * @param {number}       pageId    The page the whitelisting is active on
  * @param {string}       url       The url of the whitelisted document
  * @param {number}       typeMask  The bit mask of whitelisting types checked
  *                                 for
@@ -193,86 +203,76 @@ function logHiddenElements(page, selectors, filters, docDomain)
  *                                 document
  * @param {WhitelistFilter} filter The matched whitelisting filter
  */
-exports.logWhitelistedDocument = function(page, url, typeMask, docDomain,
+exports.logWhitelistedDocument = function(pageId, url, typeMask, docDomain,
                                           filter)
 {
-  let panel = getActivePanel(page);
-  if (panel)
+  let pageLogger = getActivePageLogger(pageId);
+  if (pageLogger)
   {
     for (let type of nonRequestTypes)
     {
       if (typeMask & filter.contentType & RegExpFilter.typeMap[type])
-        addRecord(panel, {url, type, docDomain}, filter);
+        addRecord(pageLogger, {pageId, url, type, docDomain}, filter);
     }
   }
 };
 
-/**
- * Checks whether a page is inspected by the devtools panel.
- *
- * @param {Page} page
- * @return {boolean}
- */
-exports.hasPanel = function(page)
-{
-  return panels.has(page.id);
-};
 
 function onBeforeRequest(details)
 {
-  let panel = panels.get(details.tabId);
+  let pageLogger = pageLoggers.get(details.tabId);
 
-  // Clear the devtools panel and reload the inspected tab without caching
+  // Clear the records 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)
+  if (pageLogger.reloading)
   {
-    panel.reloading = false;
+    pageLogger.reloading = false;
   }
   else
   {
-    panel.records = [];
-    panel.port.postMessage({type: "reset"});
+    pageLogger.records = [];
+    emit(pageLogger, {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;
+      pageLogger.reload = true;
   }
 }
 
 function onLoading(page)
 {
   let tabId = page.id;
-  let panel = panels.get(tabId);
+  let pageLogger = pageLoggers.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)
+  if (pageLogger && pageLogger.reload)
   {
     browser.tabs.reload(tabId, {bypassCache: true});
 
-    panel.reload = false;
-    panel.reloading = true;
+    pageLogger.reload = false;
+    pageLogger.reloading = true;
   }
 }
 
 function updateFilters(filters, added)
 {
-  for (let panel of panels.values())
+  for (let pageLogger of pageLoggers.values())
   {
-    for (let i = 0; i < panel.records.length; i++)
+    for (let i = 0; i < pageLogger.records.length; i++)
     {
-      let record = panel.records[i];
+      let record = pageLogger.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 an added filter matches a logged request,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)
       {
         if (nonRequestTypes.includes(record.request.type))
@@ -285,8 +285,8 @@ function updateFilters(filters, added)
         record.filter = filter;
       }
 
-      // If a filter shown in the devtools panel got removed, update that
-      // record to show the filter that matches now, or none, instead.
+      // If a logged filter got removed, update that record to show the filter
+      // that matches now, or none, instead.
       // For filters that aren't associated with any sub-resource request,
       // just remove the record. We wouldn't know whether another filter
       // matches instead until the page is reloaded.
@@ -297,18 +297,15 @@ function updateFilters(filters, added)
 
         if (nonRequestTypes.includes(record.request.type))
         {
-          panel.port.postMessage({
-            type: "remove-record",
-            index: i
-          });
-          panel.records.splice(i--, 1);
+          emit(pageLogger, {type: "remove-record", index: i});
+          pageLogger.records.splice(i--, 1);
           continue;
         }
 
         record.filter = matchRequest(record.request);
       }
 
-      panel.port.postMessage({
+      emit(pageLogger, {
         type: "update-record",
         index: i,
         request: record.request,
@@ -334,52 +331,98 @@ function onSubscriptionAdded(subscription)
     updateFilters(subscription.filters, true);
 }
 
-browser.runtime.onConnect.addListener(newPort =>
-{
-  let match = newPort.name.match(/^devtools-(\d+)$/);
-  if (!match)
-    return;
-
-  let inspectedTabId = parseInt(match[1], 10);
-  let localOnBeforeRequest = onBeforeRequest.bind();
-
-  browser.webRequest.onBeforeRequest.addListener(
-    localOnBeforeRequest,
+/**
+ * @namespace
+ * @static
+ */
+let DevLogger = exports.DevLogger = {
+  /**
+   * Adds a callback that is called when a request
+   * or filter hit is logged for the page.
+   *
+   * @param {number}   pageId
+   * @param {function} callback
+   */
+  on(pageId, callback)
+  {
+    if (pageLoggers.size == 0)
     {
-      urls: ["http://*/*", "https://*/*"],
-      types: ["main_frame"],
-      tabId: inspectedTabId
+      ext.pages.onLoading.addListener(onLoading);
+      FilterNotifier.on("filter.added", onFilterAdded);
+      FilterNotifier.on("filter.removed", onFilterRemoved);
+      FilterNotifier.on("subscription.added", onSubscriptionAdded);
     }
-  );
 
-  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(() =>
+    if (pageLoggers.has(pageId))
+    {
+      pageLoggers.get(pageId).listeners.push(callback);
+    }
+    else
+    {
+      let localOnBeforeRequest = onBeforeRequest.bind();
+      pageLoggers.set(pageId, {
+        listeners: [callback], records: [],
+        reloading: false, reload: false,
+        localOnBeforeRequest
+      });
+      browser.webRequest.onBeforeRequest.addListener(
+        localOnBeforeRequest,
+        {
+          urls: ["http://*/*", "https://*/*"],
+          types: ["main_frame"],
+          tabId: pageId
+        }
+      );
+    }
+  },
+
+  /**
+   * Removes a callback for a page.
+   *
+   * @param {number}   pageId
+   * @param {function} callback
+   */
+  off(pageId, callback)
   {
-    panels.delete(inspectedTabId);
-    browser.webRequest.onBeforeRequest.removeListener(localOnBeforeRequest);
+    let pageLogger = pageLoggers.get(pageId);
+    if (pageLogger.listeners.length > 1)
+    {
+      let idx = pageLogger.listeners.indexOf(callback);
+      if (idx != -1)
+        pageLogger.listeners.splice(idx, 1);
+    }
+    else
+    {
+      browser.webRequest.onBeforeRequest.removeListener(
+        pageLogger.localOnBeforeRequest
+      );
+      pageLoggers.delete(pageId);

[Sebastian Noack, 2018/03/20 01:28:24]

Is this logic correct? So if we have only one listener left, for that page, we
implicitly assume that it matches the given callback, or do I miss something?

+    }
 
-    if (panels.size == 0)
+    if (pageLoggers.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: []});
-});
+  },
+
+  /**
+   * Checks whether a page has any devLogging listeners active.
+   *
+   * @param {number}  pageId
+   * @return {boolean}
+   */
+  listening(pageId)
+  {
+    return pageLoggers.has(pageId);
+  }
+};
 
-port.on("devtools.traceElemHide", (message, sender) =>
+port.on("devlogger.traceElemHide", (message, sender) =>
 {
-  logHiddenElements(
+  exports.logHiddenElements(
     sender.page, message.selectors, message.filters,
     extractHostFromFrame(sender.frame)
   );