1304392 - Startup() with reason ADDON_INSTALL never fired when e10sBlockedByAddons triggered

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[JustOff]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160910154927

Steps to reproduce:

Install a restartless extension, not marked as e10s compatible, on clean FF profile.
For example - https://addons.mozilla.org/addon/disable-hello-pocket-reader/
As it's the first non e10s extension, browser restart is required to complete.


Actual results:

This way, startup() with reason ADDON_INSTALL is never fired for the extension.


Expected results:

Startup() with reason ADDON_INSTALL should be fired once, regardless of installation path.

Comment 1

[JustOff]

As Loic for some reason have removed "49 branch" from the description, I need to point that problem arises starting from FF 49, the previous one was fine.

Comment 2

[JustOff]

(In reply to JustOff from comment #0)
> For example - https://addons.mozilla.org/addon/disable-hello-pocket-reader/

Since the extension, listed as an example in this bug report, today has got workaround fix, it's necessary to use previous version (https://addons.mozilla.org/addon/disable-hello-pocket-reader/versions/0.4.5) to reproduce the problem.

Beta debug version (https://addons.mozilla.org/addon/disable-hello-pocket-reader/versions/0.4.6b3) can be used as well, as it reports to console the reason value from startup() and install() on each call.

Comment 3

[Andrew Swan [:aswan]]

JustOff, can you confirm that you do see a call to startup() with the reason APP_STARTUP?
Assuming you are, this is working as expected -- the current e10s rollout has the effect of making addons that are not e10s- compatible also non-restartless and the expected sequence for them is to get a call to install(), followed by a regular APP_STARTUP call to startup() when the browser restarts.

Comment 4

[JustOff]

Thanks for the clarification, but the resulting sequence still looks at least strange. And the saddest thing is that such behaviour is completely undocumented, even in corresponding addon compatibility blog post. That's why this bug was raised.

Btw, although the last version of addon mentioned in this bug is now e10s compatible, the the browser (FF49) is still asks for reboot to enable it on clean profile. Did I miss something else?

Comment 5

[Mike Kaply [:mkaply]]

> the expected sequence for them is to get a call to install(), followed by a regular APP_STARTUP call to startup() when the browser restarts.

That sequence is something that no restartless addon (including the addon SDK) would ever expect. It was technically impossible until now.

This appears to break SDK add-ons because they don't handle install, they only handle startup expecting it to say "install"

Comment 6

[Mike Kaply [:mkaply]]

So the exact problem is that in the non e10s case, the calls are:

install ADDON_INSTALL, startup ADDON_INSTALL

in the e10s restart case, we get:

install ADDON_INSTALL, startup - APP_STARTUP

No code is executed before the restart.

As I said, this breaks any add-on SDK addons that try to detect install.

Comment 7

[Mike Kaply [:mkaply]]

This seems like the likely culprit:

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2857

Comment 8

[Andrew Swan [:aswan]]

From inspection, it looks like adding a call to addStartupChange() to the "if (bootstrap)" clause here should change the startup reason to INSTALL:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProviderUtils.js#2090-2100
It seems safe to me, but I could use a sanity check that I'm not overlooking some subtlety here.

Comment 9

[Dave Townsend [:mossop]]

(In reply to Andrew Swan [:aswan] from comment #8)
> From inspection, it looks like adding a call to addStartupChange() to the
> "if (bootstrap)" clause here should change the startup reason to INSTALL:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProviderUtils.js#2090-2100
> It seems safe to me, but I could use a sanity check that I'm not overlooking
> some subtlety here.

STARTUP_CHANGE_INSTALLED was meant to be a flag to say that an add-on had been detected at startup, for API users effectively, "you won't have seen an onInstalling" event for this add-on. Setting it there would change the semantics there a little and if you choose to do that I'd probably suggest doing it regardless of bootstrapping for consistency. I don't see any of our code caring about that change so it's only if you're worried about breaking add-ons that there would be a problem.

Comment 10

[Mike Kaply [:mkaply]]

FYI, the workaround I gave to our customer for an SDK addon was to override install in bootstrap.js and set a preference and then read that preference exports.main to see if it was really first install.

Comment 11

[Andrew Swan [:aswan]]

So now I'm on the fence about making any changes.  The e10srollout schedule calls for multiprocess-compatible extensions to go back to being restartless in 50 and from what I understand we're still on track for that.  Frankly the biggest impact is in 49 where we're unlikely to make any major changes, and in light of Mossop's comments above I think that making a change in 49 is completely out of the question.
So that means that all we can really do is help non-MPC extensions in 50+, by making a change of uncertain riskiness.  It isn't a great solution but weighing all of that, I lean toward documenting the behavior including the simple workaround that Mike describes in comment 10.
Any objections?

Comment 12

[Mike Kaply [:mkaply]]

We'll still be broken on 50 with any SDK extensions that aren't multi process compatible won't we?

Comment 13

[Andrew Swan [:aswan]]

In the process of writing the test for this fix, I realized there's a corollary case here: if an extension is installed, then manually disabled, and then re-enabled, when the last enable requires a restart, then the startup reason is APP_STARTUP and not ADDON_ENABLE.  Off the top of my head, fixing that sounds a lot trickier than the issue described here.
Is leaving that case unhandled going to be a problem?

Comment 14

[Mike Kaply [:mkaply]]

When the extension is manually disabled, do we automatically switch back to e10s mode?

And then the reenable switches to non e10s again?

Maybe we need to stop trying so hard and once a user has installed an extension (and still has it installed, even if it is disabled) we don't do e10s?

Comment 15

[Andrew Swan [:aswan]]

(In reply to Mike Kaply [:mkaply] from comment #14)
> When the extension is manually disabled, do we automatically switch back to
> e10s mode?

We disable immediately without *requiring* a restart, but if the browser is restarted for some other reason and there are no blocking extensions enabled, then e10s gets turned on.

> And then the reenable switches to non e10s again?

right.

> Maybe we need to stop trying so hard and once a user has installed an
> extension (and still has it installed, even if it is disabled) we don't do
> e10s?

That sounds lousy, I tend to leave a bunch of disabled extensions in my profile.  Maybe I'm an outlier though.
I suspect this was a deliberate decision and I'm loathe to change it just for the sake of this bug.

I'm going to put up a patch that addresses install but not enable, if the enable scenario is a blocker then we can revisit this.

Comment 16

[Andrew Swan [:aswan]]

Attachment: Bug 1304392 Fix startup reason for bootstrapped addons that require restart for e10s

Review commit: https://reviewboard.mozilla.org/r/85934/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85934/

Comment 17

[Robert Helmer [:rhelmer]]

Comment on attachment 8801199 [details]
Bug 1304392 Fix startup reason for bootstrapped addons that require restart for e10s

https://reviewboard.mozilla.org/r/85934/#review84722

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/628c25b4c363
Fix startup reason for bootstrapped addons that require restart for e10s r=rhelmer

Comment 19

[Andrew Swan [:aswan]]

[Tracking Requested - why for this release]:
This bug affects bootstrapped extensions that previously worked fine but now fail to install correctly due to an interaction with the e10s rollout strategy and the fact that the browser must be restarted to enable/disable e10s.  The fix is small and low-risk and will save a lot of headaches for extension developers and users.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/628c25b4c363

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Andrew, could you please nominate a patch for uplift to Beta50, Aurora51?

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello there, could you please verify this issue is fixed as expected on a latest Nightly build (10-18)? Thanks!

Comment 23

[JustOff]

As far as I see, latest Nightly now is 10-17, I'll try to check tomorrow

Comment 24

[Andrew Swan [:aswan]]

Comment on attachment 8801199 [details]
Bug 1304392 Fix startup reason for bootstrapped addons that require restart for e10s

Approval Request Comment
[Feature/regressing bug #]:
e10srollout

[User impact if declined]:
Non-multiprocess-compatible bootstrapped addons are likely to fail to install properly if installing them forces a restart (to disable e10s)

[Describe test coverage new/current, TreeHerder]:
A unit test is included in the patch

[Risks and why]: 
The issue fixed in this patch was an unintended consequence of the e10srollout scheme.  A fair amount of code within the add-ons manager relies on the assumption that bootstrapped add-ons can always be installed without restarting the browser, including the code that establishes the "reason" parameter to the bootstrap startup() method.  That assumption (restartless install) breaks with e10srollout which in turn breaks add-ons that rely on having a call to startup() with reason set to ADDON_INSTALL to initialize themselves properly.  The risk is low, the affected code path is straightforward and the surrounding add-ons manager code has an extensive test suite to catch any unintended regressions from this patch.

[String/UUID change made/needed]:
none

Comment 25

[JustOff]

Ritu Kothari, I've tried Nightly 10-18 and it don't asks for restart at all when installing non e10s compatible addon. Is it expected behaviour?

Comment 26

[Andrew Swan [:aswan]]

JustOff, that's expected in stock Nightly.  Can you set the preference "extensions.e10sBlocksEnabling" to true and try again?

Comment 27

[JustOff]

Andrew Swan, now all works as expected!

Comment 28

[Andrew Swan [:aswan]]

Great, thanks for the quick turnaround!

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8801199 [details]
Bug 1304392 Fix startup reason for bootstrapped addons that require restart for e10s

Fix was verified on Nightly, Aurora51+, Beta50+

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8b1f7bfc29e

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/899446883f1a