Issue 2401 - Integrate CSS property rule handling in Firefox

lib/contentPolicy.js

Index: lib/contentPolicy.js
===================================================================
--- a/lib/contentPolicy.js
+++ b/lib/contentPolicy.js
@@ -140,60 +140,37 @@ var Policy = exports.Policy =
 
     // Ignore whitelisted schemes
     if (!this.isBlockableScheme(location))
       return response(true, false);
 
     // Interpret unknown types as "other"
     contentType = this.contentTypes.get(contentType) || "OTHER";
 
-    let wndLocation = frames[0].location;
-    let docDomain = getHostname(wndLocation);
-    let match = null;
-    let [sitekey, sitekeyFrame] = getSitekey(frames);
     let nogeneric = false;
-    if (!match && Prefs.enabled)
+    if (Prefs.enabled)
     {
-      let testSitekey = sitekey;
-      let testSitekeyFrame = sitekeyFrame;
-      for (let i = 0; i < frames.length; i++)
+      let whitelistHit =
+          this.isFrameWhitelisted(frames, contentType == "ELEMHIDE");

[Wladimir Palant, 2016/03/18 15:25:24]

Unfortunately, moving out the check for exception rules wasn't a very clean
change. What happened here:

* Looping over frames moved to isFrameWhitelisted(). Merely the surrounding `if
(Prefs.enabled)` check stayed here. A few variable declarations moved down,
these weren't actually used in this loop.
* Adding a filter hit is still being done here, so isFrameWhitelisted() returns
all the data necessary for that.
* If there was a hit, it could be a DOCUMENT/ELEMHIDE filter ("real"
whitelisting) or GENERICBLOCK/GENERICHIDE (merely disabled generic rules). It's
up to the caller to check matchType.
* Side-effect: this can produce only one hit now which is the correct behavior
IMHO. Original code was slightly buggy because it counted hits immediately when
it found a matching filter. However, in reality a $genericblock filter is
ignored if the frame is whitelisted further up in the frame hierarchy.
* This didn't add any redundancy beyond the getSitekey() call below, this is the
only operation which is duplicated.

[Thomas Greiner, 2016/03/30 18:02:05]

All in all sounds reasonable.

+      if (whitelistHit)
       {
-        let frame = frames[i];
-        let testWndLocation = frame.location;
-        let parentWndLocation = frames[Math.min(i + 1, frames.length - 1)].location;
-        let parentDocDomain = getHostname(parentWndLocation);
-
-        let typeMap = RegExpFilter.typeMap.DOCUMENT;
-        if (contentType == "ELEMHIDE")
-          typeMap = typeMap | RegExpFilter.typeMap.ELEMHIDE;
-        let whitelistMatch = defaultMatcher.matchesAny(testWndLocation, typeMap, parentDocDomain, false, testSitekey);
-        if (whitelistMatch instanceof WhitelistFilter)
-        {
-          let whitelistType = (whitelistMatch.contentType & RegExpFilter.typeMap.DOCUMENT) ? "DOCUMENT" : "ELEMHIDE";
-          addHit(i, whitelistType, parentDocDomain, false, testWndLocation,
-              whitelistMatch);
+        let [frameIndex, matchType, docDomain, thirdParty, location, filter] = whitelistHit;
+        addHit(frameIndex, matchType, docDomain, thirdParty, location, filter);
+        if (matchType == "DOCUMENT" || matchType == "ELEMHIDE")
           return response(true, false);
-        }
-
-        let genericType = (contentType == "ELEMHIDE" ? "GENERICHIDE" : "GENERICBLOCK");
-        let nogenericMatch = defaultMatcher.matchesAny(testWndLocation,
-            RegExpFilter.typeMap[genericType], parentDocDomain, false, testSitekey);
-        if (nogenericMatch instanceof WhitelistFilter)
-        {
+        else

[Thomas Greiner, 2016/03/30 18:02:05]

Detail: This `else` is unnecessary due to the preceding `return`.

[Wladimir Palant, 2016/03/30 18:42:17]

Sure, yet I'd prefer to keep it. Compilers don't need it, for humans however
it's more obvious that setting nogeneric is the action for match types other
than document and elemhide.

           nogeneric = true;
-          addHit(i, genericType, parentDocDomain, false, testWndLocation,
-              nogenericMatch);
-        }
-
-        if (frame == testSitekeyFrame)
-          [testSitekey, testSitekeyFrame] = getSitekey(frames.slice(i + 1));
       }
     }
 
