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

lib/hitLogger.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 hitLogger */
 
 "use strict";
 
-const {desc} = require("../adblockpluscore/lib/coreUtils");

[Manish Jethani, 2018/04/23 15:30:58]

I'm assuming the paths here are temporary and will be updated.

[kzar, 2018/05/01 10:53:08]

I'm not sure I understand this comment, but no these paths aren't temporary.

[Manish Jethani, 2018/05/02 13:25:13]

Sorry, I wasn't aware of #5760. It looks right to me now.

 const {extractHostFromFrame} = require("./url");
 const {EventEmitter} = require("../adblockpluscore/lib/events");
 const {FilterStorage} = require("../adblockpluscore/lib/filterStorage");
 const {port} = require("./messaging");
 const {RegExpFilter,
        ElemHideFilter} = require("../adblockpluscore/lib/filterClasses");
 
 const nonRequestTypes = exports.nonRequestTypes = [
   "DOCUMENT", "ELEMHIDE", "GENERICBLOCK", "GENERICHIDE", "CSP"
 ];
 
-let HitLogger = exports.HitLogger = Object.create(new EventEmitter(), desc({

[Manish Jethani, 2018/04/23 12:32:41]

We can get rid of desc here now with Object.assign.

  exports.HitLogger = Object.assign(Object.create(new EventEmitter()), {
    off(tabId, listener)
    {
      ..
    }
  });

It does the same thing but is more straightforward and probably also more
efficient.

[kzar, 2018/04/30 14:20:37]

Out of curiosity what's the point of calling `Object.create` here? In other
words why couldn't we do it like this?

exports.HitLogger = Object.assign(new EventEmitter(), {

[Manish Jethani, 2018/04/30 18:50:16]

It depends what you're trying to do. What I suggested is the exact equivalent of
what was being done. In that case HitLogger instance of EventEmitter is true,
HitLogger has an own property "off", HitLogger.__proto__ has an own property
"_listeners", and HitLogger.__proto__.__proto__ has own properties "on", "off",
"emit", etc. If we use only Object.assign then we'll merge HitLogger and
HitLogger.__proto__ which is fine because it is effectively a singleton, an
instance of EventEmitter that overrides the "off" method at the instance level.

So yeah it should be OK.

For what it's worth I wonder if we could just avoid this by writing hasListener
as:

  return (HitLogger._listeners.get(tabId) || []).length > 0;

We should probably also encapsulate the event emitter so that there's an
addListener and removeListener to complement hasListener. In that case there
would be a private EventEmitter instance in this module and the outside world
wouldn't know how it was managing the events.

  let eventEmitter = new EventEmitter();

  exports.addListener = (tabId, listener) => eventEmitter.on(tabId, listener);
  exports.removeListener = (tabId, listener) => eventEmitter.off(tabId,
listener);

  exports.hasListeners = tabId => (eventEmitter._listeners.get(tabId) ||
[]).length > 0;

[Manish Jethani, 2018/04/30 18:53:54]

Or:

  exports.addListener = eventEmitter.on.bind(eventEmitter);
  exports.removeListener = eventEmitter.off.bind(eventEmitter);

[kzar, 2018/05/01 10:53:08]

Sure I can do it this way if you prefer, Done.

-  off(tabId, listener)
-  {
-    Object.getPrototypeOf(this).off(tabId, listener);
-
-    // EventEmitter leaves an empty array once the last listener for an event
-    // is removed. Usually that's fine, but in this case we need _listeners.size
-    // and _listeners.has to be accurate.
-    let listeners = this._listeners.get(tabId);
-    if (listeners && listeners.length == 0)
-      this._listeners.delete(tabId);
-  }
-}));
+let eventEmitter = new EventEmitter();
 
 /**
- * Checks whether a tab is inspected by anything.
- *
- * @param {number} tabId
- * @return {boolean}
+ * @namespace
+ * @static
  */
-let hasListener = exports.hasListener = function(tabId)
-{
-  return HitLogger._listeners.has(tabId);
+let HitLogger = exports.HitLogger = {
+  /**
+   * Adds a listener for requests, filter hits etc related to the tab.
+   *
+   * Note: Calling code is responsible for removing the listener again,
+   *       it will not be automatically removed when the tab is closed.
+   *
+   * @param {number} tabId
+   * @param {function} listener
+   */
+  addListener: eventEmitter.on.bind(eventEmitter),
+
+  /**
+   * Removes a listener for the tab.
+   *
+   * @param {number} tabId
+   * @param {function} listener
+   */
+  removeListener: eventEmitter.off.bind(eventEmitter),
+
+  /**
+   * Checks whether a tab is being inspected by anything.
+   *
+   * @param {number} tabId
+   * @return {boolean}
+   */
+  hasListener(tabId)
+  {
+    let listeners = eventEmitter._listeners.get(tabId);
+    return listeners && listeners.length > 0;
+  }
 };
 
 /**
  * Logs a request associated with a tab or multiple tabs.
  *
- * @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
+ * @param {number[]} tabIds
+ *   The tabIds associated with the request
+ * @param {Object} request
+ *   The request to log
+ * @param {string} request.url
+ *   The URL of the request
+ * @param {string} request.type
+ *  The request type
+ * @param {string} request.docDomain
+ *  The hostname of the document
+ * @param {boolean} request.thirdParty
+ *   Whether the origin of the request and document differs
+ * @param {?string} request.sitekey
+ *   The active sitekey if there is any
+ * @param {?boolean} request.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,

[Manish Jethani, 2018/04/23 12:32:40]

Since "request" is now a type of object, I'm wondering if this function
shouldn't just be logRequest(tabIds, request, filter)

The documentation above can still expand on what the request object includes.

[kzar, 2018/05/01 10:53:08]

Done.

-                              thirdParty, sitekey,
-                              specificOnly, filter)
+exports.logRequest = (tabIds, request, filter) =>
 {
   for (let tabId of tabIds)
-  {
-    if (hasListener(tabId))

[Manish Jethani, 2018/04/23 12:32:40]

Seems unnecessary to call hasListener here. If there's no listener then nothing
will happen, the emit function calls _listeners.get anyway.

[kzar, 2018/05/01 10:53:07]

Done.

-    {
-      let request = {url, type, docDomain, thirdParty, sitekey, specificOnly};

[kzar, 2018/04/13 15:25:03]

Note: I'm not adding the tabId in here any more Thomas since with Sebastian's
recent changes we're making use of the initiatorUrl / originUrl fields to figure
out which tabs requests are associated with.

-      HitLogger.emit(tabId, request, filter);
-    }
-  }
+    eventEmitter.emit(tabId, request, filter);
 };
 
 /**
  * Logs active element hiding filters for a tab.
  *
  * @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
+ * @param {string}   docDomain  The hostname of the document
  */
 function logHiddenElements(tabId, selectors, filters, docDomain)
 {
-  if (hasListener(tabId))
+  if (HitLogger.hasListener(tabId))
   {
     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))
-          HitLogger.emit(tabId, {type: "ELEMHIDE", docDomain}, filter);
+          eventEmitter.emit(tabId, {type: "ELEMHIDE", docDomain}, filter);
       }
     }
   }
 }
 
 /**
  * Logs a whitelisting filter that disables (some kind of)
  * blocking for a particular document.
  *
  * @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 {string}       docDomain The hostname of the parent document
  * @param {WhitelistFilter} filter The matched whitelisting filter
  */
