Closed Bug 1387249 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 --- verified

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(1 file)

No description provided.
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
Priority: -- → P1
Target Milestone: --- → Firefox 57
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.
Attachment #8893585 - Flags: review?(rexboy)
(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 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`
(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
(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..
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.
(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?
Status: NEW → ASSIGNED
(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!
(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 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 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!
Attachment #8893585 - Flags: review?(rexboy) → review+
Pushed by dtownsend@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/a7b68c29f533
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
How would I verify this issue?
Flags: needinfo?(rexboy)
I don't think this is a qa-verify+ issue but let me keep the ni? for a while before figuring out.
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
Flags: needinfo?(rexboy)
I can verify this issue is fixed based on comment 19.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: jwilliams
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.