Issue 3839 - Adapt for Prefs event changes

messageResponder.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 34 matching lines @@
     var result = {};
     for (var i = 0; i < keys.length; i++)
       result[keys[i]] = obj[keys[i]];
     return result;
   }
 
   var convertSubscription = convertObject.bind(null, ["disabled",
     "downloadStatus", "homepage", "lastDownload", "title", "url"]);
   var convertFilter = convertObject.bind(null, ["text"]);
 
-  var changeListeners = null;
+  var changeListeners = new global.ext.PageMap();
+  var filterListenerAdded = false;
+  var listenedPreferences = [];
   var messageTypes = {
     "app": "app.listen",
     "filter": "filters.listen",
     "pref": "prefs.listen",
     "subscription": "subscriptions.listen"
   };
 
-  function sendMessage(type, action, args, page)
+  function sendMessage(type, action, args)
   {
-    var pages = page ? [page] : changeListeners.keys();
+    var pages = changeListeners.keys();
     for (var i = 0; i < pages.length; i++)
     {
       var filters = changeListeners.get(pages[i]);
-      if (filters[type] && filters[type].indexOf(action) >= 0)
+      var actions = filters[type];
+      if (actions && actions.indexOf(action) != -1)
       {
         pages[i].sendMessage({
           type:  messageTypes[type],
           action: action,
           args: args
         });
       }
     }
   }
 
@@ skipping 23 matching lines @@
       if (arg instanceof Subscription)
         return convertSubscription(arg);
       else if (arg instanceof Filter)
         return convertFilter(arg);
       else
         return arg;
     });
     sendMessage(type, action, args);
   }
 
-  function onPrefChange(name)
+  function getListenerFilters(page, addListener)
   {
-    sendMessage("pref", name, [Prefs[name]]);
+    if (addListener && !filterListenerAdded)
+    {
+      FilterNotifier.addListener(onFilterChange);
+      filterListenerAdded = true;
+    }

[Thomas Greiner, 2016/03/21 17:55:55]

I guess we could just unconditionally start listening when the background page
starts up and stop it when the extension shuts down.

That way we'd get rid of some unnecessary overhead around determining when we
actually need it.

[Sebastian Noack, 2016/03/21 19:40:07]

I considered it, but I rather avoid running the listener on each filter
notification when the options page has never been opened, considering that most
users don't often use the options page, that we dispatch plenty filter
notifications, and that this particular listener does quite some stuff before it
might bail out.

+
+    var listenerFilters = changeListeners.get(page);
+    if (!listenerFilters)
+    {
+      listenerFilters = Object.create(null);
+      changeListeners.set(page, listenerFilters);
+    }
+
+    return listenerFilters;
   }
 
   global.ext.onMessage.addListener(function(message, sender, callback)
   {
-    var listenerFilters = null;
-    if (/\.listen$/.test(message.type))
-    {
-      if (!changeListeners)
-      {
-        changeListeners = new global.ext.PageMap();
-        FilterNotifier.addListener(onFilterChange);
-        Prefs.onChanged.addListener(onPrefChange);
-      }
-
-      listenerFilters = changeListeners.get(sender.page);
-      if (!listenerFilters)
-      {
-        listenerFilters = Object.create(null);
-        changeListeners.set(sender.page, listenerFilters);
-      }
-    }
-
     switch (message.type)
     {
       case "app.get":
         if (message.what == "issues")
         {
           var subscriptionInit;
           try
           {
             subscriptionInit = require("subscriptionInit");
           }
@@ skipping 32 matching lines @@
             devToolsPanel: info.platform == "chromium",
             safariContentBlocker: "safari" in global
                 && "extension" in global.safari
                 && "setContentBlocker" in global.safari.extension
           });
         }
         else
           callback(null);
         break;
       case "app.listen":
+        var listenerFilters = getListenerFilters(sender.page, true);
         if (message.filter)
           listenerFilters.app = message.filter;
         else
           delete listenerFilters.app;
         break;
       case "app.open":
         if (message.what == "options")
           ext.showOptions();
         break;
       case "filters.add":
@@ skipping 80 matching lines @@
             var filter = subscription.filters[j];
             if (/^@@\|\|([^\/:]+)\^\$document$/.test(filter.text))
               continue;
 
             if (!(filter.text in seenFilter))
               FilterStorage.removeFilter(filter);
           }
         }
         break;
       case "filters.listen":
