1098357 - talos session restore [no auto restore] is broken in e10s mode

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

at one point in time this worked, but it has been broken for a few weeks.  What has changed?  I am not sure, but what I see is this:

sessionrestore talos fails to init in automation + locally (linux64).  I see 2 tabs show up locally:
1) session restore tab
2) the init tab that closes

the second tab closes and leaves the first tab around, so we hang and timeout.

I wanted to look at this in more detail, so I remove sessionstore.js from the profile for the initialization routine.  I found that without e10s we would load the browser and all tabs, then immediately call finish:
http://hg.mozilla.org/build/talos/file/2bdbc49134c4/talos/startup_test/sessionrestore/main.js#l37

but with e10s, we would load all the tabs, but end up adding an observer for "sessionstore-windows-restored" which never gets called:
http://hg.mozilla.org/build/talos/file/2bdbc49134c4/talos/startup_test/sessionrestore/main.js#l46

So the question could be:
1) are we not populating startup_info with sessionRestored and|or sessionRestoreInit?
2) do we ever fire the "sessionstore-windows-restored" observer event in e10s?
3) maybe the talos code is broken in the observer scenario

Comment 1

[Chris Peterson [:cpeterson]]

e10s test issue.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Yoric, can you weigh in here, I would like to see some traction on this when we all have time.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

I'm swamped at the moment. I won't have time to look at this before January, I'm afraid.
Tim, by any chance, do you have some time?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Sorry, don't have much time to look at this right now. Hope we can put this into the backlog.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

:ttaubert - checking into this bug again- this is the only talos test which doesn't run on e10s.  It would be nice to get that done in Q1.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Just look into it. The test profile doesn't have a sessionCheckpoints.json file, thus sessionstore thinks the previous session crashed. Adding one with the following content fixes only part of the problem:

{"profile-after-change":true,"final-ui-startup":true,"sessionstore-windows-restored":true,"quit-application-granted":true,"quit-application":true,"sessionstore-final-state-write-complete":true,"profile-change-net-teardown":true,"profile-change-teardown":true,"profile-before-change":true}

Now we seem to restore the session properly but for some reason the talos test tab doesn't show up? How exactly does that mechanism work?

Joel, are you maybe able to figure out what's wrong with a valid sessionCheckpoints.json in the profile? If I quit the first run that doesn't do anything then the 2nd run seems to open the talos tab, although it doesn't do the measurement.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

adding a sessionCheckpoint.js into the mix, I get the same results as I do before which is the end result of what you have reported Tim.

what is odd is that on first launch where we 'initialize the profile', the browser never quits.  So that is one thing which we need to figure out.  In non e10s land we fire up the browser, it gathers metrics, then it closes.  In e10s mode, it never closes, although the tab we are in does close.

In e10s mode, if I do remove sessionrestore.js from the profile, it closes as expected.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

(In reply to Joel Maher (:jmaher) from comment #7)
> what is odd is that on first launch where we 'initialize the profile', the
> browser never quits.  So that is one thing which we need to figure out.  In
> non e10s land we fire up the browser, it gathers metrics, then it closes. 
> In e10s mode, it never closes, although the tab we are in does close.

Is that specific to this test?

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

yes, when we worked on this last, sessionrestore* was the only test broken by e10s.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

The issue is most likely that we open index.html in the content process, where it won't ever receive "sessionstore-windows-restored".

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1098357 - Making Session Restore talos test e10s-friendly;r?mconley

Bug 1098357 - Making Session Restore talos test e10s-friendly;r?mconley

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8676773 [details]
MozReview Request: Bug 1098357 - Making Session Restore talos test e10s-friendly;r?mconley

https://reviewboard.mozilla.org/r/22283/#review20295

looks good to me.  I assume this passes on try server for all OS's?  to do that remove lines 300-307:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/config.py#302

then push to try :)

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8676773 [details]
MozReview Request: Bug 1098357 - Making Session Restore talos test e10s-friendly;r?mconley

https://reviewboard.mozilla.org/r/22283/#review20323

This looks good to me, assuming a green try run.

Thanks Yoric!

::: testing/talos/talos/startup_test/sessionrestore/addon/chrome.manifest:4
(Diff revision 1)
> +component {716346e5-0c45-4aa2-b601-da36f3c74bd8} nsSessionRestoreTalosTest.js

I don't think we're supposed to create ns prefixed things anymore. Might as well just call this thing SessionRestoreTalosTest.js

::: testing/talos/talos/startup_test/sessionrestore/addon/nsSessionRestoreTalosTest.js:12
(Diff revision 1)
> +const Cr = Components.results;

Unused.

::: testing/talos/talos/startup_test/sessionrestore/addon/nsSessionRestoreTalosTest.js:76
(Diff revision 1)
> +      var startup_info = Services.startup.getStartupInfo();
> +      var duration = startup_info.sessionRestored - startup_info.sessionRestoreInit;

let, not var

::: testing/talos/talos/startup_test/sessionrestore/main.js:42
(Diff revision 1)
> -main();
> +// Im case the add-on has broadcasted the message before we were loaded,

typo: "Im" -> "In"

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8676773 [details]
MozReview Request: Bug 1098357 - Making Session Restore talos test e10s-friendly;r?mconley

Bug 1098357 - Making Session Restore talos test e10s-friendly;r?mconley

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Applied feedback.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a7dcc2bc843

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b08adc2a4a92

Comment 17

[Carsten Book [:Tomcat]]

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