576553 - Don't restart for extension installation any more (No EM restart)

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Benjamin Smedberg]

Don't restart for extension installation any more (No EM restart)

Comment 1

[(not currently active) Ted Mielczarek]

When we fix this it'd be nice to fix the automation.py consumers that do a double-launch to work around this currently.

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py.in#598
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/runreftest.py#145

Comment 2

[Benjamin Smedberg]

Attachment: EM doesn't need to cause restarts any more, rev. 1

Comment 3

[Dave Townsend [:mossop]]

Taking this over, there are a bunch of test changes also required and some things depends on this restart right now. Should be easy to fix them though.

Comment 4

[Dave Townsend [:mossop]]

Attachment: remove restart logic from nsAppRunner.cpp

This removes the restart logic from nsAppRunner which is no longer necessary. Will attach a patch with whitespace changes ignored in a moment since there is lots of indentation changes here.

Comment 5

[Dave Townsend [:mossop]]

Attachment: remove restart logic from nsAppRunner.cpp (-w)

Same patch with whitespace changes ignored

Comment 6

[Dave Townsend [:mossop]]

Attachment: remove need for restart from add-ons manager

This removes the restart from the add-ons manager. There is a bit of code rearrangement here. Having checkForChanges on providers is pretty redundant now so it is just removed. XPIProvider calls its checkForChanges inside of startup. Some code is moved around to ensure we do the startup tasks after checkForChanges has made any relevant changes.

Then there is all the changes to the tests harness to remove the restart support.

Comment 7

[Dave Townsend [:mossop]]

Don't think we would block the release on it. Really really want it though. I don't think we can directly automatically test this either but I suspect our automated test suites will fail if the app restarts during startup after these changes.

Comment 8

[Robert Strong (they/them - no direct email)]

Quick note: seems like the perfect opportunity to move shuttingDown from nsIAppStartup2 to nsIAppStartup

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 456888 [details] [diff] [review]
remove need for restart from add-ons manager

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>@@ -1025,56 +1025,79 @@ var XPIProvider = {
>                                 [DIR_EXTENSIONS],
>                                 AddonManager.SCOPE_PROFILE, false);
> 
>     this.defaultSkin = Prefs.getDefaultCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN,
>                                                 "classic/1.0");
>     this.currentSkin = Prefs.getCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN,
>                                          this.defaultSkin);
>     this.selectedSkin = this.currentSkin;
>-
>-    // Tell the Chrome Registry which Skin to select
>-    if (Prefs.getBoolPref(PREF_DSS_SWITCHPENDING, false)) {
>-      try {
>-        this.selectedSkin = Prefs.getCharPref(PREF_DSS_SKIN_TO_SELECT);
>-        Services.prefs.setCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN,
>-                                   this.selectedSkin);
>-        Services.prefs.clearUserPref(PREF_DSS_SKIN_TO_SELECT);
>-        LOG("Changed skin to " + this.selectedSkin);
>-        this.currentSkin = this.selectedSkin;
>-      }
>-      catch (e) {
>-        ERROR(e);
>-      }
>-      Services.prefs.clearUserPref(PREF_DSS_SWITCHPENDING);
>-    }
>+    this.applyThemeChange();
> 
>     var version = Services.appinfo.version.replace(BRANCH_REGEXP, "$1");
>     this.checkCompatibilityPref = PREF_EM_CHECK_COMPATIBILITY + "." + version;
>     this.checkCompatibility = Prefs.getBoolPref(this.checkCompatibilityPref,
>                                                 true)
>     this.checkUpdateSecurity = Prefs.getBoolPref(PREF_EM_CHECK_UPDATE_SECURITY,
>                                                  true)
>+    this.enabledAddons = [];
>+
>+    Services.prefs.addObserver(this.checkCompatibilityPref, this, false);
>+    Services.prefs.addObserver(PREF_EM_CHECK_UPDATE_SECURITY, this, false);
>+
>+    this.checkForChanges(aAppChanged);
>+
>+    // Changes to installed extensions may have changed which theme is selected
>+    this.applyThemeChange();
Seems strange to have to call applyThemeChange twice during startup. Can this be reworked to only call it once?