-    if (!match && contentType == "ELEMHIDE")
+    let match = null;
+    let wndLocation = frames[0].location;
+    let docDomain = getHostname(wndLocation);
+    let [sitekey, sitekeyFrame] = getSitekey(frames);
+    if (contentType == "ELEMHIDE")
     {
       match = ElemHide.getFilterByKey(location);
       location = match.text.replace(/^.*?#/, '#');
 
       if (!match.isActiveOnDomain(docDomain))
         return response(true, false);
 
       let exception = ElemHide.getException(match, docDomain);
@@ -235,44 +212,93 @@ var Policy = exports.Policy =
       scheme = match ? match[1] : null;
     }
     else
       scheme = location.scheme;
     return !this.whitelistSchemes.has(scheme);
   },
 
   /**
-   * Checks whether a page is whitelisted.
+   * Checks whether a top-level window is whitelisted.
    * @param {String} url
-   * @param {String} [parentUrl] location of the parent page
-   * @param {String} [sitekey] public key provided on the page
-   * @return {Filter} filter that matched the URL or null if not whitelisted
+   *    URL of the document loaded into the window
+   * @return {?WhitelistFilter}
+   *    exception rule that matched the URL if any
    */
-  isWhitelisted: function(url, parentUrl, sitekey)
+  isWhitelisted: function(url)

[Wladimir Palant, 2016/03/18 15:25:24]

This method was never called with more than one parameter - nor would it produce
correct results if called that way.

   {
     if (!url)
       return null;
 
     // Do not apply exception rules to schemes on our whitelistschemes list.
     if (!this.isBlockableScheme(url))
       return null;
 
-    if (!parentUrl)
-      parentUrl = url;
-
     // Ignore fragment identifier
     let index = url.indexOf("#");
     if (index >= 0)
       url = url.substring(0, index);
 
-    let result = defaultMatcher.matchesAny(url, RegExpFilter.typeMap.DOCUMENT, getHostname(parentUrl), false, sitekey);
+    let result = defaultMatcher.matchesAny(url, RegExpFilter.typeMap.DOCUMENT,
+        getHostname(url), false, null);
     return (result instanceof WhitelistFilter ? result : null);
   },
 
   /**
+   * Checks whether a frame is whitelisted.
+   * @param {Array} frames
+   *    frame structure as returned by getFrames() in child/utils module.
+   * @param {boolean} isElemHide
+   *    true if element hiding whitelisting should be considered
+   * @return {?Array}
+   *    An array with the hit parameters: frameIndex, contentType, docDomain,
+   *    thirdParty, location, filter. Note that the filter could be a
+   *    genericblock/generichide exception rule. If nothing matched null is
+   *    returned.
+   */
+  isFrameWhitelisted: function(frames, isElemHide)
+  {
+    let [sitekey, sitekeyFrame] = getSitekey(frames);
+    let nogenericHit = null;
+
+    let typeMap = RegExpFilter.typeMap.DOCUMENT;
+    if (isElemHide)
+      typeMap = typeMap | RegExpFilter.typeMap.ELEMHIDE;
+    let genericType = (isElemHide ? "GENERICHIDE" : "GENERICBLOCK");
+
+    for (let i = 0; i < frames.length; i++)
+    {
+      let frame = frames[i];
+      let wndLocation = frame.location;
+      let parentWndLocation = frames[Math.min(i + 1, frames.length - 1)].location;
+      let parentDocDomain = getHostname(parentWndLocation);
+
+      let match = defaultMatcher.matchesAny(wndLocation, typeMap, parentDocDomain, false, sitekey);
+      if (match instanceof WhitelistFilter)
+      {
+        let whitelistType = (whitelistMatch.contentType & RegExpFilter.typeMap.DOCUMENT) ? "DOCUMENT" : "ELEMHIDE";
+        return [i, whitelistType, parentDocDomain, false, wndLocation, match];
+      }
+
+      if (!nogenericHit)
+      {
+        match = defaultMatcher.matchesAny(wndLocation,
+            RegExpFilter.typeMap[genericType], parentDocDomain, false, sitekey);
+        if (match instanceof WhitelistFilter)
+          nogenericHit = [i, genericType, parentDocDomain, false, wndLocation, match];
+      }
+
+      if (frame == sitekeyFrame)
+        [sitekey, sitekeyFrame] = getSitekey(frames.slice(i + 1));
+    }
+
+    return nogenericHit;
+  },
+
+  /**
    * Deletes nodes that were previously stored with a
    * RequestNotifier.storeNodesForEntries() call or similar.
    * @param {string} id  unique ID of the nodes
    */
   deleteNodes: function(id)
   {
     port.emit("deleteNodes", id);
   },