Closed
Bug 576553
Opened 15 years ago
Closed 15 years ago
Don't restart for extension installation any more (No EM restart)
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla2.0b2
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: benjamin, Assigned: mossop)
References
Details
Attachments
(3 files, 2 obsolete files)
20.59 KB,
patch
|
Details | Diff | Splinter Review | |
13.48 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
96.98 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
Don't restart for extension installation any more (No EM restart)
Comment 1•15 years ago
|
||
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
Reporter | ||
Comment 2•15 years ago
|
||
Attachment #455705 -
Flags: review?(dtownsend)
Updated•15 years ago
|
blocking2.0: --- → ?
Flags: in-testsuite?
Flags: in-litmus?
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee | ||
Comment 3•15 years ago
|
||
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.
Assignee: benjamin → dtownsend
Assignee | ||
Comment 4•15 years ago
|
||
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.
Attachment #455705 -
Attachment is obsolete: true
Attachment #456886 -
Flags: review?
Attachment #455705 -
Flags: review?(dtownsend)
Assignee | ||
Comment 5•15 years ago
|
||
Same patch with whitespace changes ignored
Assignee | ||
Comment 6•15 years ago
|
||
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.
Attachment #456888 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 7•15 years ago
|
||
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.
blocking2.0: ? → -
Flags: in-testsuite? → in-testsuite-
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Comment 8•15 years ago
|
||
Quick note: seems like the perfect opportunity to move shuttingDown from nsIAppStartup2 to nsIAppStartup
Comment 9•15 years ago
|
||
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•15 years ago
|
||
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.
Attachment #456888 -
Flags: review?(robert.bugzilla) → review-
Assignee | ||
Comment 11•15 years ago
|
||
(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•15 years ago
|
||
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
Attachment #456888 -
Flags: review- → review+
Assignee | ||
Comment 13•15 years ago
|
||
Moved that code and fixed the nit.
Attachment #456888 -
Attachment is obsolete: true
Attachment #457151 -
Flags: review+
Assignee | ||
Updated•15 years ago
|
Attachment #456886 -
Flags: review? → review?(benjamin)
Reporter | ||
Comment 14•15 years ago
|
||
Comment on attachment 456887 [details] [diff] [review]
remove restart logic from nsAppRunner.cpp (-w)
Woot
Attachment #456887 -
Flags: review+
Assignee | ||
Comment 15•15 years ago
|
||
And there was much rejoicing: http://hg.mozilla.org/mozilla-central/rev/8c7df4612f8e
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3b2
Reporter | ||
Updated•15 years ago
|
Attachment #456886 -
Flags: review?(benjamin)
Comment 16•15 years ago
|
||
Dave, what are the paths we will have to test to verify this fix?
Assignee | ||
Comment 17•15 years ago
|
||
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•15 years ago
|
||
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.
Flags: in-litmus? → in-litmus-
Comment 19•15 years ago
|
||
(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?
Assignee | ||
Comment 20•15 years ago
|
||
(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•15 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Comment 22•15 years ago
|
||
for windows builds you should be able to use:
start /wait firefox.exe
it shouldn't give you a prompt until firefox exits
You need to log in
before you can comment on or make changes to this bug.
Description
•