Closed
Bug 1261657
Opened 9 years ago
Closed 9 years ago
StartupPerformance.jsm can record remoteness flips as eager tab loads.
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mconley, Assigned: mconley)
References
Details
Attachments
(1 file)
For context, please see bug 1259770 comment 9.
Essentially, anytime a browser tab goes from a non-remote-friendly page (like about:newtab!) to a remote-friendly page (like any webpage!), or vice versa, we fire an SSTabRestored event, and if it's been less than 10 seconds since the last remoteness flip like that, we'll reset the timer in StartupPerformance.jsm and consider that SSTabRestored an eager tab load off of the main session restore.
This was initially discovered during my investigations into the strange discrepancy between e10s and non-e10s in bug 1259770. It's not at all user facing, but it means that our Telemetry data for FX_SESSION_RESTORE_NUMBER_OF_EAGER_TABS_RESTORED, FX_SESSION_RESTORE_NUMBER_OF_TABS_RESTORED, FX_SESSION_RESTORE_NUMBER_OF_WINDOWS_RESTORED, FX_SESSION_RESTORE_AUTO_RESTORE_DURATION_UNTIL_EAGER_TABS_RESTORED_MS and FX_SESSION_RESTORE_MANUAL_RESTORE_DURATION_UNTIL_EAGER_TABS_RESTORED_MS are probably untrustworthy, since I imagine it's probably a pretty common use-case to open a new tab and browse to a website within the first 10 seconds of opening the browser.
Instead of monkeying with the talos test to try to avoid the erroneous SSTabRestore recording, I think we actually want to correct it in StartupPerformance.jsm so that our probes are more accurate.
What I propose is that the SSTabRestored event gets a detail that says whether or not the event is the result of a remoteness flip, and then have StartupPerformance.jsm ignore that event.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mconley
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/44019/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44019/
Attachment #8737569 -
Flags: review?(dteller)
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8737569 [details]
MozReview Request: Bug 1261657 - Don't record SSTabRestored events in StartupPerformance that are the result of a remoteness flip. r?Yoric
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44019/diff/1-2/
Comment on attachment 8737569 [details]
MozReview Request: Bug 1261657 - Don't record SSTabRestored events in StartupPerformance that are the result of a remoteness flip. r?Yoric
https://reviewboard.mozilla.org/r/44019/#review40573
Good catch, thank you very much.
I have no idea how I could have spotted the remoteness flip.
::: browser/components/sessionstore/SessionStore.jsm:3969
(Diff revision 2)
> * Dispatch an SSWindowState_____ event for the given window.
> * @param aWindow the window
> * @param aType the type of event, SSWindowState will be prepended to this string
> */
> _sendWindowStateEvent: function ssi_sendWindowStateEvent(aWindow, aType) {
> let event = aWindow.document.createEvent("Events");
This one is still `Events`, while below you change to `CustomEvent`. Not sure if there is a reason.
::: browser/components/sessionstore/SessionStore.jsm:3983
(Diff revision 2)
> - _sendTabRestoredNotification: function ssi_sendTabRestoredNotification(aTab) {
> - let event = aTab.ownerDocument.createEvent("Events");
> - event.initEvent("SSTabRestored", true, false);
> + * @param aIsRemotenessUpdate
> + * True if this tab was restored due to flip from running from
> + * out-of-main-process to in-main-process or vice-versa.
> + */
> + _sendTabRestoredNotification(aTab, aIsRemotenessUpdate) {
> + let event = aTab.ownerDocument.createEvent("CustomEvent");
Is there any chance that this change from `Events` to `CustomEvent` will impact add-on compatibility?
Attachment #8737569 -
Flags: review?(dteller) → review+
Assignee | ||
Comment 4•9 years ago
|
||
https://reviewboard.mozilla.org/r/44019/#review40573
As someone who works on e10s, tinkers with sessionstore, and was the reviewer, _I_ should have caught this, so I take responsibility. :)
Thanks for the fast review!
> This one is still `Events`, while below you change to `CustomEvent`. Not sure if there is a reason.
I use CustomEvent so that I can pass the detail object - otherwise, it won't accept it via it's init function `initEvent` (see [this documentation](https://developer.mozilla.org/en-US/docs/Web/API/Event/initEvent)).
> Is there any chance that this change from `Events` to `CustomEvent` will impact add-on compatibility?
I don't believe so, no. This will still look like an event called SSTabRestored, but it'll have the detail property on it.
Assignee | ||
Comment 6•9 years ago
|
||
Comment 7•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in
before you can comment on or make changes to this bug.
Description
•