Issue 6843 - Log snippet filter hits

lib/contentFiltering.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
@@ skipping 159 matching lines @@
   let code = executableCode.get(script);
   if (code)
     return code;
 
   code = compileScript(script, [snippetsLibrarySource]);
 
   executableCode.set(script, code);
   return code;
 }
 
-function executeScript(script, tabId, frameId, request)
+function executeScript(script, tabId, frameId)
 {
   try
   {
     let details = {
       code: getExecutableCode(script),
       matchAboutBlank: true,
       runAt: "document_start"
     };
 
     // Chrome <50 throws an exception if chrome.tabs.executeScript is called
     // with a frameId of 0.
     if (frameId != 0)
       details.frameId = frameId;
 
-    browser.tabs.executeScript(tabId, details)
-    .then(() =>

[Manish Jethani, 2018/08/27 05:47:36]

Ideally this would take the form `.then(() => {}, error => {})` rather than
`.then(() => {}).catch(error => {})` since we're only specifically interested in
the errors coming from the call itself rather than any subsequent handlers.

[hub, 2018/08/27 12:56:47]

this is obsolete now.

-    {
-      let {filter} = request;

[Manish Jethani, 2018/08/27 05:47:36]

We could read out `request.url` and `request.docDomain` right here as well.

[hub, 2018/08/27 12:56:47]

this is obsolete now.

-
-      let tabIds = [tabId];
-      if (filter)
-        FilterNotifier.emit("filter.hitCount", filter, 0, 0, tabIds);
-
-      logRequest(tabIds, {
-        url: request.url, type: "SNIPPET", docDomain: request.docDomain
-      }, filter);
-    })
+    return browser.tabs.executeScript(tabId, details)
     .catch(error =>
     {
       // Sometimes a frame is added and removed very quickly, in such cases we
       // simply ignore the error.
       if (error.message == "The frame was removed.")
         return;
 
       // Sometimes the frame in question is just not found. We don't know why
       // this is exactly, but we simply ignore the error.
       if (/^No frame with id \d+ in tab \d+\.$/.test(error.message))
         return;
 
       throw error;
     });
   }
   catch (error)
   {
     // See the comment in the catch block associated with the call to
-    // tabs.insertCSS for why we catch and ignore any errors here.
+    // tabs.insertCSS for why we catch any error here and simply
+    // return a rejected promise.
+    return Promise.reject(error);
   }

[Manish Jethani, 2018/08/27 05:47:36]

I think it would be cleaner to keep executeScript limited to its original
purpose, which is to call tabs.executeScript with some boilerplate. In that
case, we can return the promise returned by tabs.executeScript and let the
caller add a `then` handler to log the hit (perhaps in a separate
`logSnippetFilterHit` function). The catch block here would also then have to
return a rejected promise (`return Promise.reject(error)`).

[hub, 2018/08/27 12:56:47]

Done.

 }
 
 port.on("content.applyFilters", (message, sender) =>
 {
   let selectors = [];
   let emulatedPatterns = [];
   let trace = HitLogger.hasListener(sender.page.id);
   let inline = !userStyleSheetsSupported;
 
   let {elemhide, snippets} = message.filterTypes ||
                              {elemhide: true, snippets: true};
 
   if (!checkWhitelisted(sender.page, sender.frame, null,
                         RegExpFilter.typeMap.DOCUMENT))
   {
-    let hostname = extractHostFromFrame(sender.frame);
+    let docDomain = extractHostFromFrame(sender.frame);
 
     if (snippets)
     {
-      for (let filter of Snippets.getScriptsForDomain(hostname))
-        executeScript(filter.script, sender.page.id, sender.frame.id,
-          {
-            docDomain: hostname,

[Sebastian Noack, 2018/08/27 04:51:35]

Nit: If you rename the variable "hostname" to "docDomain", you can use the
object creation shorthand here, and consequently this code also wraps nicer:

  executeScript(filter.script, sender.page.id, sender.frame.id,
                {docDomain, url: sender.frame.url.href, filter});

[hub, 2018/08/27 12:56:47]

Done.

+      for (let filter of Snippets.getFiltersForDomain(docDomain))
+      {
+        executeScript(filter.script, sender.page.id, sender.frame.id).then(() =>
+        {
+          let tabIds = [sender.page.id];
+          if (filter)
+            FilterNotifier.emit("filter.hitCount", filter, 0, 0, tabIds);
+
+          logRequest(tabIds, {
             url: sender.frame.url.href,
-            filter
-          });
+            type: "SNIPPET",
+            docDomain
+          }, filter);
+        });
+      }
     }
 
     if (elemhide && !checkWhitelisted(sender.page, sender.frame, null,
                                       RegExpFilter.typeMap.ELEMHIDE))
     {
       let specificOnly = checkWhitelisted(sender.page, sender.frame, null,
                                           RegExpFilter.typeMap.GENERICHIDE);
-      selectors = ElemHide.getSelectorsForDomain(hostname, specificOnly);
-
-      for (let filter of ElemHideEmulation.getRulesForDomain(hostname))
+      selectors = ElemHide.getSelectorsForDomain(docDomain, specificOnly);
+
+      for (let filter of ElemHideEmulation.getRulesForDomain(docDomain))
         emulatedPatterns.push({selector: filter.selector, text: filter.text});
     }
   }
 
   if (!inline && !updateFrameStyles(sender.page.id, sender.frame.id,
                                     selectors, "standard"))
   {
     inline = true;
   }
 
@@ skipping 16 matching lines @@
   updateFrameStyles(sender.page.id, sender.frame.id, message.selectors,
                     message.groupName, message.appendOnly);
 });
 
 fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"})
 .then(response => response.ok ? response.text() : "")
 .then(text =>
 {
   snippetsLibrarySource = text;
 });