854937 - reason is 1/STARTUP for addons preinstalled in profile/extensions folder

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Alexandre Poirot [:ochameau]]

Attachment: Sample profile

Currently, when putting an uncompressed bootstrap addon to profile's extensions folder, we receive a call to install() with reason=5/INSTALL, but then gets startup() being called with reason=1/STARTUP.

It ends up introducing some weirdness with jetpack as we are dropping addon into profile extensions folder (by using mozrunner) and we are expecting to have INSTALL reason during the first run of firefox on such profile. (Some module reacts differently based on startup reason)

I submitted a sample profile folder that print reason code to the JS error console.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Tentative fix

I tried to fix that without much success.
Here is a tentative, but it ends up always setting startup call reason to 5/INSTALL.

The call to install method is being done here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#2819
So I also tried to add a property on this `newAddon` object, but it seems to end up being a different object than `this.bootstrappedAddons[id]` where we call the startup method from here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#1674

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 729601 [details] [diff] [review]
Tentative fix

Review of attachment 729601 [details] [diff] [review]:
-----------------------------------------------------------------

This is a nice solution, almost there I think.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +1674,5 @@
> +      let reason = BOOTSTRAP_REASONS.APP_STARTUP;
> +      // Eventually set INSTALLED reason when a bootstrap addon
> +      // is dropped in profile folder and automatically installed
> +      if (AddonManager.getStartupChanges(AddonManager.STARTUP_CHANGE_INSTALLED))
> +        reason = BOOTSTRAP_REASONS.ADDON_INSTALL;

AddonManager.getStartupChanges returns an array of IDs that are in that state, so you want to use indexOf to check if this ID was just installed.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 854937: Fix reason code for bootstrap addon dropped in profile extensions folder.

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Dave Townsend (:Mossop) from comment #2)
> AddonManager.getStartupChanges returns an array of IDs that are in that
> state, so you want to use indexOf to check if this ID was just installed.

Oh I mean to also copy and paste the indexOf, but looks like I missed it!

So do you think that it is a reasonable patch? TBH I don't really know what is getStartupChanges, but various usages in this file was looking like a way to tackle this kind of issues...

Comment 5

[Dave Townsend [:mossop]]

(In reply to Alexandre Poirot (:ochameau) from comment #4)
> (In reply to Dave Townsend (:Mossop) from comment #2)
> > AddonManager.getStartupChanges returns an array of IDs that are in that
> > state, so you want to use indexOf to check if this ID was just installed.
> 
> Oh I mean to also copy and paste the indexOf, but looks like I missed it!
> 
> So do you think that it is a reasonable patch? TBH I don't really know what
> is getStartupChanges, but various usages in this file was looking like a way
> to tackle this kind of issues...

Yeah, it was made to track what changes had been detected during startup so it's really perfect for this, it just didn't exist when I first wrote the bootstrap code and when I added it I didn't think to use it to fix this.

Now I don't understand why it isn't working though :s

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 854937: Fix reason code for bootstrap addon dropped in profile extensions folder.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 729658 [details] [diff] [review]
Bug 854937: Fix reason code for bootstrap addon dropped in profile extensions folder.

$ make xpcshell-tests
INFO | Passed: 274
INFO | Failed: 0

Looks like we are covered by test 8:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js#436

Comment 8

[Dave Townsend [:mossop]]

Does this fix bug 852623?

Comment 9

[Alexandre Poirot [:ochameau]]

(In reply to Dave Townsend (:Mossop) from comment #8)
> Does this fix bug 852623?

Yes, but it will also regress jetpack. We need a fix to avoid throwing on cfx run in addon/runner code.
it throws as it doesn't wait for session-store even and later tries to acccess the hidden dom window which isn't ready yet:
https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/addon/runner.js#L73-L81

May be I should use bug 852623 to submit a sdk patch to prevent this regression?

Comment 10

[Dave Townsend [:mossop]]

Hmm that seems likely to be a common problem across add-ons making me wonder whether this is the right fix at all :s

Comment 11

[Alexandre Poirot [:ochameau]]

Existing addons may or may not work well when we install them during startup (with/without this patch), but I think it will be quite rare to see the same pattern than jetpack, where we conditionally wait for browser readyness depending on reason code...

Here we do not change startup/install call, but only change the startup call reason argument when we execute a pre-installed addon.

We also have to mitigate for cases where it is going to fix issues:
https://mxr.mozilla.org/addons/source/10122/bootstrap.js#32
And one similar to widget issue:
https://mxr.mozilla.org/addons/source/4346/bootstrap.js#103
https://mxr.mozilla.org/addons/source/4346/includes/buttons.js

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 729658 [details] [diff] [review]
Bug 854937: Fix reason code for bootstrap addon dropped in profile extensions folder.

(In reply to Alexandre Poirot (:ochameau) from comment #11)
> Existing addons may or may not work well when we install them during startup
> (with/without this patch), but I think it will be quite rare to see the same
> pattern than jetpack, where we conditionally wait for browser readyness
> depending on reason code...
> 
> Here we do not change startup/install call, but only change the startup call
> reason argument when we execute a pre-installed addon.
> 
> We also have to mitigate for cases where it is going to fix issues:
> https://mxr.mozilla.org/addons/source/10122/bootstrap.js#32
> And one similar to widget issue:
> https://mxr.mozilla.org/addons/source/4346/bootstrap.js#103
> https://mxr.mozilla.org/addons/source/4346/includes/buttons.js

I think in both those cases this change would break the add-on if it were pre-installed. Now granted that may be rare so that might be a saving grace. Still I'd like Blair's opinion here.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, from what I've seen its still a rare occurrence - most extensions installed like this are still non-restartless (ignoring extensions manually installed by powerusers/developers). And although an even rarer edge-case, extensions that are affected by this are likely to be already broken on OSX when they're installed when no windows are open. 

So at least if we fix this now, we'll be fixing it before it becomes a common issue. And I'd rather have this fixed, and avoid it potentially becoming a bug we have to keep and work around in the future.

Oh, and: elegant fix, nice work!

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Jorge: This could potentially break some restartless add-ons installed by a 3rd party (foreign installs), so might be something we want to monitor. If anything is affected, it's likely to only break the first time Firefox runs after such an add-on is installed - they'll likely work fine after Firefox restarts.

Comment 15

[Jorge Villalobos [:jorgev] (he/him)]

It's unlikely that this is causing real problems in the wild, since all third-party installs I've seen so far are old-style add-ons. We'll keep an eye out, though.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/f03cd121303c

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f03cd121303c