Split out request / hit logging from devtools code into separate API

[kzar]

Background

With issue #154 we added our own Adblock Plus pane to the developer tools which lists requests and filter hits for a page. It's useful for filter list authors to understand what's going on.

The issue reporter for the legacy Firefox extension provided similar information, we'd like to do the same for the WebExt extension #6386.

Since there are now two consumers for the information that we're logging for the developer tools pane it makes sense to split out the logging part into a separate API.

What to change

Create the module lib/hitLogger.js in adblockpluschrome with an API something like this:

exports.nonRequestTypes = [
  "DOCUMENT", "ELEMHIDE", "GENERICBLOCK", "GENERICHIDE", "CSP"
];

/**
 * @namespace
 * @static
 */
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(tabId, listener)

  /**
   * Removes a listener for the tab.
   *
   * @param {number} tabId
   * @param {function} listener
   */
  removeListener(tabId, listener)

  /**
   * Checks whether a tab is being inspected by anything.
   *
   * @param {number} tabId
   * @return {boolean}
   */
  hasListener(tabId)
}

/**
 * Logs a request associated with a tab or multiple tabs.
 *
 * @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, request, 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 hostname of the parent document
 * @param {WhitelistFilter} filter The matched whitelisting filter
 */
exports.logWhitelistedDocument = function(tabId, url, typeMask, docDomain, filter)

port.on("hitLogger.traceElemHide", (message, sender) =>
{
   ...
});

Listeners should be called back with a request and filter:

listener(request, filter).

Where a request might look something like:

{"pageId":123,"url":"https://example.com/example.jpg","type":"IMAGE","docDomain":"www.reddit.com","thirdParty":true,"sitekey":null,"specificOnly":false}

and a filter could be an element hiding, whitelisting or blocking filter - or null if there is no filter associate e.g. we're logging a request rather than a filter hit.

Hints for testers

• Ensure that the Adblock Plus developer tools pane still works as before. Test as much functionality as possible, for example blocking a request and whitelisting it again from the developer tools pane.
• Ensure element hiding still workings on Microsoft Edge.

[kzar]

Please confirm this looks OK everyone and I'll mark it ready. In the mean time I'm starting an implementation.

[greiner]

Looking good. Sounds like the only thing that's not part of this is the endpoint with which we'd connect to the background page. If so, that's great because it'd allow us to simply extend message responder.

Would we also get requests from background tabs or only the ones the extension is able to associate with the pageId that we listen to? It'd probably be helpful to listen to such requests independently because I don't know whether we want to include those in issue reports whereas it makes sense to show them in the the DevTools pane.
Being able to differentiate between those would also be helpful for #5093 BTW.

[kzar]

Replying to greiner:

Would we also get requests from background tabs or only the ones the extension is able to associate with the pageId that we listen to? It'd probably be helpful to listen to such requests independently because I don't know whether we want to include those in issue reports whereas it makes sense to show them in the the DevTools pane.
Being able to differentiate between those would also be helpful for #5093 BTW.

Honestly I'm not sure yet, I think once I have a proof of concept working I'll have a better idea.

[mapx]

So, still using devtools ? If you want the normal users use such feature you could consider having a logger in a separate tab (and have a keyboard shortcut to open it). You already have the uBo example which log all the requests even those "behind the scene", you can implement all the search features you want and finally have the same logger for all browsers.

[kzar]

Replying to mapx:

So, still using devtools ? If you want the normal users use such feature you could consider having a logger in a separate tab (and have a keyboard shortcut to open it). You already have the uBo example which log all the requests even those "behind the scene", you can implement all the search features you want and finally have the same logger for all browsers.

That is something else mapx, but FWIW the work in this issue might potentially help with it.

[greiner]

@mapx I've created #6404 so that we can gather what's required for such a page. Hopefully, we should be able to extend the existing DevTools pane to also work as a standalone page to avoid any unnecessary duplication.

Depending on how that page turns out we may then also want to investigate the added value the integration into the DevTools window provides and whether such a standalone page would render it redundant.

[kzar]

Would we also get requests from background tabs or only the ones the extension is able to associate with the pageId that we listen to? It'd probably be helpful to listen to such requests independently because I don't know whether we want to include those in issue reports whereas it makes sense to show them in the the DevTools pane.

So you'll get both, but ​the pageId is now given for requests so it should simply be a matter of checking if that's -1 or not.

[kzar]

The API is going to change quite a bit so the issue description is no longer accurate. I'll update that when I'm ready, but in the mean time watch the discussion in the codereview if you're interested.

[kzar]

So you'll get both, but ​the pageId is now given for requests so it should simply be a matter of checking if that's -1 or not.

Note: This is no longer true, Sebastian's made some changes recently (#6543) to figure out the associated tabs for these requests.

[kzar]

Replying to kzar:

The API is going to change quite a bit so the issue description is no longer accurate...

OK I've updated it again now, should be a bit more accurate. Does it look OK to you Thomas?

[greiner]

Replying to kzar:

OK I've updated it again now, should be a bit more accurate. Does it look OK to you Thomas?

Yep. I was only worried about background tabs previously since those could've been relevant for an issue report but given that we can now associate each of those with the given tab and send all relevant requests to the issue reporter, my concern is no longer valid.

[abpbot]

A commit referencing this issue has landed:
​Issue 6402 - Split filter hit / request logging out into own API

[Ross]

Done. DevTools and Issue Reporter still work as expected.

ABP 3.1.0.2069
Chrome 67 / 64 / 49 / Windows 7
Firefox 60 / 55 / 51 / Windows 7
Opera 52 / 45 / 38 / Windows 7

ABP 0.9.11.2052
Edge 42.17134.1.0 / Windows 10