>+
>+    // Check that the add-ons list still exists
>+    let addonsList = FileUtils.getFile(KEY_PROFILEDIR, [FILE_XPI_ADDONS_LIST],
>+                                       true);
>+    if (!addonsList.exists()) {
>+      LOG("Add-ons list is missing, recreating");
>+      XPIDatabase.writeAddonsList([]);
>+    }
Why move this from checkForChanges to here? It used to set the PREF_BOOTSTRAP_ADDONS preference as well... why is this no longer necessary?

>@@ -1755,71 +1800,34 @@ var XPIProvider = {
>         let oldSkin = XPIDatabase.getVisibleAddonForInternalName(this.currentSkin);
>         if (!oldSkin || oldSkin.appDisabled)
>           this.enableDefaultTheme();
>       }
> 
>       // If the application crashed before completing any pending operations then
>       // we should perform them now.
>       if (changed || Prefs.getBoolPref(PREF_PENDING_OPERATIONS)) {
>-        LOG("Restart necessary");
>+        LOG("Updating database from changes to installed add-ons");
nit s/from/with/

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 456888 [details] [diff] [review]
remove need for restart from add-ons manager

Looks good but I'd like replies to the questions in comment #9 so minusing for now.

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #9)
> Comment on attachment 456888 [details] [diff] [review]
> remove need for restart from add-ons manager
> 
> >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
> >--- a/toolkit/mozapps/extensions/XPIProvider.jsm
> >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
> >...
> >@@ -1025,56 +1025,79 @@ var XPIProvider = {
> >                                 [DIR_EXTENSIONS],
> >                                 AddonManager.SCOPE_PROFILE, false);
> > 
> >     this.defaultSkin = Prefs.getDefaultCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN,
> >                                                 "classic/1.0");
> >     this.currentSkin = Prefs.getCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN,
> >                                          this.defaultSkin);
> >     this.selectedSkin = this.currentSkin;
> >-
> >-    // Tell the Chrome Registry which Skin to select
> >-    if (Prefs.getBoolPref(PREF_DSS_SWITCHPENDING, false)) {
> >-      try {
> >-        this.selectedSkin = Prefs.getCharPref(PREF_DSS_SKIN_TO_SELECT);
> >-        Services.prefs.setCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN,
> >-                                   this.selectedSkin);
> >-        Services.prefs.clearUserPref(PREF_DSS_SKIN_TO_SELECT);
> >-        LOG("Changed skin to " + this.selectedSkin);
> >-        this.currentSkin = this.selectedSkin;
> >-      }
> >-      catch (e) {
> >-        ERROR(e);
> >-      }
> >-      Services.prefs.clearUserPref(PREF_DSS_SWITCHPENDING);
> >-    }
> >+    this.applyThemeChange();
> > 
> >     var version = Services.appinfo.version.replace(BRANCH_REGEXP, "$1");
> >     this.checkCompatibilityPref = PREF_EM_CHECK_COMPATIBILITY + "." + version;
> >     this.checkCompatibility = Prefs.getBoolPref(this.checkCompatibilityPref,
> >                                                 true)
> >     this.checkUpdateSecurity = Prefs.getBoolPref(PREF_EM_CHECK_UPDATE_SECURITY,
> >                                                  true)
> >+    this.enabledAddons = [];
> >+
> >+    Services.prefs.addObserver(this.checkCompatibilityPref, this, false);
> >+    Services.prefs.addObserver(PREF_EM_CHECK_UPDATE_SECURITY, this, false);
> >+
> >+    this.checkForChanges(aAppChanged);
> >+
> >+    // Changes to installed extensions may have changed which theme is selected
> >+    this.applyThemeChange();
> Seems strange to have to call applyThemeChange twice during startup. Can this
> be reworked to only call it once?

I can look to try however it may get complicated. The problem that this is trying to solve is the case where a user switches to a new theme and then restarts. During the restart the theme they have switched to may get made incompatible (by an app upgrade) or be disabled (by removing its folder). The way this works right now is to set the selected theme properly before going into checkForChanges and then if that makes any theme change re-set the selected theme. If it doesn't applyThemeChange will be basically a no-op due to the test at the top of it.

