Closed Bug 596336 Opened 14 years ago Closed 14 years ago

bootstrap.js functions are not called for subsequent installs in the same browser instance

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla2.0b7
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dietrich, Assigned: mossop)

References

Details

Attachments

(3 files, 1 obsolete file)

backend patch rev 1
10.95 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
frontend patch rev 2
30.45 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
bustage fixes
7.00 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review 
STR:

1. open firefox
2. install a bootstrapped add-on (confirm via dump or whatever, that startup() is called)
3. uninstall it
4. install it again

Expected: startup() is called

Actual: startup() is not called

note: i confirmed that this applies to shutdown() as well. i haven't tried the others.

maybe related to bug 594490 somehow, as i was seeing that too.
requesting blocking: add-ons will very likely malfunction if their bootstrap functions are not called.
blocking2.0: --- → betaN+
Whiteboard: 596336
Whiteboard: 596336
Lets add dependency for now. Bug 594490 is a bit vague while we have a clear issue here.
Blocks: 594490
Ok I can reproduce this now, it is tied up with bug 553494 and it is very likely that bug 594490 is the same issue.

1. Install a restartless add-on.
2. Open the add-ons manager and click remove, do not change panes though (at this point the add-on is actually just disabled).
3. Install the restartless add-on again.

The uninstall script is called for the old version and the install script is called for the new. The startup script isn't called, because the new version is disabled too. Now you'll also have the new version in the list and the old version offering to undo the install in the list. At this point switching around panes throws errors into the console and things are generally wrong in the database.
Assignee: nobody → dtownsend
Need a small change to the backend here. Previously when upgrading an add-on the userDisabled setting just gets carried over from what the previous version had. This change allows that to be modified during the install process. Specifically if the userDisabled value for an add-on on an install changes before onInstallEnded is fired then that is the final value used when installed.

We do this by firstly setting the initial userDisabled on the new Addon from the old one (really we should have been doing this anyway), and then removing the carry-over from updateAddonMetadata and adding it back to the one case where an update happens and no saved metadata is available.
Attachment #478577 - Flags: review?(robert.bugzilla)
Attached patch frontend patch rev 1 (obsolete) — — Splinter Review 
For the frontend there are two issues. Firstly when a bootstrapped add-on is upgraded and is visible in the list we must update the list item to use the new add-on object. Secondly for those pending uninstall we have to make sure the new version gets the right disabled state.
Attachment #478578 - Flags: review?(bmcbride)
Status: NEW → ASSIGNED
Whiteboard: [has patch][needs review Unfocused][needs review rs]
Comment on attachment 478578 [details] [diff] [review]
frontend patch rev 1

This should make fixing bug 598863 pretty easy too. Actually, I wonder if that should be fixed here - its very related to these fixes, and should only be a line or two.

>       <method name="onInstallEnded">
>+        <parameter name="aInstall"/>
>+        <parameter name="aAddon"/>
>         <body><![CDATA[
>-          this._updateState();
>+          // If the install completed without needing a restart then switch to
>+          // using the new Addon
>+          if (!(aAddon.pendingOperations & AddonManager.PENDING_INSTALL))
>+            this._initWithAddon(aAddon);

I think this needs to take into account PENDING_UPGRADE too.

>+      <method name="onInstallStarted">
>+        <parameter name="aInstall"/>
>+        <body><![CDATA[
>+          // Make sure any newly installed add-on has the correct disabled state
>+          aInstall.addon.userDisabled = this.getAttribute("wasDisabled") == "true";

Check the wasDisabled attribute exists first.

>+      <method name="onInstallEnded">
>+        <parameter name="aInstall"/>
>+        <parameter name="aAddon"/>
>+        <body><![CDATA[
>+          // If the install completed without needing a restart then switch to
>+          // using the new Addon
>+          if (!(aAddon.pendingOperations & AddonManager.PENDING_INSTALL))
>+            this.mAddon = aAddon;

Again, needs to take into account PENDING_UPGRADE.

>diff --git a/toolkit/mozapps/extensions/test/browser/browser_bug596336.js b/toolkit/mozapps/extensions/test/browser/browser_bug596336.js
>+function get_items_in_list() {

Function name here is misleading - get_num_items_in_list()? Or something.
Attachment #478578 - Flags: review?(bmcbride) → review-
Whiteboard: [has patch][needs review Unfocused][needs review rs] → [has patch][needs review rs]
Blocks: 600031
(In reply to comment #6)
> Comment on attachment 478578 [details] [diff] [review]
> frontend patch rev 1
> 
> This should make fixing bug 598863 pretty easy too. Actually, I wonder if that
> should be fixed here - its very related to these fixes, and should only be a
> line or two.

And by "line or two" you actually meant lots of lines plus tests but I guess it found a bug so ok.

> >       <method name="onInstallEnded">
> >+        <parameter name="aInstall"/>
> >+        <parameter name="aAddon"/>
> >         <body><![CDATA[
> >-          this._updateState();
> >+          // If the install completed without needing a restart then switch to
> >+          // using the new Addon
> >+          if (!(aAddon.pendingOperations & AddonManager.PENDING_INSTALL))
> >+            this._initWithAddon(aAddon);
> 
> I think this needs to take into account PENDING_UPGRADE too.

I don't think it does, PENDING_UPGRADE can only be true for already installed add-ons not new ones, technically if we get here and the new add-on is PENDING_INSTALL then the existing add-on must be PENDING_UPGRADE (assuming the provider isn't broken at least).
Attachment #478578 - Attachment is obsolete: true
Attachment #478918 - Flags: review?(bmcbride)
Blocks: 598863
Whiteboard: [has patch][needs review rs] → [has patch][needs review rs][needs review Unfocused]
Comment on attachment 478918 [details] [diff] [review]
frontend patch rev 2

(In reply to comment #7)
> And by "line or two" you actually meant lots of lines plus tests

Oops.

> > I think this needs to take into account PENDING_UPGRADE too.
> 
> I don't think it does, PENDING_UPGRADE can only be true for already installed
> add-ons not new ones, technically if we get here and the new add-on is
> PENDING_INSTALL then the existing add-on must be PENDING_UPGRADE (assuming the
> provider isn't broken at least).

Hmm, ok, guess that makes sense.



>@@ -2114,16 +2116,26 @@ var gDetailView = {
...
>+  onExternalInstall: function(aAddon, aExistingAddon, aNeedsRestart) {
>+    if (aExistingAddon.id != this._addon.id)
>+      return;

Since gDetailView registers itself as an install event listener (in addition to a addon event listener), it can get this event for any new install (not just upgrades). In which case, aExistingAddon can be null. So this will need to filter out the case where it isn't for an upgrade.
Attachment #478918 - Flags: review?(bmcbride) → review+
OS: Linux → All
Hardware: x86 → All
Whiteboard: [has patch][needs review rs][needs review Unfocused] → [has patch][needs review rs]
Attachment #478577 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [has patch][needs review rs] → [has patch]
Landed:
http://hg.mozilla.org/mozilla-central/rev/06cc2ca85b6c
http://hg.mozilla.org/mozilla-central/rev/e464e1369480
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b8
Backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch bustage fixes — — Splinter Review 
Some additional fixes required due to changes in the test harness and other code. This makes sure we ignore the Mochitest add-on that is now present.
Attachment #482697 - Flags: review?(bmcbride)
Comment on attachment 482697 [details] [diff] [review]
bustage fixes

Sorry this took so long :(
Attachment #482697 - Flags: review?(bmcbride) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/0dd1aab31305
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Hard to test manually. I trust the automated tests we have for this patch.
Marking as verified fixed based on no reported test failures.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: