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

RESOLVED FIXED in Firefox 44

Status

Testing
Talos
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jmaher, Assigned: Yoric)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(e10s+, firefox44 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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: 984139
tracking-e10s: --- → +
(Reporter)

Comment 2

3 years ago
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+
(Reporter)

Comment 5

3 years ago
: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)
(Reporter)

Comment 7

3 years ago
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)
(Reporter)

Updated

2 years ago
Blocks: 1130445

Updated

2 years ago
Blocks: 1144120

Updated

2 years ago
Blocks: 1198187
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)
(Reporter)

Comment 9

2 years ago
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".
Created 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: review?(mconley)
Attachment #8676773 - Flags: feedback?(jmaher)
(Reporter)

Comment 12

2 years ago
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+

Updated

2 years ago
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"
(Reporter)

Updated

2 years ago
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+
Applied feedback.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a7dcc2bc843
Keywords: checkin-needed

Comment 16

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b08adc2a4a92
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b08adc2a4a92
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1222849
You need to log in before you can comment on or make changes to this bug.