1412050 - [dt-onboarding] restoreSession triggers the onboarding flow if devtools.enabled is false

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Julian Descottes [:jdescottes]]

STRs:
- set devtools.enabled to false
- restart browser
- go to History > Restore Previous Session

=> onboarding page is displayed

The initial "restoreScratchpadSessions" entry point in the devtools shim was normally only called if there were scratchpad sessions to restore (in which case you could assume devtools.enabled was true).

However since Bug 1388552, restore session now always calls the DevToolsShim restoreDevToolsSession entry point, whether there's DevTools content to restore or not.

This means the DevTools shim should be aware of devtools.enabled and should only try to initialize DevTools when restoreDevToolsSession is called _if_ devtools.enabled is true.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1412050 - avoid devtools onboarding flow when restoring session;

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

Comment 2

[Alexandre Poirot [:ochameau]]

Ah. It looks like bug 1388552 regressed lazy startup of DevTools:
http://searchfox.org/mozilla-central/diff/4902b5d260bdd9916c56ef48e6817e67086c0a12/browser/components/sessionstore/SessionStore.jsm#2772

The issue remains with this patch. We call initDevTools when restoring the session.
This would fix the issue when the pref is false, but not the default when it is true.

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #2)
> Ah. It looks like bug 1388552 regressed lazy startup of DevTools:
> http://searchfox.org/mozilla-central/diff/
> 4902b5d260bdd9916c56ef48e6817e67086c0a12/browser/components/sessionstore/
> SessionStore.jsm#2772
> 
> The issue remains with this patch. We call initDevTools when restoring the
> session.
> This would fix the issue when the pref is false, but not the default when it
> is true.

It regressed the lazy startup when we are restoring sessions, not in the general case.

Restore sessions is only called:
- on demand (History -> Restore Previous Session)
- when restarting Firefox after a crash right
- when restarting Firefox if browser.startup.page is set to 3 (Preferences -> General -> When Firefox starts: Show your windows and tabs from Last time)

If we want to fix it we need to check somewhere that the session contains DevTools-related data. I would put this back in SessionStore.jsm and heavily comment it, so that if we add more devtools data to the session later, we don't regress this. 

I would like to keep the new version of the shim though, as it gets rid of the maybeInitializeDevTools. I wanted to avoid having to check for devtools.enabled in the shim, but in the end I think it makes some code paths clearer.

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8922444 [details]
Bug 1412050 - avoid devtools onboarding flow when restoring session;

https://reviewboard.mozilla.org/r/193490/#review198926

Looks good, just have one comment about... a comment!

