Closed Bug 1261657 Opened 5 years ago Closed 5 years ago

StartupPerformance.jsm can record remoteness flips as eager tab loads.

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

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: nobody → mconley
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+
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.
https://hg.mozilla.org/mozilla-central/rev/c2bfc27e083f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.