Closed Bug 1304392 Opened 3 years ago Closed 3 years ago

Startup() with reason ADDON_INSTALL never fired when e10sBlockedByAddons triggered

Categories

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

49 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla52
Iteration:
52.2 - Oct 17
Tracking Status
firefox49 --- wontfix
firefox50 + fixed
firefox51 + fixed
firefox52 --- verified

People

(Reporter: Off.Just.Off, Assigned: aswan)

Details

(Whiteboard: triaged)

Attachments

(1 file)

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.
Component: Untriaged → Extension Compatibility
OS: Unspecified → All
Hardware: Unspecified → All
Component: Extension Compatibility → General
Product: Firefox → Add-on SDK
Version: 49 Branch → unspecified
Component: General → Add-ons Manager
Product: Add-on SDK → Toolkit
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.
Version: unspecified → 49 Branch
(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.
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.
Flags: needinfo?(Off.Just.Off)
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?
Flags: needinfo?(Off.Just.Off)
> 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"
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.
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.
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
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.
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?
Flags: needinfo?(rhelmer)
Flags: needinfo?(mozilla)
We'll still be broken on 50 with any SDK extensions that aren't multi process compatible won't we?
Flags: needinfo?(mozilla)
Assignee: nobody → aswan
Priority: -- → P2
Whiteboard: triaged
Flags: needinfo?(rhelmer)
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?
Flags: needinfo?(mozilla)
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?
Flags: needinfo?(mozilla)
(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.
Status: UNCONFIRMED → ASSIGNED
Iteration: --- → 52.2 - Oct 17
Ever confirmed: true
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
Attachment #8801199 - Flags: review?(rhelmer) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/628c25b4c363
Fix startup reason for bootstrapped addons that require restart for e10s r=rhelmer
[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.
https://hg.mozilla.org/mozilla-central/rev/628c25b4c363
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi Andrew, could you please nominate a patch for uplift to Beta50, Aurora51?
Flags: needinfo?(aswan)
Hello there, could you please verify this issue is fixed as expected on a latest Nightly build (10-18)? Thanks!
Flags: needinfo?(Off.Just.Off)
As far as I see, latest Nightly now is 10-17, I'll try to check tomorrow
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
Flags: needinfo?(aswan)
Attachment #8801199 - Flags: approval-mozilla-beta?
Attachment #8801199 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(Off.Just.Off)
JustOff, that's expected in stock Nightly.  Can you set the preference "extensions.e10sBlocksEnabling" to true and try again?
Flags: needinfo?(Off.Just.Off)
Andrew Swan, now all works as expected!
Flags: needinfo?(Off.Just.Off)
Great, thanks for the quick turnaround!
Status: RESOLVED → VERIFIED
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+
Attachment #8801199 - Flags: approval-mozilla-beta?
Attachment #8801199 - Flags: approval-mozilla-beta+
Attachment #8801199 - Flags: approval-mozilla-aurora?
Attachment #8801199 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.