1365541 - Fix FX_SESSION_RESTORE_RESTORE_WINDOW_MS regression

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Dão Gottwald [:dao]]

(In reply to Chris H-C :chutten from bug 1054740 comment #56)
> It appears as though this bug may have contributed to a sudden change in the
> Telemetry probe FX_SESSION_RESTORE_RESTORE_WINDOW_MS[1] which seems to have
> occurred in Nightly 20170506[2][3].
> 
> It seems as though it got rather slower. And it seems from [2] that it's
> undoing a previous improvement in this arena.
> 
> Is this actually a regression? Is this intentional? Is it expected?

If I read the graph correctly, it's a regression and it's neither intentional nor expected.

> Is this probe measuring something useful?

I think so.

> [1]:
> http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/750/
> alerts/?from=2017-05-06&to=2017-05-06
> [2]:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=0b255199db9d6a6f189b89b7906f99155bde3726&tochange=37a5
> b7f6f101df2eb292b1b6baaf1540c9920e20
> [3]: https://mzl.la/2rnqPjP

Comment 1

[Dão Gottwald [:dao]]

Wondering if bug 1364849 will help here...

Comment 2

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #1)
> Wondering if bug 1364849 will help here...

It looks like it didn't:

https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=null&keys=!__none__!__none__&max_channel_version=nightly%252F56&measure=FX_SESSION_RESTORE_RESTORE_WINDOW_MS&min_channel_version=nightly%252F55&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-05-06&trim=1&use_submission_date=0

Comment 3

[Dão Gottwald [:dao]]

I'm going to re-implement reusing the selected tab, hopefully with less code than that removed in bug 1054740.

Comment 4

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b2b8f4c0342ff49396581c35ae83ef74bd7ee21

Comment 5

[Dão Gottwald [:dao]]

Attachment: Bug 1365541 - Re-implement reusing the selected tab when restoring a window to fix FX_SESSION_RESTORE_RESTORE_WINDOW_MS regression.

Review commit: https://reviewboard.mozilla.org/r/150442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150442/

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8879105 [details]
Bug 1365541 - Re-implement reusing the selected tab when restoring a window to fix FX_SESSION_RESTORE_RESTORE_WINDOW_MS regression.

https://reviewboard.mozilla.org/r/150442/#review156188

LGTM, thanks for working on this Dão!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6d27875faec
Re-implement reusing the selected tab when restoring a window to fix FX_SESSION_RESTORE_RESTORE_WINDOW_MS regression. r=mikedeboer

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c6d27875faec

Comment 10

[randomreporter]

I reported about white flashes when switching to a loading tab in this comment to bug 1367596:
https://bugzilla.mozilla.org/show_bug.cgi?id=1367596#c22

Mike Conley wrote a plan to help reduce the remaining flashes had been laid out in bug 1371884 that has been closed as a duplicate of this bug so I am writing here. I still see white flashes when switching to a loading tab. The easiest STR:
Enable bookmarks toolbar, create https://videocardz.com/ bookmark
Drag this bookmark to the tab bar in order to open it in the background
Count to 10 in order to make sure page has loaded
Switch to this tab

ER: Contents of the web page are rendered immediately
AR: Contents of the web page are rendered after annoying white flash

ni Dao Gottwald because I think this is a serious regression.

Comment 11

[Dão Gottwald [:dao]]

(In reply to ajfhajf from comment #10)
This should have its own bug. I needinfo'd mconley.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 14

[Dão Gottwald [:dao]]

I'm waiting for a second (Friday's) FX_SESSION_RESTORE_RESTORE_WINDOW_MS data point. Thursday's data point is promising, suggesting that FX_SESSION_RESTORE_RESTORE_WINDOW_MS is lower than it ever was.

Comment 15

[Dão Gottwald [:dao]]

Verifying based on https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=null&keys=!__none__!__none__&max_channel_version=nightly%252F56&measure=FX_SESSION_RESTORE_RESTORE_WINDOW_MS&min_channel_version=nightly%252F55&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-05-06&trim=1&use_submission_date=0

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8879105 [details]
Bug 1365541 - Re-implement reusing the selected tab when restoring a window to fix FX_SESSION_RESTORE_RESTORE_WINDOW_MS regression.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1054740
[User impact if declined]: slower startup
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: this is central sessionstore code and as such stressed by hundreds of tests
[String changes made/needed]: /

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8879105 [details]
Bug 1365541 - Re-implement reusing the selected tab when restoring a window to fix FX_SESSION_RESTORE_RESTORE_WINDOW_MS regression.

Backout for perf regression, let's take this for beta 5 (or 6, depending on when we go to build).

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ceb3cb73a8b1

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This was backed out from 55 for causing bug 1388628.