Investigate FX_SESSION_RESTORE_RESTORE_WINDOW_MS regression starting on 2016-07-26

VERIFIED FIXED in Firefox 50

Status

()

Firefox
Session Restore
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

({regression})

50 Branch
Firefox 51
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

The timeline for this regression seems to coincide with the work I did in bug 1261842.

https://hg.mozilla.org/mozilla-central/rev/402b6b26d2eb

is the likely regressor here. My current theory is that for pinned tabs, we initialize them as remote:

http://searchfox.org/mozilla-central/rev/740bb4ed16d070cf5d466231b30e80b9204aec54/browser/components/sessionstore/SessionStore.jsm#2956

And then here:

http://searchfox.org/mozilla-central/rev/740bb4ed16d070cf5d466231b30e80b9204aec54/browser/components/sessionstore/SessionStore.jsm#3226

We flip them to non-remote again, which is totally stupid since we're just going to flip them remote once they start loading their content (which is ASAP).

I suspect that's what's going on. I'll have a patch soon.
Created attachment 8777528 [details]
Bug 1291860 - Don't flip remoteness of pinned tabs on session restore.

The initial browser of new windows starts remote now. When restoring a session,
if we're restoring content into the initial tab and it's going to be loaded
on demand, then we would flip it to non-remote so that it can't background crash.

We'd do this for pinned tabs too, which is silly, since pinned tabs load ASAP.

This patch causes us to skip the remoteness flip if the tab we're restoring
is pinned.

Review commit: https://reviewboard.mozilla.org/r/69076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69076/
Attachment #8777528 - Flags: review?(mdeboer)
https://reviewboard.mozilla.org/r/69076/#review66166

::: browser/components/sessionstore/SessionStore.jsm:3228
(Diff revision 1)
>      // browser is, by default, remote, so this check and flip is
>      // mostly for that case.
> -    if (browser.isRemoteBrowser && (!restoreImmediately || forceOnDemand)) {
> +    //
> +    // Note that pinned tabs are exempt from this flip, since
> +    // they'll be loaded immediately anyways.
> +    if (browser.isRemoteBrowser &&

Perhaps it wise at this point to put this logic into a separate method, including the story above it?

```js
this._maybeUpdateBrowserRemoteness(Object.assign({ browser, tabbrowser, tabData }, options));

// ...

_maybeUpdateBrowserRemoteness({ browser, tabbrowser, tabData, restoreImmediately, forceOnDemand }) {
  // ...
},
```

With that r=me, because I the change LGTM.
Comment on attachment 8777528 [details]
Bug 1291860 - Don't flip remoteness of pinned tabs on session restore.

https://reviewboard.mozilla.org/r/69076/#review66168
Attachment #8777528 - Flags: review?(mdeboer) → review+
Comment on attachment 8777528 [details]
Bug 1291860 - Don't flip remoteness of pinned tabs on session restore.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69076/diff/1-2/
I want to point out that this patch isn't a definitive fix for the regression. It's fixing a real inefficiency, but I'm not 100% certain it's the bottleneck that's showing up in Telemetry. I guess we'll see once it's in the tree and has some time to filter out to our users.
Hey chutten,

Out of curiosity, do you know how long we'll have to wait after this goes out in a Nightly to determine whether or not it's having an impact?
Flags: needinfo?(chutten)

Comment 7

2 years ago
The aggregator fuelling telemetry.mozilla.org runs nightly on whatever data has been received that previous day. If you take a look at the evolution for your measure [1] you can see that it has data right up to yesterday. Now, if you look at the second graph on that page, you can see the submission volume over time and see that the submission volume for yesterday is really low.

This is expected as we don't get all of the data immediately. It can take a day or two to get a representative sample.

So, the overall delay is probably: 1 day for the aggregator run + 1 day (maybe 2) to get a representative sample of the population so that the numbers coming back are representative. So: 2 days (maybe three). Incidentally, this is why the medusa alerting system doesn't run on data less than 48 hours old.

[1]: https://mzl.la/2axIyju
Flags: needinfo?(chutten)
Hey mikedeboer, I've updated the patch with your suggestion and added some documentation. Still look good?
Flags: needinfo?(mdeboer)
(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #8)
> Hey mikedeboer, I've updated the patch with your suggestion and added some
> documentation. Still look good?

Yup! I do think we need to reconsider the JSDoc formatting in sessionstore, because *boy* is that not fun to read and/ or write. Way too non-standard and verbose.
But that's not part of this patch. Carry on! :)
Flags: needinfo?(mdeboer)

Comment 10

2 years ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3bf9c257e4bd
Don't flip remoteness of pinned tabs on session restore. r=mikedeboer

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3bf9c257e4bd
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment on attachment 8777528 [details]
Bug 1291860 - Don't flip remoteness of pinned tabs on session restore.

Approval Request Comment
[Feature/regressing bug #]:

Bug 1261842.

[User impact if declined]:

Bug 1290280 (which has already been approved for uplift) won't be able to land. I forgot that this patch is a dependency on bug 1290280, which is why it had merge conflicts when our sheriffs attempted to land it.

[Describe test coverage new/current, TreeHerder]:

Bug 1290280 includes tests.

[Risks and why]: 

I'd say low risk, especially because the tests from bug 1290280 show us doing the right thing now.

[String/UUID change made/needed]:

None.
Attachment #8777528 - Flags: approval-mozilla-aurora?

Updated

2 years ago
status-firefox50: --- → affected

Updated

2 years ago
Keywords: regression
Comment on attachment 8777528 [details]
Bug 1291860 - Don't flip remoteness of pinned tabs on session restore.

Recent regression, stabilized on Nightly for 2 weeks, Aurora50+
Attachment #8777528 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: nobody → mconley
Version: unspecified → 50 Branch
You need to log in before you can comment on or make changes to this bug.