Issue 6843 - Log snippet filter hits

lib/contentFiltering.js

Index: lib/contentFiltering.js
===================================================================
--- a/lib/contentFiltering.js
+++ b/lib/contentFiltering.js
@@ -17,21 +17,22 @@
 
 /** @module contentFiltering */
 
 "use strict";
 
 const {RegExpFilter} = require("../adblockpluscore/lib/filterClasses");
 const {ElemHide} = require("../adblockpluscore/lib/elemHide");
 const {ElemHideEmulation} = require("../adblockpluscore/lib/elemHideEmulation");
+const {FilterNotifier} = require("../adblockpluscore/lib/filterNotifier");
 const {Snippets, compileScript} = require("../adblockpluscore/lib/snippets");
 const {checkWhitelisted} = require("./whitelisting");
 const {extractHostFromFrame} = require("./url");
 const {port} = require("./messaging");
-const {HitLogger} = require("./hitLogger");
+const {HitLogger, logRequest} = require("./hitLogger");
 const info = require("info");
 
 // Chromium's support for tabs.removeCSS is still a work in progress and the
 // API is likely to be different from Firefox's; for now we just don't use it
 // at all, even if it's available.
 // See https://crbug.com/608854
 const styleSheetRemovalSupported = info.platform == "gecko";
 
@@ -186,17 +187,17 @@
       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)
+    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
@@ -205,49 +206,65 @@
         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);

[Sebastian Noack, 2018/08/27 17:37:34]

It appears I'm missing something here. But what is the difference
between "p" and "p.catch(e => Promise.reject(e))"?

[hub, 2018/08/27 19:45:24]

Promise.catch(f) with call Promise.then(undefined, f) under the hood (this is
the MDN documentation)

However, here, we are in a plain exception catch(), so we don't have a promise.

[Manish Jethani, 2018/08/27 20:07:54]

If the call to `tabs.executeScript` throws, the function won't return a promise,
it'll return undefined; we need to return a "thenable" here (and it seems
appropriate to return a rejected promise here as it makes it consistent between
Firefox and Chrome, see comment in `addStyleSheet` for the background).

[Sebastian Noack, 2018/08/27 20:15:11]

Acknowledged. I misread the code and thought we are in catch() call of a
promise, not catch-block here.

   }
 }
 
 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 script of Snippets.getScriptsForDomain(hostname))
-        executeScript(script, sender.page.id, sender.frame.id);
+      for (let filter of Snippets.getFiltersForDomain(docDomain))
+      {
+        executeScript(filter.script, sender.page.id, sender.frame.id)
+        .then(() =>

[Sebastian Noack, 2018/08/27 17:37:34]

Nit: Wrapping here appears unnecessary.

[hub, 2018/08/27 19:45:24]

without wrapping the `>` reach column 80 so I felt it was better this way.

done

[Sebastian Noack, 2018/08/27 20:15:11]

Yes, the line may have up to 80 characters, not more, exactly 80 is fine.

+        {
+          let tabIds = [sender.page.id];
+          if (filter)
+            FilterNotifier.emit("filter.hitCount", filter, 0, 0, tabIds);
+
+          logRequest(tabIds, {
+            url: sender.frame.url.href,
+            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);
+      selectors = ElemHide.getSelectorsForDomain(docDomain, specificOnly);
 
-      for (let filter of ElemHideEmulation.getRulesForDomain(hostname))
+      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;