> >+    // Check that the add-ons list still exists
> >+    let addonsList = FileUtils.getFile(KEY_PROFILEDIR, [FILE_XPI_ADDONS_LIST],
> >+                                       true);
> >+    if (!addonsList.exists()) {
> >+      LOG("Add-ons list is missing, recreating");
> >+      XPIDatabase.writeAddonsList([]);
> >+    }
> Why move this from checkForChanges to here? It used to set the
> PREF_BOOTSTRAP_ADDONS preference as well... why is this no longer necessary?

Yeah I can move that back into checkForChanges. It used to set the preference because it would also bail out early at that point and tell the app to restart immediately. Now there is no restart so we continue on and the preference eventually gets set in the quit-application-granted observer.

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 456888 [details] [diff] [review]
remove need for restart from add-ons manager

(In reply to comment #11)
> (In reply to comment #9)
>...
> > >+    this.applyThemeChange();
> > Seems strange to have to call applyThemeChange twice during startup. Can this
> > be reworked to only call it once?
> 
> I can look to try however it may get complicated. The problem that this is
> trying to solve is the case where a user switches to a new theme and then
> restarts. During the restart the theme they have switched to may get made
> incompatible (by an app upgrade) or be disabled (by removing its folder). The
> way this works right now is to set the selected theme properly before going
> into checkForChanges and then if that makes any theme change re-set the
> selected theme. If it doesn't applyThemeChange will be basically a no-op due to
> the test at the top of it.
Sounds fine

> > Why move this from checkForChanges to here? It used to set the
> > PREF_BOOTSTRAP_ADDONS preference as well... why is this no longer necessary?
> 
> Yeah I can move that back into checkForChanges. It used to set the preference
> because it would also bail out early at that point and tell the app to restart
> immediately. Now there is no restart so we continue on and the preference
> eventually gets set in the quit-application-granted observer.
Since checkForChanges can call writeAddonsList it seems like it would be better moved back.

r=me with that and the nit fixed mentioned previously

Comment 13

[Dave Townsend [:mossop]]

Attachment: remove need for restart from add-ons manager rev 2

Moved that code and fixed the nit.

Comment 14

[Benjamin Smedberg]

Comment on attachment 456887 [details] [diff] [review]
remove restart logic from nsAppRunner.cpp (-w)

Woot

Comment 15

[Dave Townsend [:mossop]]

And there was much rejoicing: http://hg.mozilla.org/mozilla-central/rev/8c7df4612f8e

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, what are the paths we will have to test to verify this fix?

Comment 17

[Dave Townsend [:mossop]]

One way to test this would be to install an extension, exit Firefox and then start it from the console, if you get the console prompt back while Firefox opens then it has restarted during startup, if not then there was no restart during startup. I don't really think it is something you could do as a part of a traditional litmus test though.

Comment 18

[Jesse Ruderman]

The code for restarting is gone, so we don't really need to test that it doesn't happen.  It's better to identify actions whose implementations have changed (here and in bug 568691), and make sure we have good tests for those things.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #17)
> One way to test this would be to install an extension, exit Firefox and then
> start it from the console, if you get the console prompt back while Firefox
> opens then it has restarted during startup, if not then there was no restart

Does that only work on OS X and Windows? It works fine there but on Linux I never get back to the console even for Shiretoko and Namoroka. So I wonder how to test it on Linux.

(In reply to comment #18)
> The code for restarting is gone, so we don't really need to test that it
> doesn't happen.  It's better to identify actions whose implementations have
> changed (here and in bug 568691), and make sure we have good tests for those
> things.

Jesse, I would say that we should handle that on another bug. Could you file one with your initial thoughts?

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #19)
> (In reply to comment #17)
> > One way to test this would be to install an extension, exit Firefox and then
> > start it from the console, if you get the console prompt back while Firefox
> > opens then it has restarted during startup, if not then there was no restart
> 
> Does that only work on OS X and Windows? It works fine there but on Linux I
> never get back to the console even for Shiretoko and Namoroka. So I wonder how
> to test it on Linux.

Should be the same on Linux.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on OS X and Windows with builds like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100719 Minefield/4.0b2pre

I'm still not able to verify it on Linux because we don't return to the prompt on the Ubuntu box I'm using. But as Dave mentioned, it works fine for him. So lets call it verified fixed.

Comment 22

[timeless]

for windows builds you should be able to use:

start /wait firefox.exe

it shouldn't give you a prompt until firefox exits