Issue 6091 - Add checks for overly generic antiadblock filters

lib/antiadblockInit.js

Index: lib/antiadblockInit.js
===================================================================
--- a/lib/antiadblockInit.js
+++ b/lib/antiadblockInit.js
@@ -10,16 +10,18 @@
  * 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/>.
  */
 
+/* global publicSuffixes  */

[kzar, 2018/05/22 13:24:02]

Nit: Seems like we generally write "globals" instead of "global" in our code so
far.

[Jon Sonesen, 2018/05/22 21:25:18]

Done.

+
 "use strict";
 
 const {Prefs} = require("prefs");
 const {ActiveFilter} = require("filterClasses");
 const {FilterStorage} = require("filterStorage");
 const {FilterNotifier} = require("filterNotifier");
 const {Subscription} = require("subscriptionClasses");
 const {Notification} = require("notification");
@@ -47,17 +49,23 @@
     for (let filter of subscription.filters)
     {
       if (filter instanceof ActiveFilter && filter.domains)
       {
         for (let [domain, included] of filter.domains)
         {
           let urlFilter = "||" + domain + "^$document";
           if (domain && included && urlFilters.indexOf(urlFilter) == -1)

[kzar, 2018/05/22 13:24:02]

Seems kinda inefficient how we're doing urlFilters.indexOf each time. Perhaps we
should make urlFilters a Set, but then convert it to an Array when assigning it
to notification.urlFilters?

[Jon Sonesen, 2018/05/22 18:26:11]

I agree here, and was thinking about this. Was not sure if it counted as an
unrelated change though. @Thomas whats your opinion here?

[Thomas Greiner, 2018/05/22 18:43:29]

I don't know whether it'd be faster and, if so, by how much but it should be
easy to find out using jsPerf or other means.

The results may also depend on the amount of domains we have to process so note
that the antiadblockfilters.txt list currently contains less than 400 filters
with less than 300 domains.

-            urlFilters.push(urlFilter);
+          {
+            if (["COM", "INFO", "ORG", "NET"].indexOf(domain) == -1)

[Jon Sonesen, 2018/05/18 20:16:18]

Is hardly an exhaustive list of TLDs but there are also quite a few so not sure
how many we should cover.

[Thomas Greiner, 2018/05/22 12:43:10]

All public suffixes (including TLDs) are listed in
https://publicsuffix.org/list/public_suffix_list.dat and
https://publicsuffix.org/list/effective_tld_names.dat but, for some reason, it
seems like "com" and others are not included in the lib/publicSuffixList.js file
we generate when building the extension (see
https://hg.adblockplus.org/buildtools/file/168dac24ad9e/publicSuffixListUpdat...).

Instead, we're filtering out lines which don't contain a dot and consider any
domain that doesn't contain a dot as a TLD (see
https://hg.adblockplus.org/adblockpluschrome/file/939bf6cdd435/lib/tldjs.js#l30).
Be careful, however, when checking for that because of FQDNs which end with a
dot but it looks like we're not checking for those there either.

[kzar, 2018/05/22 13:24:02]

Yes, I expected we would first check that the domain had at least one dot before
checking it wasn't present in the public suffix list. We definitely don't these
hard coded domains here.

I didn't consider the case of a trailing dot however. I did a test to see if
they can even show up and found they can. So Thomas is right, be careful to
handle those. (I found we don't handle trailing dots consistently, we strip them
for blocking filters but not element hiding filters, so I've opened #6690 to
discuss that.)

[Jon Sonesen, 2018/05/22 18:26:11]

Okay, thanks for that. Was not sure how we handled generic filters in other
places and this was just a "works for now" implementation as I wasn't 100% sure
how to proceed. Thank you guys for clearing it up.

+            {
+              if (!(publicSuffixes.hasOwnProperty(domain)))

[Thomas Greiner, 2018/05/22 12:43:10]

Ideally, this logic would be part of `Filter.prototype.isGeneric()` because I'd
consider filters which apply to a large subset of domains (e.g. all "com"
domains) also as generic.

@Dave What's your take on that?

[kzar, 2018/05/22 13:24:02]

Well you're right that a filter for the domain "com" is a very generic filter
and therefore what you suggest makes sense. Unfortunately in the context of the
core code a filter being generic or specific means something else, which means
we can't do it in practice (without overhauling getSelectorsForDomain and other
core code).

[Thomas Greiner, 2018/05/22 17:48:59]

That's quite a pity. Nevermind then.

[Jon Sonesen, 2018/05/22 18:26:11]

Acknowledged.

+                urlFilters.push(urlFilter);
+            }
+          }
         }
       }
     }
     notification.urlFilters = urlFilters;
     Notification.addNotification(notification);
     Notification.addQuestionListener(notification.id, notificationListener);
   }