::: devtools/shim/DevToolsShim.jsm:203
(Diff revision 1)
>  
>    /**
> -   * Should be called if a shim method attempts to initialize devtools.
> -   * - if DevTools are already initialized, returns true.
> -   * - if DevTools are not initialized, call initDevTools from devtools-startup:
> -   *   - if devtools.enabled is true, DevTools will synchronously initialize and the
> +   * Initialize DevTools via DevToolsStartup if needed. Should not be called if
> +   * DevTools are not enabled, because the initialization would trigger the onboarding
> +   * flow. If the entry point is supposed to trigger the onboarding, call it explicitly
> +   * via DevtoolsStartup.openInstallPage().

"because the initialization would trigger the onboarding" isn't correct. Here you throw.

What about this:
Initialize DevTools via DevToolsStartup if needed. This method throws if DevTools are not enabled.
If the entry point is supposed to trigger the onboarding, call it explicitly via DevtoolsStartup.openInstallPage()

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #3)
> (In reply to Alexandre Poirot [:ochameau] from comment #2)
> > Ah. It looks like bug 1388552 regressed lazy startup of DevTools:
> > http://searchfox.org/mozilla-central/diff/
> > 4902b5d260bdd9916c56ef48e6817e67086c0a12/browser/components/sessionstore/
> > SessionStore.jsm#2772
> > 
> > The issue remains with this patch. We call initDevTools when restoring the
> > session.
> > This would fix the issue when the pref is false, but not the default when it
> > is true.
> 
> It regressed the lazy startup when we are restoring sessions, not in the
> general case.

Still quite bad. I don't consider session restore as an edge case.

> If we want to fix it we need to check somewhere that the session contains
> DevTools-related data. I would put this back in SessionStore.jsm and heavily
> comment it, so that if we add more devtools data to the session later, we
> don't regress this. 

May be DevToolsShims.restoreDevToolsSession makes more sense as this is DevTools specifics.
We originaly put it in SessionStore.jsm to save from loading DevToolsShim.jsm.
Is DevToolsShim.jsm still not loaded during Firefox startup anyway?

> I would like to keep the new version of the shim though, as it gets rid of
> the maybeInitializeDevTools. I wanted to avoid having to check for
> devtools.enabled in the shim, but in the end I think it makes some code
> paths clearer.

r+ I had that same feeling during the review of maybeInitializeDevTools.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1412050 - session restore should not initialize devtools when unnecessary;

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

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8922444 [details]
Bug 1412050 - avoid devtools onboarding flow when restoring session;

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

Comment 8

[Julian Descottes [:jdescottes]]

Thanks for the review! Fixed the comment.
Added a second changeset to stop initializing devtools during session restore if unnecessary.

I updated the unit test to verify that, but I think we should add an integration test that checks that DevTools are not initialized after Firefox starts, under various conditions. I feel like the unit test gets updated whenever we update the implementation and doesn't really provide a good non-regression layer. It's great to catch implementation issues, but not much more.

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> (In reply to Julian Descottes [:jdescottes] from comment #3)
> > (In reply to Alexandre Poirot [:ochameau] from comment #2)
> > > Ah. It looks like bug 1388552 regressed lazy startup of DevTools:
> > > http://searchfox.org/mozilla-central/diff/
> > > 4902b5d260bdd9916c56ef48e6817e67086c0a12/browser/components/sessionstore/
> > > SessionStore.jsm#2772
> > > 
> > > The issue remains with this patch. We call initDevTools when restoring the
> > > session.
> > > This would fix the issue when the pref is false, but not the default when it
> > > is true.
> > 
> > It regressed the lazy startup when we are restoring sessions, not in the
> > general case.
> 
> Still quite bad. I don't consider session restore as an edge case.
> 
> > If we want to fix it we need to check somewhere that the session contains
> > DevTools-related data. I would put this back in SessionStore.jsm and heavily
> > comment it, so that if we add more devtools data to the session later, we
> > don't regress this. 
> 
> May be DevToolsShims.restoreDevToolsSession makes more sense as this is
> DevTools specifics.

I thought SessionStore was still pulling data from devtools (opened scratchpads, browser state etc...) to update its state object, but now it's delegating all of that to devtoolshism/gdevtools. Which is probably better anyway (avoids leaking devtools specifics).

So I agree, I put the logic in DevToolsShim now.

> We originaly put it in SessionStore.jsm to save from loading
> DevToolsShim.jsm.
> Is DevToolsShim.jsm still not loaded during Firefox startup anyway?

DevToolsShim is actually loaded first by SessionStore.jsm. Either during the restore (which is a regression), or during a state save, which always happens after a small delay following the startup. If it was not for SessionStore, DevToolsShim wouldn't be loaded at all. Keeping in mind that it's a small jsm, without sync dependencies.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8922705 [details]
Bug 1412050 - session restore should not initialize devtools when unnecessary;

https://reviewboard.mozilla.org/r/193862/#review199252

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a32bdc2c0138
avoid devtools onboarding flow when restoring session;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c11eb11eddec
session restore should not initialize devtools when unnecessary;r=bgrins

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a32bdc2c0138
https://hg.mozilla.org/mozilla-central/rev/c11eb11eddec

Comment 12

[Ionuț Goldan [:igoldan]]

Improvements noticed on these perf tests:

== Change summary for alert #10248 (as of October 29 2017 09:01 UTC) ==

Improvements:

  2%  sessionrestore_many_windows linux64 pgo e10s     1,581.79 -> 1,545.75
  2%  sessionrestore_many_windows windows10-64 opt e10s2,907.54 -> 2,841.83
  2%  sessionrestore_many_windows linux64 opt e10s     1,798.08 -> 1,758.42
  2%  sessionrestore_many_windows windows7-32 opt e10s 3,015.33 -> 2,949.50
  2%  sessionrestore_many_windows windows7-32 pgo e10s 3,557.75 -> 3,488.33
  1%  sessionrestore_many_windows windows10-64 pgo e10s3,528.04 -> 3,476.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10248

Comment 13

[Julian Descottes [:jdescottes]]

Thanks for the perf update. 
Makes sense as we no longer initialize DevTools in the scenarios tested here.