Issue 1162 - Cache notification URL matcher

lib/notification.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
@@ skipping 120 matching lines @@
   // 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 = "";
   }
@@ skipping 28 matching lines @@
  * Regularly fetches notifications and decides which to show.
  * @class
  */
 let Notification = exports.Notification =
 {
   /**
    * Called on module startup.
    */
   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);
@@ skipping 102 matching lines @@
       showListeners.splice(index, 1);
   },
 
   /**
    * Determines which notification is to be shown next.
    * @param {string} url URL to match notifications to (optional)
    * @return {Object} notification to be shown, or null if there is none
    */
   _getNextToShow(url)
   {
-    let remoteData = Prefs.notificationdata.data.notifications;
+    let remoteData = [];
+    if (typeof Prefs.notificationdata.data == "object" &&
+        Prefs.notificationdata.data.notifications instanceof Array)
+    {
+      remoteData = Prefs.notificationdata.data.notifications;
+    }
     let notifications = localData.concat(remoteData);
     if (notifications.length === 0)
       return null;
 
     const {addonName, addonVersion, application,
            applicationVersion, platform, platformVersion} = require("info");
 
     let targetChecks = {
       extension: v => v == addonName,
       extensionMinVersion:
@@ skipping 233 matching lines @@
     else if (index != -1 && forceValue !== true)
       categories.splice(index, 1);
 
     // HACK: JSON values aren't saved unless they are assigned a
     // different object.
     Prefs.notifications_ignoredcategories =
       JSON.parse(JSON.stringify(categories));
   }
 };
 Notification.init();