+        var listenerFilters = getListenerFilters(sender.page, true);
         if (message.filter)
           listenerFilters.filter = message.filter;
         else
           delete listenerFilters.filter;
         break;
       case "filters.remove":
         var filter = Filter.fromText(message.text);
         var subscription = null;
         if (message.subscriptionUrl)
           subscription = Subscription.fromURL(message.subscriptionUrl);
 
         if (!subscription)
           FilterStorage.removeFilter(filter);
         else
           FilterStorage.removeFilter(filter, subscription, message.index);
         break;
       case "prefs.get":
         callback(Prefs[message.key]);
         break;
       case "prefs.listen":
+        var listenerFilters = getListenerFilters(sender.page);
         if (message.filter)
+        {
+          message.filter.forEach(function(preference)
+          {
+            if (listenedPreferences.indexOf(preference) == -1)

[Thomas Greiner, 2016/03/21 17:55:55]

So only the first UI that registers a listener for a preference will actually
get it?

[Sebastian Noack, 2016/03/21 19:40:07]

Yes, the first UI that listens for a particular preference, causes a listener
for that preference to be added. But note that the listener calls sendMessage()
which notifies all pages that are listening for that preference.

+            {
+              listenedPreferences.push(preference);
+              Prefs.on(preference, function()
+              {
+                sendMessage("pref", preference, [Prefs[preference]]);
+              });

[Thomas Greiner, 2016/03/21 17:55:55]

This listener is never removed.

As you pointed out `FilterNotifier.removeListener()` and
`Prefs.onChanged.removeListener()` were never called. So far that hasn't been an
issue since the messageResponder couldn't accumulate more than a single listener
instance and since Chrome (unlike Firefox) throws those away on extension
shutdown anyway.
This change, however, would turn it into a memory leak if we end up having one
listener per preference per UI.

Just FYI: Currently, stopping a listener is achieved by sending "prefs.listen"
without "filter" - probably better to have an explicit "prefs.unlisten" method
but that's how it was implemented so far.

[Sebastian Noack, 2016/03/21 19:40:07]

Removing the listeners isn't trivial, since you cannot send messages in on
unload. Also there are scenarios (e.g. when a tab runs out of memory, crashes
for a different reason, etc.) where onload isn't triggered at all. So we would
have to listen for the tab to be removed, replaced or navigating somewhere else.
The PageMap object accounts for all of these scenarios. However, the abstraction
layer doesn't expose some of these events directly. Neither, do I think that the
addional complexity would be justified. After all, leaking one listener per
preference isn't too bad, and IMO not any worse than before, considering that
the listener we leaked before was called for any preference. Now we have
multiple listeners but they are only called for specific preferences. hence add
less overhead.

+            }
+          });
           listenerFilters.pref = message.filter;
+        }
         else
           delete listenerFilters.pref;
         break;
       case "prefs.toggle":
         if (message.key == "notifications_ignoredcategories")
           NotificationStorage.toggleIgnoreCategory("*");
         else
           Prefs[message.key] = !Prefs[message.key];
         break;
       case "subscriptions.add":
@@ skipping 26 matching lines @@
             return false;
           if (s instanceof DownloadableSubscription && message.downloadable)
             return true;
           if (s instanceof SpecialSubscription && message.special)
             return true;
           return false;
         });
         callback(subscriptions.map(convertSubscription));
         break;
       case "subscriptions.listen":
+        var listenerFilters = getListenerFilters(sender.page, true);

[Thomas Greiner, 2016/03/21 17:55:55]

Nice. No idea why we didn't implement it like this before already.

         if (message.filter)
           listenerFilters.subscription = message.filter;
         else
           delete listenerFilters.subscription;
         break;
       case "subscriptions.remove":
         var subscription = Subscription.fromURL(message.url);
         if (subscription.url in FilterStorage.knownSubscriptions)
           FilterStorage.removeSubscription(subscription);
         break;
@@ skipping 29 matching lines @@
           if (subscription instanceof DownloadableSubscription)
             Synchronizer.execute(subscription, true);
         }
         break;
       case "subscriptions.isDownloading":
         callback(Synchronizer.isExecuting(message.url));
         break;
     }
   });
 })(this);