Issue 1162 - Cache notification URL matcher

lib/notification.js

Index: lib/notification.js
===================================================================
--- a/lib/notification.js
+++ b/lib/notification.js
@@ -38,6 +38,7 @@
   relentless: 2,
   critical: 3
 };
+const MATCHER = Symbol("Notification matcher");
 
 let showListeners = [];
 let questionListeners = {};
@@ -103,10 +104,76 @@
 }
 
 /**
+ * Initializes notification's matcher based on notification's URL filters
+ * @param {Object} notification
+ */
+function initNotificationMatcher(notification)
+{
+  if (MATCHER in notification || !(notification.urlFilters instanceof Array))
+    return;
+
+  let matcher = new Matcher();
+  for (let urlFilter of notification.urlFilters)
+  {
+    matcher.add(Filter.fromText(urlFilter));
+  }
+  notification[MATCHER] = matcher;
+}
+
+/**
+ * Matches URL against notification's URL filters
+ * @param {Object} notification
+ * @param {string} [url]
+ * @return {boolean} whether notification and URL match
+ */
+function matchesUrl(notification, url)
+{
+  // No matching necessary if there's nothing to match
+  if (typeof url !== "string" && !(MATCHER in notification))
+    return true;
+
+  // Notification shouldn't match if extension is disabled
+  if (!Prefs.enabled)
+    return false;
+
+  // Notification shouldn't match if matching cannot be done
+  if (typeof url !== "string" || !(MATCHER in notification))
+    return false;

[sergei, 2018/01/30 17:25:05]

It somehow conflicts with above
if (typeof url !== "string" && !(MATCHER in notification))
  return true;

Should we actually return `true` when the type of `url` parameter is actually
unexpected?

[Thomas Greiner, 2018/01/30 19:17:53]

The logic behind it is that if the notification doesn't target URLs then we
should just continue with checking whether it should be shown or not.
However, if one of them is missing it means that
- either the notification doesn't target URLs but we're only checking for
notifications which target URLs
- or that the notification targets URLs but that there's no URL to match against

In both situations there's a mismatch which means that the notification
shouldn't be shown.

---

If you think it'd be helpful, I can split this condition up into two separate
ones to add an individual comment to each of them.

Alternatively, I could introduce variables to clarify what each part stands for
and thereby also reduce duplication.

e.g.
let shouldCheck = (typeof url !== "string");
let targetsUrls = (MATCHER in notification);

if (!shouldCheck && !targetsUrls)
  return true;

if (!shouldCheck || !targetsUrls)
  return false;

[sergei, 2018/01/30 21:06:48]

Acknowledged.

+
+  let host;
+  try
+  {
+    host = new URL(url).hostname;
+  }
+  catch (e)
+  {
+    host = "";
+  }
+
+  // Notification shouldn't match if extension is disabled on provided domain
+  let exception = defaultMatcher.matchesAny(
+    url, RegExpFilter.typeMap.DOCUMENT, host, false, null
+  );
+  if (exception instanceof WhitelistFilter)
+    return false;
+
+  // Notification should match if one of its filters matches
+  let filter = notification[MATCHER].matchesAny(
+    url, RegExpFilter.typeMap.DOCUMENT, host, false, null
+  );
+  return !!filter;
+}
+
+/**
  * The object providing actual downloading functionality.
  * @type {Downloader}
  */
 let downloader = null;
+
+/**
+ * List of notifications provided by the extension
+ * @type {Object[]}
+ */
 let localData = [];
 
 /**
@@ -120,6 +187,15 @@
    */
   init()
   {
+    let {data} = Prefs.notificationdata;

[sergei, 2018/01/30 17:25:05]

Prefs' properties can be not ready yet, I would recommend to move it into a
function and check whether Prefs are already loaded then call it, otherwise add
the function as a listener for load event, Prefs in parent projects should be
adjusted too.

[sergei, 2018/01/30 17:28:24]

Perhaps alternatively we could do it in `matchesUrl`.

[Thomas Greiner, 2018/01/30 19:17:53]

In theory that's correct but in practice this is not an issue since the
preference is initialized as `{}` (at least in adblockpluschrome). Anyway, I do
agree that it wouldn't be a bad idea to catch it just in case, because it's only
an implicit dependency that we have here which makes it quite unreliable.

Besides that, the same issue also applies to
`Prefs.notifications_ignoredcategories` as well as pretty much every other use
of `Prefs.notificationdata` so we may need to rethink our approach on a more
fundamental level.
One option I could think of would be to introduce a lib/prefs.js module in
adblockpluscore so that it can define and initialize its own preferences.

Would you prefer if I add a workaround for this particular case or should I
create a ticket so that we can tackle the underlying issue separately?

[sergei, 2018/01/30 21:06:48]

It does happen on practice (at least I observed the same issue in
libadblockplus). Let's consider how it works:
there is some moral equivalent of "main.js" which just loads all our js files,
including notification.js and let's say prefs.js is loaded by it too (it's
equivalent when prefs.js is first time loaded by notification.js and
notification.js is loaded as a result of series of calls of `require` BTW), so

