The sessionrestore_no_auto_restore test is supposed to measure how long it takes to restore the browser session when Firefox is configured to go to about:home immediately. The current configuration of sessionrestore_no_auto_restore has the test loading the sessionrestore test page instead. This means that we, for example, end up painting the tab throbber for this test, which adds noise and is not a real-world scenario. We should modify the test to open the test page once the measurement has completed.

:Yoric, you had originally written the sessionrestore tests, can you comment on if this makes sense?

Yes, that makes sense.

Comment on attachment 8876824 [details] Bug 1372292 - Load sessionrestore Talos test result page in a tab after the measurement is done. https://reviewboard.mozilla.org/r/148144/#review153454 ::: testing/talos/talos/startup_test/sessionrestore/addon/SessionRestoreTalosTest.js:91 (Diff revision 1) > if (hasRestoredTabs) { > Services.obs.removeObserver(this, StartupPerformance.RESTORED_TOPIC); > } > + > + // onReady might fire before the browser window has finished initializing > + // or sometimes soon after. We handle both cases here. Mmmh... How does this handle the case in which `onReady` fires too late?

Comment on attachment 8876825 [details] Bug 1372292 - Update sessionrestore Talos test profiling support. https://reviewboard.mozilla.org/r/148146/#review153456

Comment on attachment 8876826 [details] Bug 1372292 - Bump sessionrestore Talos test add-on version and signed add-on. https://reviewboard.mozilla.org/r/148148/#review153458

Comment on attachment 8876824 [details] Bug 1372292 - Load sessionrestore Talos test result page in a tab after the measurement is done. https://reviewboard.mozilla.org/r/148144/#review154004 ::: testing/talos/talos/test.py:171 (Diff revision 1) > - url = 'startup_test/sessionrestore/index.html' > shutdown = False > reinstall = ['sessionstore.js', 'sessionCheckpoints.json'] > - # Restore the session > + # Restore the session. We have to provide a URL, otherwise Talos > + # asks for a manifest URL. > + url = 'about:blank' Should make this about:home, since I think that's more realistic.

Comment on attachment 8876824 [details] Bug 1372292 - Load sessionrestore Talos test result page in a tab after the measurement is done. https://reviewboard.mozilla.org/r/148144/#review153454 > Mmmh... How does this handle the case in which `onReady` fires too late? In that case, we'll find the most recent window with `let win = Services.wm.getMostRecentWindow("navigator:browser");` and pass that window to `onWindow` immediately.

Does comment 10 address your concern, or is there a case I'm not seeing?

(In reply to Mike Conley (:mconley) from comment #11) > Does comment 10 address your concern, or is there a case I'm not seeing? Can you just add a comment explaining this in the code? With this, you have my r+.

Comment on attachment 8876824 [details] Bug 1372292 - Load sessionrestore Talos test result page in a tab after the measurement is done. https://reviewboard.mozilla.org/r/148144/#review154300

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #12) > (In reply to Mike Conley (:mconley) from comment #11) > > Does comment 10 address your concern, or is there a case I'm not seeing? > > Can you just add a comment explaining this in the code? With this, you have > my r+. Sure thing, thanks!

Try pushes coming in to see if this affects numbers: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=7f472c4484ee&newProject=try&newRevision=5a04005fdf37&framework=1&showOnlyImportant=0 The only thing that might affect this is the fact that we're loading about:home now in both tests. So we might see a small bump here, but I'd say it's more realistic then what's currently being tested.

I'm having trouble signing this one using the instructions at: https://wiki.mozilla.org/EngineeringProductivity/HowTo/SignExtensions I get: Server response: You do not own this addon. (status: 403) Waiting on TheOne to ensure that I'm using the right signing account.

Solved.

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/564f68456744 Load sessionrestore Talos test result page in a tab after the measurement is done. r=Yoric https://hg.mozilla.org/integration/autoland/rev/fbd39f16600f Update sessionrestore Talos test profiling support. r=Yoric https://hg.mozilla.org/integration/autoland/rev/be3b14913489 Bump sessionrestore Talos test add-on version and signed add-on. r=Yoric

Just a heads up jmaher - this is likely going to cause what appears to be a small regression in sessionrestore_no_auto_restore. This is because I've modified that test to load about:home in the initial tab instead of the test page. about:home loading in the initial tab when not restoring a session by default is actually a far more realistic scenario, so any "regression" here should actually be interpreted as the test becoming more real-world.

ni'ing igoldan as per jmaher's recommendation for comment 26.

Thank you Mike, for making me aware of this test update.

The expected regression alerts have started to appear. These are among the first == Change summary for alert #7414 (as of June 19 2017 18:22 UTC) == Regressions: 6% sessionrestore_no_auto_restore linux64 opt e10s 676.42 -> 719.75 6% sessionrestore_no_auto_restore linux64 pgo e10s 584.67 -> 620.50 6% sessionrestore_no_auto_restore windows7-32 pgo e10s 626.50 -> 664.08 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7414

https://hg.mozilla.org/mozilla-central/rev/564f68456744 https://hg.mozilla.org/mozilla-central/rev/fbd39f16600f https://hg.mozilla.org/mozilla-central/rev/be3b14913489

Comment on attachment 8876824 [details] Bug 1372292 - Load sessionrestore Talos test result page in a tab after the measurement is done. https://reviewboard.mozilla.org/r/148144/#review165458 ::: testing/talos/talos/startup_test/sessionrestore/addon/SessionRestoreTalosTest.js:93 (Diff revision 3) > } > + > + // onReady might fire before the browser window has finished initializing > + // or sometimes soon after. We handle both cases here. > + let win = Services.wm.getMostRecentWindow("navigator:browser"); > + if (!win || !win.gBrowser) { This test causes intermittent failures: (from bug 1381853 comment #38) > There's a race here between the sessionStartup.onceInitialized promise > resolving, and the browser window loading. The existing code works well if: > - the promise resolves at a time when the window doesn't exist yet, or > hasn't finished loading all its scripts (ie. DOMContentLoaded hasn't been > fired yet) > - the promise resolves at a time when the browser window has finished > delayed startup. > > The existing code breaks if the promise resolves at a point where we have > already loaded all scripts, but haven't reached the delayedStartupFinished > point yet. This case becomes very frequent with the patches here, as > reducing the amount of scripts we load makes the browser window reach > DOMContentLoaded faster.