sessionrestore_no_auto_restore test should load about:home, and not the sessionrestore test page

RESOLVED FIXED in Firefox 56

Status

Testing
Talos
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [PI:June])

MozReview Requests

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

Attachments

(3 attachments)

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?
Flags: needinfo?(dteller)
Whiteboard: [PI:June]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee: nobody → mconley
Yes, that makes sense.
Flags: needinfo?(dteller)
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
Attachment #8876825 - Flags: review?(dteller) → review+
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
Attachment #8876826 - Flags: review?(dteller) → review+
(Assignee)

Comment 9

a year ago
mozreview-review
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.
(Assignee)

Comment 10

a year ago
mozreview-review-reply
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?
Flags: needinfo?(mconley) → needinfo?(dteller)
(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+.
Flags: needinfo?(dteller)
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
Attachment #8876824 - Flags: review?(dteller) → review+
(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!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Flags: needinfo?(awagner)
Solved.
Flags: needinfo?(awagner)
Comment hidden (mozreview-request)

Comment 25

a year ago
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.
Flags: needinfo?(ionut.goldan)
Thank you Mike, for making me aware of this test update.
Flags: needinfo?(ionut.goldan)
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

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/564f68456744
https://hg.mozilla.org/mozilla-central/rev/fbd39f16600f
https://hg.mozilla.org/mozilla-central/rev/be3b14913489
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 31

a year ago
mozreview-review
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.
You need to log in before you can comment on or make changes to this bug.