CURRENTLY RUNNING JS -- BEGIN --
require("...")
require("prefs")
  init settings to default values and
  load previously stored settings asynchronously
    at some point later
      schedule the call of "done" callback after finishing currently running JS
code
require("...")
require("notification")
  Notification.init()
    here we have only default values for Prefs
require("...")
CURRENTLY RUNNING JS  -- END --
....
CURRENTLY RUNNING JS -- BEGIN --
"done" callback
  set Prefs
now one may use Prefs.
CURRENTLY RUNNING JS  -- END --

BTW, in some test of libadblockplus there were a couple of such cases and for
the sake of simplicity and in order to not bother adblockpluscore we just called
"done" callback for preferences immediately (synchronously), so Prefs are ready
right after `require("prefs")` :)
You are right, currently no one observed the problem here because the
probability of the race condition with Prefs.notifications_ignoredcategories in
particular is too low because it accessed first time after a huge delay, but for
that new code, Prefs will not be populated by asynchronously loaded values yet.

In general of course it would be good to address it at another level, e.g.
either even load the rest of the modules or at least call their `init` functions
after Prefs are loaded. However, it can cause undesired delays, so I think such
complexity (fiddling with the "load" event) looks good for me. Actually, for the
sake of simplicity we can just add a method to Prefs which contains the logic
and either calls a callback immediately or adds it as a listener for some "load"
event.
Maybe it's a good idea to make the decision even about the approach to deal with
such case in an issue, feel free to file as many as you think there should be :)

[kzar, 2018/01/31 11:06:27]

How about using the Prefs.untilLoaded Promise?

[sergei, 2018/02/05 12:49:34]

It seems the best option.

+    if (data)
+    {
+      for (let notification of data.notifications)
+      {
+        initNotificationMatcher(notification);
+      }
+    }
+
     downloader = new Downloader(this._getDownloadables.bind(this),
                                 INITIAL_DELAY, CHECK_INTERVAL);
     downloader.onExpirationChange = this._onExpirationChange.bind(this);
@@ -174,6 +250,7 @@
             notification.type = notification.severity;
           delete notification.severity;
         }
+        initNotificationMatcher(notification);
       }
       Prefs.notificationdata.data = data;
     }
@@ -239,7 +316,6 @@
     {
       remoteData = Prefs.notificationdata.data.notifications;
     }
-
     let notifications = localData.concat(remoteData);
     if (notifications.length === 0)
       return null;
@@ -298,39 +374,8 @@
         }
       }
 
-      if (typeof url === "string" || notification.urlFilters instanceof Array)
-      {
-        if (Prefs.enabled && typeof url === "string" &&
-            notification.urlFilters instanceof Array)
-        {
-          let host;
-          try
-          {
-            host = new URL(url).hostname;
-          }
-          catch (e)
-          {
-            host = "";
-          }
-
-          let exception = defaultMatcher.matchesAny(
-            url, RegExpFilter.typeMap.DOCUMENT, host, false, null
-          );
-          if (exception instanceof WhitelistFilter)
-            continue;
-
-          let matcher = new Matcher();
-          for (let urlFilter of notification.urlFilters)
-            matcher.add(Filter.fromText(urlFilter));
-          if (!matcher.matchesAny(url, RegExpFilter.typeMap.DOCUMENT, host,
-              false, null))
-          {
-            continue;
-          }
-        }
-        else
-          continue;
-      }
+      if (!matchesUrl(notification, url))
+        continue;
 
       if (notification.targets instanceof Array)
       {
@@ -433,7 +478,10 @@
   addNotification(notification)
   {
     if (localData.indexOf(notification) == -1)
+    {
+      initNotificationMatcher(notification);
       localData.push(notification);
+    }
   },
 
   /**