Issue 7003 - Look up whitelist filter only if URL is blocked

lib/matcher.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/>.
  */
 
 "use strict";
 
 /**
  * @fileOverview Matcher class implementing matching addresses against
  *               a list of filters.
  */
 
-const {WhitelistFilter} = require("./filterClasses");
+const {RegExpFilter, WhitelistFilter} = require("./filterClasses");
 
 /**
  * Regular expression for matching a keyword in a filter.
  * @type {RegExp}
  */
 const keywordRegExp = /[^a-z0-9%*][a-z0-9%]{3,}(?=[^a-z0-9%*])/;
 
 /**
  * Regular expression for matching all keywords in a filter.
  * @type {RegExp}
  */
 const allKeywordsRegExp = new RegExp(keywordRegExp, "g");
 
 /**
+ * Bitmask for "types" that are for exception rules only, like
+ * <code>$document</code>, <code>$elemhide</code>, and so on.
+ * @type {number}
+ */
+const WHITELIST_ONLY_TYPES = RegExpFilter.typeMap.DOCUMENT |
+                             RegExpFilter.typeMap.ELEMHIDE |
+                             RegExpFilter.typeMap.GENERICHIDE |
+                             RegExpFilter.typeMap.GENERICBLOCK;
+
+/**
  * Checks whether a particular filter is slow.
  * @param {RegExpFilter} filter
  * @returns {boolean}
  */
 function isSlowFilter(filter)
 {
   return !filter.pattern || !keywordRegExp.test(filter.pattern);
 }
 
 exports.isSlowFilter = isSlowFilter;
@@ skipping 263 matching lines @@
    * @inheritdoc
    */
   matchesAnyInternal(location, typeMask, docDomain, thirdParty, sitekey,
                      specificOnly)
   {
     let candidates = location.toLowerCase().match(/[a-z0-9%]{3,}/g);
     if (candidates === null)
       candidates = [];
     candidates.push("");
 
+    let whitelistHit = null;
     let blacklistHit = null;
-    for (let i = 0, l = candidates.length; i < l; i++)
+
+    if ((typeMask & ~WHITELIST_ONLY_TYPES) != 0)

[Sebastian Noack, 2018/10/21 22:41:39]

I wonder if this logic is necessary. Shouldn't calling code that is specifically
interested in whitelisting filters rather use matcher.whitelist.matchesAny()?

[Manish Jethani, 2018/10/22 11:05:16]

I like the idea, but we would lose out on the caching of the result (which
should be valuable for these types of lookups).

I see that lib/whitelisting.js and lib/filterComposer.js already call
defaultMatcher.whitelist.matchesAny directly. We could just introduce new
individual caches in both blacklist and whitelist.

I think though that I am leaning towards the current patch, which optimizes for
whitelist-only types as well. If we go with this, we can change the code in
adblockpluschrome to call defaultMatcher.matchesAny in all cases.

What do you think?

[Sebastian Noack, 2018/10/22 15:04:31]

No strong opinion either way, but in case we go with your approach, we should
probably hide defaultMatcher.whitelist and defaultMatcher.blacklist from the
public API.

Also mind putting the result of `(typeMask & ~WHITELIST_ONLY_TYPES) != 0` into a
variable rather than repeating yourself?

[Manish Jethani, 2018/10/22 20:39:30]

We will be doing this as part of this:

https://codereview.adblockplus.org/29897555/
It's not repetition because the two checks are different.

[Manish Jethani, 2018/10/22 21:16:19]

I've added comments to make it clearer.

[Sebastian Noack, 2018/10/22 22:12:35]

I don't see any changes in the current patch there that will make
defaultMatcher.blacklist and defaultMatcher.whitelist private.
Acknowledged.

[Manish Jethani, 2018/10/22 22:57:56]

Yes, I'll suggest that change there if we agree on this one here. Regardless,
this change does not depend on that one (i.e. whitelist could still be public
and this change would still make sense).

     {
-      let substr = candidates[i];
-      let result = this.whitelist._checkEntryMatch(
-        substr, location, typeMask, docDomain, thirdParty, sitekey
-      );
-      if (result)
-        return result;
-      if (blacklistHit === null)
+      for (let i = 0, l = candidates.length; !blacklistHit && i < l; i++)
       {
-        blacklistHit = this.blacklist._checkEntryMatch(
-          substr, location, typeMask, docDomain, thirdParty, sitekey,
-          specificOnly
-        );
+        blacklistHit = this.blacklist._checkEntryMatch(candidates[i], location,
+                                                       typeMask, docDomain,
+                                                       thirdParty, sitekey,
+                                                       specificOnly);
       }
     }
-    return blacklistHit;
+
+    if (blacklistHit || (typeMask & WHITELIST_ONLY_TYPES) != 0)
+    {
+      for (let i = 0, l = candidates.length; !whitelistHit && i < l; i++)
+      {
+        whitelistHit = this.whitelist._checkEntryMatch(candidates[i], location,
+                                                       typeMask, docDomain,
+                                                       thirdParty, sitekey);
+      }
+    }
+
+    return whitelistHit || blacklistHit;
   }
 
   /**
    * @see Matcher#matchesAny
    * @inheritdoc
    */
   matchesAny(location, typeMask, docDomain, thirdParty, sitekey, specificOnly)
   {
     let key = location + " " + typeMask + " " + docDomain + " " + thirdParty +
       " " + sitekey + " " + specificOnly;
@@ skipping 16 matching lines @@
 
 exports.CombinedMatcher = CombinedMatcher;
 
 /**
  * Shared {@link CombinedMatcher} instance that should usually be used.
  * @type {CombinedMatcher}
  */
 let defaultMatcher = new CombinedMatcher();
 
 exports.defaultMatcher = defaultMatcher;