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

lib/devLogger.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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * 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,
        WhitelistFilter,
        ElemHideFilter} = require("filterClasses");
 const {SpecialSubscription} = require("subscriptionClasses");
 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"];
 
-// 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;
 }
 
 function getFilterInfo(filter)
 {
   if (!filter)
     return null;
 
   let userDefined = false;
   let subscriptionTitle = null;
@@ skipping 10 matching lines @@
   }
 
   return {
     text: filter.text,
     whitelisted: filter instanceof WhitelistFilter,
     userDefined,
     subscription: subscriptionTitle
   };
 }
 
-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 &&
 
     // Ignore partial (e.g. ELEMHIDE) whitelisting if there is already
     // a DOCUMENT exception which disables all means of blocking.
     (record.request.type == "DOCUMENT" ?
        nonRequestTypes.includes(request.type) :
        record.request.type == request.type) &&
 
     // Matched element hiding filters don't relate to a particular request,
     // so we have to compare the selector in order to avoid duplicates.
     (record.filter && record.filter.selector) == (filter && filter.selector)
   );
 }
 
-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});
   }
 }
 
 function matchRequest(request)
 {
   return defaultMatcher.matchesAny(
     request.url,
     RegExpFilter.typeMap[request.type],
     request.docDomain,
     request.thirdParty,
     request.sitekey,
     request.specificOnly
   );
 }
 
 /**
- * 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
  * @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,
+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)
     {
       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);
+          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
  * @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(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))
           continue;
 
         let filter = matchRequest(record.request);
         if (!filters.includes(filter))
           continue;
 
         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.
       else
       {
         if (!filters.includes(record.filter))
           continue;
 
         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,
         filter: getFilterInfo(record.filter)
       });
     }
   }
 }
 
 function onFilterAdded(filter)
 {
   updateFilters([filter], true);
 }
 
 function onFilterRemoved(filter)
 {
   updateFilters([filter], false);
 }
 
 function onSubscriptionAdded(subscription)
 {
   if (subscription instanceof SpecialSubscription)
     updateFilters(subscription.filters, true);
 }
 
-browser.runtime.onConnect.addListener(newPort =>
-{
-  let match = newPort.name.match(/^devtools-(\d+)$/);
-  if (!match)
-    return;
+/**
+ * @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)
+    {
+      ext.pages.onLoading.addListener(onLoading);
+      FilterNotifier.on("filter.added", onFilterAdded);
+      FilterNotifier.on("filter.removed", onFilterRemoved);
+      FilterNotifier.on("subscription.added", onSubscriptionAdded);
+    }
 
-  let inspectedTabId = parseInt(match[1], 10);
-  let localOnBeforeRequest = onBeforeRequest.bind();
+    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
+        }
+      );
+    }
+  },
 
-  browser.webRequest.onBeforeRequest.addListener(
-    localOnBeforeRequest,
+  /**
+   * Removes a callback for a page.
+   *
+   * @param {number}   pageId
+   * @param {function} callback
+   */
+  off(pageId, callback)
+  {
+    let pageLogger = pageLoggers.get(pageId);
+    if (pageLogger.listeners.length > 1)
     {
-      urls: ["http://*/*", "https://*/*"],
-      types: ["main_frame"],
-      tabId: inspectedTabId
+      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)
-  {
-    ext.pages.onLoading.addListener(onLoading);
-    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)
+    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)
   );
 });