1387249 - Add-on will incorrectly wait for browser startup when installed at runtime

Status: VERIFIED FIXED

(Firefox :: New Tab Page, enhancement, P1)

Comment 1

[Dave Townsend [:mossop]]

Attachment: Bug 1387249: If the add-on is installed at runtime don't wait for browser startup notifications.

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

Comment 2

[[e-mail]]

as bug 1377470 comment 17, the system addon startup aReason after version update is `ADDON_INSTALL`, remove that will cause the whole checking process not work

Comment 3

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8893585 [details]
Bug 1387249: If the add-on is installed at runtime don't wait for browser startup notifications.

https://reviewboard.mozilla.org/r/164660/#review170244

As Gasolin addressed, when upgrading from earlier version it also triggers ADDON_INSTALL rather than APP_STARTUP.. we may need to find a way to separate that case and the really runtime-install case.

Comment 4

[Dave Townsend [:mossop]]

(In reply to KM Lee [:rexboy] from comment #3)
> Comment on attachment 8893585 [details]
> Bug 1387249: If the add-on is installed at runtime don't wait for browser
> startup notifications.
> 
> https://reviewboard.mozilla.org/r/164660/#review170244
> 
> As Gasolin addressed, when upgrading from earlier version it also triggers
> ADDON_INSTALL rather than APP_STARTUP.. we may need to find a way to
> separate that case and the really runtime-install case.

Yes, when upgrading from an earlier version at runtime we see the ADDON_INSTALL and must do initialisation immediately because waiting for startup methods won't happen. I'm not sure what you're trying to say.

Comment 5

[[e-mail]]

Comment on attachment 8893585 [details]
Bug 1387249: If the add-on is installed at runtime don't wait for browser startup notifications.

https://reviewboard.mozilla.org/r/164660/#review170340

::: browser/extensions/onboarding/bootstrap.js:199
(Diff revision 1)
>  
>  function uninstall(aData, aReason) {}
>  
>  function startup(aData, aReason) {
>    // Only start Onboarding when the browser UI is ready
> -  if (aReason === APP_STARTUP || aReason === ADDON_INSTALL) {
> +  if (aReason === APP_STARTUP) {

we should keep `aReason === ADDON_INSTALL` instead of `aReason === APP_STARTUP`

Comment 6

[Dave Townsend [:mossop]]

(In reply to Fred Lin [:gasolin] from comment #5)
> Comment on attachment 8893585 [details]
> Bug 1387249: If the add-on is installed at runtime don't wait for browser
> startup notifications.
> 
> https://reviewboard.mozilla.org/r/164660/#review170340
> 
> ::: browser/extensions/onboarding/bootstrap.js:199
> (Diff revision 1)
> >  
> >  function uninstall(aData, aReason) {}
> >  
> >  function startup(aData, aReason) {
> >    // Only start Onboarding when the browser UI is ready
> > -  if (aReason === APP_STARTUP || aReason === ADDON_INSTALL) {
> > +  if (aReason === APP_STARTUP) {
> 
> we should keep `aReason === ADDON_INSTALL` instead of `aReason ===
> APP_STARTUP`

That would mean no longer waiting for browser startup when the addon is loaded in early startup

Comment 7

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Dave Townsend [:mossop] from comment #4)
> (In reply to KM Lee [:rexboy] from comment #3)
> > Comment on attachment 8893585 [details]
> > Bug 1387249: If the add-on is installed at runtime don't wait for browser
> > startup notifications.
> > 
> > https://reviewboard.mozilla.org/r/164660/#review170244
> > 
> > As Gasolin addressed, when upgrading from earlier version it also triggers
> > ADDON_INSTALL rather than APP_STARTUP.. we may need to find a way to
> > separate that case and the really runtime-install case.
> 
> Yes, when upgrading from an earlier version at runtime we see the
> ADDON_INSTALL and must do initialisation immediately because waiting for
> startup methods won't happen. I'm not sure what you're trying to say.

Sorry for the late response. The situation we ran into was the ADDON_INSTALL sometimes DO emit before browser ready.

The scenario I can confirm is:
1. Install version 54 release and run a new profile.
2. then upgrade to the 57 nightly running the same profile.

And upon startup, it got events in the following sequence:
startup() with ADDON_INSTALL => BROWSER_READY_NOTIFICATION emitted => BROWSER_SESSION_STORE_NOTIFICATION emitted
So the condition there was to avoid that case.
It turns out that we may need a way to check the ready state of browser inside bootstrap.js..

Comment 8

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Not sure if this is a good idea: 
Use Services.wm.getMostRecentWindow("navigator:browser") to try if we can get a window (where it's not available upon startup or version update),
then check document.readyState === "complete". If it's already ready we call onBrowserReady() immediately.

Comment 9

[Dave Townsend [:mossop]]

(In reply to KM Lee [:rexboy] from comment #7)
> (In reply to Dave Townsend [:mossop] from comment #4)
> > (In reply to KM Lee [:rexboy] from comment #3)
> > > Comment on attachment 8893585 [details]
> > > Bug 1387249: If the add-on is installed at runtime don't wait for browser
> > > startup notifications.
> > > 
> > > https://reviewboard.mozilla.org/r/164660/#review170244
> > > 
> > > As Gasolin addressed, when upgrading from earlier version it also triggers
> > > ADDON_INSTALL rather than APP_STARTUP.. we may need to find a way to
> > > separate that case and the really runtime-install case.
> > 
> > Yes, when upgrading from an earlier version at runtime we see the
> > ADDON_INSTALL and must do initialisation immediately because waiting for
> > startup methods won't happen. I'm not sure what you're trying to say.
> 
> Sorry for the late response. The situation we ran into was the ADDON_INSTALL
> sometimes DO emit before browser ready.
> 
> The scenario I can confirm is:
> 1. Install version 54 release and run a new profile.
> 2. then upgrade to the 57 nightly running the same profile.
> 
> And upon startup, it got events in the following sequence:
> startup() with ADDON_INSTALL => BROWSER_READY_NOTIFICATION emitted =>
> BROWSER_SESSION_STORE_NOTIFICATION emitted
> So the condition there was to avoid that case.
> It turns out that we may need a way to check the ready state of browser
> inside bootstrap.js..

That's strange. Can you file an add-ons manager bug on that, it shouldn't happen.

(In reply to KM Lee [:rexboy] from comment #8)
> Not sure if this is a good idea: 
> Use Services.wm.getMostRecentWindow("navigator:browser") to try if we can
> get a window (where it's not available upon startup or version update),
> then check document.readyState === "complete". If it's already ready we call
> onBrowserReady() immediately.

This would fail on OSX where the app can be running without a window available.

How about nsIAppStartup.startingUp?

Comment 10

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Dave Townsend [:mossop] from comment #9)
> 
> That's strange. Can you file an add-ons manager bug on that, it shouldn't
> happen.
> 
I guess maybe that's not a bug because version 54 doesn't have onboarding installed, so the installation happens on the first run of updating to version 57.
If I update from version 55/56 to version 57, it just gets APP_STARTUP.

> (In reply to KM Lee [:rexboy] from comment #8)
> 
> This would fail on OSX where the app can be running without a window
> available.
> 
> How about nsIAppStartup.startingUp?
I'll have a try later by today. Thank you!

Comment 11

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to KM Lee [:rexboy] from comment #10)
> I guess maybe that's not a bug because version 54 doesn't have onboarding
> installed, so the installation happens on the first run of updating to
> version 57.
> If I update from version 55/56 to version 57, it just gets APP_STARTUP.
> 
Correction: version 55 to 57 gets APP_INSTALL. (Which is reasonable since Onboarding is not built into version 55)

Comment 12

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8893585 [details]
Bug 1387249: If the add-on is installed at runtime don't wait for browser startup notifications.

https://reviewboard.mozilla.org/r/164660/#review172314

::: browser/extensions/onboarding/bootstrap.js:199
(Diff revision 1)
>  
>  function uninstall(aData, aReason) {}
>  
>  function startup(aData, aReason) {
>    // Only start Onboarding when the browser UI is ready
> -  if (aReason === APP_STARTUP || aReason === ADDON_INSTALL) {
> +  if (aReason === APP_STARTUP) {

Just had a try with Services.startup.startingUp
It works good and get true upon `startup()`, so maybe just change the condition to
`if (Services.startup.startingUp)`

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8893585 [details]
Bug 1387249: If the add-on is installed at runtime don't wait for browser startup notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164660/diff/1-2/

Comment 14

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8893585 [details]
Bug 1387249: If the add-on is installed at runtime don't wait for browser startup notifications.

https://reviewboard.mozilla.org/r/164660/#review172588

Thank you!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7b68c29f533
If the add-on is installed at runtime don't wait for browser startup notifications. r=rexboy

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 17

[Justin [:JW_SoftvisionQA]]

How would I verify this issue?

Comment 18

[KM Lee [:rexboy] (inactivate after Sep. 22)]

I don't think this is a qa-verify+ issue but let me keep the ni? for a while before figuring out.

Comment 19

[[e-mail]]

I think it's QA verify-able and is appreciated,

The steps are:
1. download release, beta, older nightly (before ~8/12) and open with a new profile.
2. open the same profile with latest nightly.
3. All path should show update user tour instead of new user tour

Comment 20

[Justin [:JW_SoftvisionQA]]

I can verify this issue is fixed based on comment 19.

Comment 21

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed, I verified using Fx 57.0b7, on Windows 10 x64, Ubuntu 14.04 LTS x 64 and mac OS X 10.12.6.