Closed
Bug 613294
Opened 14 years ago
Closed 14 years ago
Upgrading a restartless add-on to a non-restartless add-on doesn't call the uninstall method
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla6
Tracking | Status | |
---|---|---|
blocking2.0 | --- | .x+ |
People
(Reporter: mossop, Assigned: mossop)
Details
Attachments
(1 file, 1 obsolete file)
15.82 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
The code recognises that the updating requires a restart so we don't immediately shutdown and uninstall the old add-on and instead do the upgrade at the restart, but the code doesn't currently handle calling the uninstall method on the old add-on at this point.
Assignee | ||
Comment 1•14 years ago
|
||
This should probably block as it is likely that add-on developers will switch between restartless and non-restartless depending on their needs.
blocking2.0: --- → betaN+
Comment 2•14 years ago
|
||
Notes from the Grand Retriage: worth fixing on branch. Not worth holding FF4 for the hypothetical addon authors porting to restartless *and then back again*.
blocking2.0: betaN+ → .x
Assignee | ||
Updated•14 years ago
|
Whiteboard: [wanted fx5]
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → dtownsend
Assignee | ||
Comment 3•14 years ago
|
||
Relatively straightforward. Check the list of active bootstrapped add-ons to see if we're about to upgrade one and if so call its uninstall method. We have to be sure to load the manifest from the new add-on earlier than previously to be sure we get the reason code right but this is only moving work not doing anything more.
Attachment #527548 -
Flags: review?(robert.bugzilla)
![]() |
||
Comment 4•14 years ago
|
||
Comment on attachment 527548 [details] [diff] [review]
patch rev 1
>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
>@@ -1817,68 +1817,114 @@ var XPIProvider = {
> ERROR("Failed to uninstall add-on " + id + " in " + aLocation.name, e);
> }
> // The file check later will spot the removal and cleanup the database
> continue;
> }
> }
>
> aManifests[aLocation.name][id] = null;
>- let existingAddonID = null;
>+ let existingAddonID = id;
>
> // Check for a cached AddonInternal for this add-on, it may contain
> // updated compatibility information
> let jsonfile = stagingDir.clone();
> jsonfile.append(id + ".json");
> if (jsonfile.exists()) {
> LOG("Found updated manifest for " + id + " in " + aLocation.name);
> let fis = Cc["@mozilla.org/network/file-input-stream;1"].
> createInstance(Ci.nsIFileInputStream);
> let json = Cc["@mozilla.org/dom/json;1"].
> createInstance(Ci.nsIJSON);
>
> try {
> fis.init(jsonfile, -1, 0, 0);
> aManifests[aLocation.name][id] = json.decodeFromStream(fis,
> jsonfile.fileSize);
>- existingAddonID = aManifests[aLocation.name][id].existingAddonID;
>+ existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
nit: this is a tad weird since existingAddonID is already id but I think I'm ok with it
> }
> catch (e) {
> ERROR("Unable to read add-on manifest for " + id + " in " +
> aLocation.name, e);
> }
> finally {
> fis.close();
> }
> }
>
>+ // If there was no cached AddonInternal then load it directly
>+ if (!aManifests[aLocation.name][id]) {
>+ try {
>+ aManifests[aLocation.name][id] = loadManifestFromFile(stageDirEntry);
>+ existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
>+ }
>+ catch (e) {
>+ // This add-on can't be installed so just remove it now
>+ stageDirEntry.remove(true);
>+ ERROR("Unable to read add-on manifest for " + id + " in " +
>+ aLocation.name, e);
This is even weirder in that the previous error has the exact same text.
![]() |
||
Comment 5•14 years ago
|
||
Comment on attachment 527548 [details] [diff] [review]
patch rev 1
>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
>@@ -1817,68 +1817,114 @@ var XPIProvider = {
>...
> try {
> fis.init(jsonfile, -1, 0, 0);
> aManifests[aLocation.name][id] = json.decodeFromStream(fis,
> jsonfile.fileSize);
>- existingAddonID = aManifests[aLocation.name][id].existingAddonID;
>+ existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
> }
> catch (e) {
> ERROR("Unable to read add-on manifest for " + id + " in " +
> aLocation.name, e);
should this still be an error now that you have a fallback below?
> }
> finally {
> fis.close();
> }
> }
>
>+ // If there was no cached AddonInternal then load it directly
>+ if (!aManifests[aLocation.name][id]) {
>+ try {
>+ aManifests[aLocation.name][id] = loadManifestFromFile(stageDirEntry);
>+ existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
>+ }
>+ catch (e) {
>+ // This add-on can't be installed so just remove it now
>+ stageDirEntry.remove(true);
>+ ERROR("Unable to read add-on manifest for " + id + " in " +
>+ aLocation.name, e);
>+ }
>+ }
>+
> LOG("Processing install of " + id + " in " + aLocation.name);
>+ if (existingAddonID in this.bootstrappedAddons) {
>+ var oldBootstrap = this.bootstrappedAddons[existingAddonID];
>+ try {
>+ var existingAddon = aLocation.getLocationForID(existingAddonID);
>+ if (oldBootstrap.descriptor == existingAddon.persistentDescriptor) {
>+ // We'll be replacing a currently active bootstrapped add-on so
>+ // call its uninstall method
>+ let oldVersion = aManifests[aLocation.name][id].version;
>+ let newVersion = oldBootstrap.version;
>+ let uninstallReason = Services.vc.compare(newVersion, oldVersion) < 0 ?
>+ BOOTSTRAP_REASONS.ADDON_UPGRADE :
>+ BOOTSTRAP_REASONS.ADDON_DOWNGRADE;
>+
>+ this.callBootstrapMethod(existingAddonID, oldBootstrap.version,
>+ existingAddon, "uninstall", uninstallReason);
>+ this.unloadBootstrapScope(existingAddonID);
>+ }
>+ else {
>+ oldBootstrap = null;
>+ }
>+ }
>+ catch (e) {
>+ }
>+ }
>+
> try {
> var addonInstallLocation = aLocation.installAddon(id, stageDirEntry,
> existingAddonID);
> if (aManifests[aLocation.name][id])
> aManifests[aLocation.name][id]._sourceBundle = addonInstallLocation;
> }
> catch (e) {
> ERROR("Failed to install staged add-on " + id + " in " + aLocation.name,
> e);
> delete aManifests[aLocation.name][id];
>+
>+ if (oldBootstrap) {
>+ // Re-install the old add-on
>+ this.callBootstrapMethod(existingAddonID, oldBootstrap.version,
>+ existingAddon, "install",
>+ BOOTSTRAP_REASONS.ADDON_INSTALL);
There is a case where you set oldBootstrap to null above... is that somehow ok?
Attachment #527548 -
Flags: review?(robert.bugzilla) → review-
![]() |
||
Comment 6•14 years ago
|
||
(In reply to comment #5)
>...
> There is a case where you set oldBootstrap to null above... is that somehow ok?
Specifically, if the add-on should be reinstalled?
Assignee | ||
Comment 7•14 years ago
|
||
(In reply to comment #4)
> >+ // If there was no cached AddonInternal then load it directly
> >+ if (!aManifests[aLocation.name][id]) {
> >+ try {
> >+ aManifests[aLocation.name][id] = loadManifestFromFile(stageDirEntry);
> >+ existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
> >+ }
> >+ catch (e) {
> >+ // This add-on can't be installed so just remove it now
> >+ stageDirEntry.remove(true);
> >+ ERROR("Unable to read add-on manifest for " + id + " in " +
> >+ aLocation.name, e);
> This is even weirder in that the previous error has the exact same text.
Changed the messages to specify what file they were trying to load the manifest from.
(In reply to comment #5)
> Comment on attachment 527548 [details] [diff] [review]
> patch rev 1
>
> >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
> >@@ -1817,68 +1817,114 @@ var XPIProvider = {
> >...
> > try {
> > fis.init(jsonfile, -1, 0, 0);
> > aManifests[aLocation.name][id] = json.decodeFromStream(fis,
> > jsonfile.fileSize);
> >- existingAddonID = aManifests[aLocation.name][id].existingAddonID;
> >+ existingAddonID = aManifests[aLocation.name][id].existingAddonID || id;
> > }
> > catch (e) {
> > ERROR("Unable to read add-on manifest for " + id + " in " +
> > aLocation.name, e);
> should this still be an error now that you have a fallback below?
I think so, it suggests that the json file is corrupt somehow, we'd need to know that we failed to load it if we experience issues with compat updates for new installs seeming to not apply.
> >+
> >+ if (oldBootstrap) {
> >+ // Re-install the old add-on
> >+ this.callBootstrapMethod(existingAddonID, oldBootstrap.version,
> >+ existingAddon, "install",
> >+ BOOTSTRAP_REASONS.ADDON_INSTALL);
> There is a case where you set oldBootstrap to null above... is that somehow ok?
Changed this a little but basically when oldBootstrap is null it means the new add-on isn't replacing an existing bootstrapped add-on, so there will be nothing to call the install method on again.
Assignee | ||
Comment 8•14 years ago
|
||
Updated patch
Attachment #527548 -
Attachment is obsolete: true
Attachment #528148 -
Flags: review?(robert.bugzilla)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [wanted fx5] → [wanted fx5][has patch][needs review rs]
![]() |
||
Comment 9•14 years ago
|
||
Comment on attachment 528148 [details] [diff] [review]
patch rev 2
>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
>...
>@@ -1817,68 +1816,112 @@ var XPIProvider = {
>...
>+ var oldBootstrap = null;
> LOG("Processing install of " + id + " in " + aLocation.name);
>+ if (existingAddonID in this.bootstrappedAddons) {
>+ try {
>+ var existingAddon = aLocation.getLocationForID(existingAddonID);
>+ if (this.bootstrappedAddons[existingAddonID].descriptor ==
>+ existingAddon.persistentDescriptor) {
>+ oldBootstrap = this.bootstrappedAddons[existingAddonID];
>+
>+ // We'll be replacing a currently active bootstrapped add-on so
>+ // call its uninstall method
>+ let oldVersion = aManifests[aLocation.name][id].version;
>+ let newVersion = oldBootstrap.version;
>+ let uninstallReason = Services.vc.compare(newVersion, oldVersion) < 0 ?
>+ BOOTSTRAP_REASONS.ADDON_UPGRADE :
>+ BOOTSTRAP_REASONS.ADDON_DOWNGRADE;
What should happen if it is the same version... followup bug?
Attachment #528148 -
Flags: review?(robert.bugzilla) → review+
Assignee | ||
Comment 10•14 years ago
|
||
(In reply to comment #9)
> Comment on attachment 528148 [details] [diff] [review]
> patch rev 2
>
> >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
> >...
> >@@ -1817,68 +1816,112 @@ var XPIProvider = {
> >...
> >+ var oldBootstrap = null;
> > LOG("Processing install of " + id + " in " + aLocation.name);
> >+ if (existingAddonID in this.bootstrappedAddons) {
> >+ try {
> >+ var existingAddon = aLocation.getLocationForID(existingAddonID);
> >+ if (this.bootstrappedAddons[existingAddonID].descriptor ==
> >+ existingAddon.persistentDescriptor) {
> >+ oldBootstrap = this.bootstrappedAddons[existingAddonID];
> >+
> >+ // We'll be replacing a currently active bootstrapped add-on so
> >+ // call its uninstall method
> >+ let oldVersion = aManifests[aLocation.name][id].version;
> >+ let newVersion = oldBootstrap.version;
> >+ let uninstallReason = Services.vc.compare(newVersion, oldVersion) < 0 ?
> >+ BOOTSTRAP_REASONS.ADDON_UPGRADE :
> >+ BOOTSTRAP_REASONS.ADDON_DOWNGRADE;
> What should happen if it is the same version... followup bug?
Turns out everywhere has the same problem so I've filed bug 652645 to come to a solution. For now this matches what happens elsewhere so I'll land this.
Assignee | ||
Comment 11•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [wanted fx5][has patch][needs review rs] → [wanted fx5]
Target Milestone: --- → mozilla6
Comment 12•14 years ago
|
||
Dave, when has the uninstall method being called? Is it after the restart? Because that's what I'm seeing with my example extension update.
Assignee | ||
Comment 13•14 years ago
|
||
(In reply to comment #12)
> Dave, when has the uninstall method being called? Is it after the restart?
> Because that's what I'm seeing with my example extension update.
Correct, because the non-restartless add-on doesn't get installed until after the restart we don't call uninstall on the old one until then.
Comment 14•14 years ago
|
||
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110529 Firefox/6.0a2
Dave, can we remove the [wanted-fx5] whiteboard entry? I don't think we want to get this patch onto the 5 branch.
Status: RESOLVED → VERIFIED
Assignee | ||
Updated•14 years ago
|
Whiteboard: [wanted fx5]
You need to log in
before you can comment on or make changes to this bug.
Description
•