Issue 3247 - Closing the browser shortly after initial launch(s) causes no filters lists to be sele…

adblockplus/Api.jsm

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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 19 matching lines @@
 
 function require(module)
 {
   let result = {};
   result.wrappedJSObject = result;
   Services.obs.notifyObservers(result, "adblockplus-require", module);
   return result.exports;
 }
 
 let {Filter} = require("filterClasses");
+let {FilterNotifier} = require("filterNotifier");
 let {FilterStorage} = require("filterStorage");
 let {defaultMatcher} = require("matcher");
 let {Prefs} = require("prefs");
 let {Subscription, SpecialSubscription, RegularSubscription, DownloadableSubscription, ExternalSubscription} = require("subscriptionClasses");
 let {Synchronizer} = require("synchronizer");
 let {UI} = require("ui");
 
+let subscriptionsSavedPref = "subscriptions_saved";
+
+function initListeners()
+{
+  FilterNotifier.on("load", onLoad);
+  FilterNotifier.on("save", onSave);
+}
+
+function onLoad()
+{
+  let {addonVersion} = require("info");
+  if (Prefs.currentVersion == addonVersion && !getBoolPref(subscriptionsSavedPref))
+  {
+    UI.addSubscription(UI.currentWindow, Prefs.currentVersion);

[Felix Dahlke, 2016/09/30 07:44:12]

This'll solve the subscription issue I guess, but shouldn't we run all the first
run actions (i.e. `UI.firstRunActions`) again if they weren't previously
completed, not just the `addSubscription` step? (I might be missing something,
it's been a little while since I was working on this code.)

[diegocarloslima, 2016/10/25 16:16:20]

Actually firstRunActions is always performed. The issue is on this snipped from
ui.js:
    let prevVersion = Prefs.currentVersion;
    if (prevVersion != addonVersion)
    {
      Prefs.currentVersion = addonVersion;
      this.addSubscription(window, prevVersion);

Here, Prefs.currentVersion is stored before addSubscription completes. So if we
kill the app before addSubscription is stored on disk, next time this is run, it
will fail for the prevVersion != addonVersion check, because it was already
stored and it will never complete the addSubscription call

[Felix Dahlke, 2016/10/27 17:12:49]

Acknowledged.

+  }
+}
+
+function onSave()
+{
+  if (FilterStorage.subscriptions.some((subscription) => subscription instanceof DownloadableSubscription && subscription.url != Prefs.subscriptions_exceptionsurl))
+  {
+    setBoolPref(subscriptionsSavedPref, true);
+  }
+}
+
+function getBoolPref(name)
+{
+  let branch = getPrefsBranch();
+  try
+  {
+    return branch.getBoolPref(name);
+  }
+  catch (e)
+  {
+    return null;

[anton, 2016/09/30 06:39:04]

LGTM in general if hiding exception with just `null` is ok for js (in java it's
not good usually)

[Felix Dahlke, 2016/09/30 07:44:12]

That's alright, but because of type coercion, this is going to be treated the
same as `false` in most cases (e.g. in the condition above). Just something to
be aware of.

[diegocarloslima, 2016/10/25 16:16:20]

 I did that way to have a more flexible way of handling what is returned by
getBoolPref, it could be useful for checking if any value has ever being defined
for that preference

+  }
+}
+
+function setBoolPref(name, value)
+{
+  let branch = getPrefsBranch();
+  branch.setBoolPref(name, value);
+  Services.prefs.savePrefFile(null);
+}
+
+function getPrefsBranch()
+{
+  let {addonRoot, addonName} = require("info");
+  let branchName = "extensions." + addonName + ".";
+  return Services.prefs.getBranch(branchName);
+}
+
 function getWhitelistingFilter(url)
 {
   let uriObject = Services.io.newURI(url, null, null);
   try
   {
     return defaultMatcher.whitelist.matchesAny(
       uriObject.spec, "DOCUMENT", uriObject.host, false, null);
   }
   catch (e)
   {
@@ skipping 98 matching lines @@
       {
         FilterStorage.removeFilter(filter);
         if (filter.subscriptions.length)
           filter.disabled = true;
         filter = getWhitelistingFilter(url);
       }
     }
   },
   initCommunication: function()
   {
+    initListeners();
+
     Messaging.addListener((function(data)
     {
       if (!data)
         return {"success": false, "error": "malformed request"};
 
       if (data["action"] == "getFiltersLoaded")
         return {"success": true, "value": this.filtersLoaded};
 
       if (!this.filtersLoaded)
         return {"success": false, "error": "filters not loaded"};
@@ skipping 49 matching lines @@
           {
             this.whitelistSite(data["url"], data["whitelisted"]);
             return {"success": true};
           }
           break;
       }
       return {"success": false, "error": "malformed request"};
     }).bind(this), "AdblockPlus:Api");
   }
 };