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);
+  }
+}));
+
+/**
+ * Checks whether a tab is inspected by anything.
+ *
+ * @param {number} tabId
+ * @return {boolean}
+ */
+let hasListener = exports.hasListener = function(tabId)
+{
+  return HitLogger._listeners.has(tabId);
+};
+
+/**
+ * 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
+ */
+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)
+{
+  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);
+    }
+  }
+};
+
+/**
+ * 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
+ */
+function logHiddenElements(tabId, selectors, filters, docDomain)
+{
+  if (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);
+      }
+    }
+  }
+}
+
+/**
+ * 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 {WhitelistFilter} filter The matched whitelisting filter
+ */
+exports.logWhitelistedDocument = function(tabId, url, typeMask, docDomain,
+                                          filter)
+{
+  if (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);
+    }
+  }
+};
+
+port.on("hitLogger.traceElemHide", (message, sender) =>
+{
+  logHiddenElements(
+    sender.page.id, message.selectors, message.filters,
+    extractHostFromFrame(sender.frame)
+  );
+});