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

VERIFIED FIXED in mozilla2.0b7

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: dietrich, Assigned: mossop)

Tracking

(Blocks: 1 bug)

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
requesting blocking: add-ons will very likely malfunction if their bootstrap functions are not called.
blocking2.0: --- → betaN+
Whiteboard: 596336
(Assignee)

Updated

7 years ago
Whiteboard: 596336
Lets add dependency for now. Bug 594490 is a bit vague while we have a clear issue here.
Blocks: 594490
(Assignee)

Comment 3

7 years ago
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
(Assignee)

Comment 4

7 years ago
Created attachment 478577 [details] [diff] [review]
backend patch rev 1

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)
(Assignee)

Comment 5

7 years ago
Created attachment 478578 [details] [diff] [review]
frontend patch rev 1

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)
(Assignee)

Updated

7 years ago
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
(Assignee)

Comment 7

7 years ago
Created attachment 478918 [details] [diff] [review]
frontend patch rev 2

(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)
(Assignee)

Updated

7 years ago
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]
(Assignee)

Comment 9

7 years ago
Landed:
http://hg.mozilla.org/mozilla-central/rev/06cc2ca85b6c
http://hg.mozilla.org/mozilla-central/rev/e464e1369480
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b8
(Assignee)

Comment 10

7 years ago
Backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

7 years ago
Relanded the backend part: http://hg.mozilla.org/mozilla-central/rev/83c1aa707d32
(Assignee)

Comment 12

7 years ago
Created attachment 482697 [details] [diff] [review]
bustage fixes

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+
(Assignee)

Comment 14

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/0dd1aab31305
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
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.