-exports.logWhitelistedDocument = function(tabId, url, typeMask, docDomain,
-                                          filter)
+exports.logWhitelistedDocument = (tabId, url, typeMask, docDomain, filter) =>
 {
-  if (hasListener(tabId))
+  if (HitLogger.hasListener(tabId))
   {
     for (let type of nonRequestTypes)

[Manish Jethani, 2018/04/23 12:32:40]

We can avoid this for loop here. nonRequestTypes should be declared like so:

  nonRequestTypes = RegExpFilter.typeMap.DOCUMENT |
                    RegExpFilter.typeMap.ELEMHIDE |
                    RegExpFilter.typeMap.GENERICBLOCK |
                    RegExpFilter.typeMap.GENERICHIDE |
                    RegExpFilter.typeMap.CSP;

Then we can simply do:

  if (typeMask & filter.contentType & nonRequestTypes)
    HitLogger.emit(tabId, {url, type, domain}, filter);

This way we can also avoid the hasListener check above.

[Manish Jethani, 2018/04/23 13:32:03]

Sorry I misunderstood, so we want to know which of the nonRequestTypes matches
and emit an event for each one. In that case this won't work, please ignore this
comment.

[kzar, 2018/05/01 10:53:07]

Acknowledged.

     {
       if (typeMask & filter.contentType & RegExpFilter.typeMap[type])
-        HitLogger.emit(tabId, {url, type, docDomain}, filter);
+        eventEmitter.emit(tabId, {url, type, docDomain}, filter);
     }
   }
 };
 
 port.on("hitLogger.traceElemHide", (message, sender) =>
 {
   logHiddenElements(
     sender.page.id, message.selectors, message.filters,
     extractHostFromFrame(sender.frame)
   );
 });