Closed Bug 1098357 Opened 5 years ago Closed 4 years ago

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

Categories

(Testing :: Talos, defect)

defect
Not set

Tracking

(e10s+, firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
e10s + ---
firefox44 --- fixed

People

(Reporter: jmaher, Assigned: Yoric)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
e10s test issue.
Blocks: e10s-tests
tracking-e10s: --- → +
Yoric, can you weigh in here, I would like to see some traction on this when we all have time.
Flags: needinfo?(dteller)
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?
Flags: needinfo?(dteller) → needinfo?(ttaubert)
Sorry, don't have much time to look at this right now. Hope we can put this into the backlog.
Flags: needinfo?(ttaubert) → firefox-backlog+
: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.
Flags: needinfo?(ttaubert)
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.
Flags: needinfo?(ttaubert) → needinfo?(jmaher)
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.
Flags: needinfo?(jmaher)
Blocks: e10s-rc
Assignee: nobody → dteller
(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?
Flags: needinfo?(jmaher)
yes, when we worked on this last, sessionrestore* was the only test broken by e10s.
Flags: needinfo?(jmaher)
The issue is most likely that we open index.html in the content process, where it won't ever receive "sessionstore-windows-restored".
Bug 1098357 - Making Session Restore talos test e10s-friendly;r?mconley
Attachment #8676773 - Flags: review?(mconley)
Attachment #8676773 - Flags: feedback?(jmaher)
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 :)
Attachment #8676773 - Flags: review+
Attachment #8676773 - Flags: review?(mconley) → review+
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"
Attachment #8676773 - Flags: feedback?(jmaher) → feedback+
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
Attachment #8676773 - Flags: feedback+
https://hg.mozilla.org/mozilla-central/rev/b08adc2a4a92
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.