Closed Bug 1257554 Opened 8 years ago Closed 8 years ago

[e10s] Remove restore on demand checkbox

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
e10s m9+ ---
firefox46 --- wontfix
firefox47 --- verified
firefox48 --- verified

People

(Reporter: jimm, Assigned: blassey)

References

Details

Attachments

(3 files)

Attached image checkbox.jpg
This feature (when unchecked) is broken under e10s. We should hide the checkbox until we can fix it.

This will need uplifting to 47.
a clarification: restore on demand is the default setting and works under e10s. unchecking this checkbox produces broken behavior.
Dolske, this is blocking e10s, can you help us get an assignee?
Flags: needinfo?(dolske)
Assignee: nobody → blassey.bugs
Attachment #8733363 - Flags: review?(blassey.bugs)
Attachment #8733363 - Flags: review?(blassey.bugs) → review?(felipc)
Attachment #8733363 - Flags: review?(felipc) → review+
Jeff, the patch here removes the checkbox from the preferences page. However, it won't help the users who already toggled it and have that set on their profile.

As I understand it, the purpose of the removal is that "not restoring on demand is just an awful experience", and if that's the case, we should probably just remove the feature entirely.

How should we handle it?

It would be too risky to uplift the feature removal, but it's easy to add a workaround (that can be uplifted uplift) to always treat restore on demand as true (i.e., ignore the pref that is in the profile)
Flags: needinfo?(jgriffiths)
(In reply to :Felipe Gomes (needinfo me!) from comment #4)
...
> It would be too risky to uplift the feature removal, but it's easy to add a
> workaround (that can be uplifted uplift) to always treat restore on demand
> as true (i.e., ignore the pref that is in the profile)

To be clear, are you saying you would both remove the preferences page checkbox *and* introduce code that ignores the preference when set? I prefer that because IMO not loading all tabs is a net win. We do not have data on how many people actually have the preference toggled so it is impossible to gauge the user impact. 
Flags: needinfo?(jgriffiths)
I support this removal -- there's a little bit of history in bug 711193 and bug 648683 which added this feature. The basic evolution of this, IIRC, was:

* Firefox used to restore all tabs. This was bad for startup performance (especially for lots of tabs).
* We added throttling so it only restored N tabs at a time. Made startup somewhat less painful, but you could still quickly end up with lots of tabs eating up CPU/memory.
* We noticed setting N=0 was nice for only restoring tabs as you used them, made it the default.

I think the main reason for adding a UI pref was concern (a bit omgchange) that you might be expecting something to be autorestored so it does something useful (notifying you of new mail, chat, playing music, etc.) But we soon made pinned tabs always restore (bug 674452), which is really a better way to support such usecases.

So removing the UI feels simple. Ideally we'd add some telemetry data to see how many people are actually disabled this, and let that inform how we might approach actually removing the support entirely. I'd suspect this isn't something lots of people change, but don't have actual data... Although from a quick check, Chrome seems to always restore all tabs, so there might be demand for it. And it's been suggested that restore-on-demand can make the browser feel slower (because the tabs have to load after you click them), although I'd say this is still an overall win.


As Felipe notes in the last comment, existing users may have already flipped this pref, so for E10S we'd have to either remove support for this pref entirely (not just the UI), fix it, or (temporarily) have it opt you out of E10S.

What exactly is the brokenness? Comment 1 says it produces "broken behavior", but it actually seems to work fine for me with E10S. The browser opens, background tabs start restoring, and everything seems ok. Are we actually talking about the 2x slowdown described in bug 1037179?
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #5)
> To be clear, are you saying you would both remove the preferences page
> checkbox *and* introduce code that ignores the preference when set? I prefer
> that because IMO not loading all tabs is a net win. We do not have data on
> how many people actually have the preference toggled so it is impossible to
> gauge the user impact.

Yeah, because just removing the checkbox won't help almost anyone because it does not reset their pref, just removes from the UI. So we would only be avoiding new users from getting into that situation, but not fixing the problem for the existing users.
https://hg.mozilla.org/mozilla-central/rev/5276ca75c7f6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Can someone clarify if this pref is gone for good? Asking because of bug 792210.

One more note: if you plan to uplift this on aurora (I see a comment, but no approval requests), please create a patch that doesn't touch string files to avoid creating unnecessary noise.
(In reply to Justin Dolske [:Dolske] from comment #6)
> I think the main reason for adding a UI pref was concern (a bit omgchange)
> that you might be expecting something to be autorestored so it does
> something useful (notifying you of new mail, chat, playing music, etc.) But
> we soon made pinned tabs always restore (bug 674452), which is really a
> better way to support such usecases.

Will pinned tabs work with e10s?
Flags: needinfo?(jmathies)
(In reply to Masatoshi Kimura [:emk] from comment #11)
> Will pinned tabs work with e10s?

Yes, they already do. That comment refers to the fact that pinned tabs always reload, independently from the pref.
only removed the UI (which is done)
but please let the preference be(as it works well in non-e10s & is quite useful to many)

on a personal note to devs
Instead of fixing e10s, you would rather eliminate the users' choice?
Firefox not being moved away from the highly user customization platform that made it the success that is was?
(In reply to Masatoshi Kimura [:emk] from comment #11)
> (In reply to Justin Dolske [:Dolske] from comment #6)
> > I think the main reason for adding a UI pref was concern (a bit omgchange)
> > that you might be expecting something to be autorestored so it does
> > something useful (notifying you of new mail, chat, playing music, etc.) But
> > we soon made pinned tabs always restore (bug 674452), which is really a
> > better way to support such usecases.
> 
> Will pinned tabs work with e10s?

Are you asking if bug 674452 works under e10s? It should work for normal startup restoration. I'm not sure about thew behavior after a content process crash. Mike, any idea?
Flags: needinfo?(jmathies) → needinfo?(mconley)
This feature is still useful for quite a lot of people in my opinion. Just because it's broken doesn't mean we should sweep it under the rug and hope for the best. 

With e10s getting better and better I can easily imagine a day when opening Firefox and restoring all tabs at once would be a bad experience, especially with throttling (like what Chrome does).
(In reply to Justin Dolske [:Dolske] from comment #6)

> ...Ideally we'd add some telemetry data to see
> how many people are actually disabled this, and let that inform how we might
> approach actually removing the support entirely. 

Justin: can you log a followup bug to add telemetry?
(In reply to Jim Mathies [:jimm] from comment #15)
> (In reply to Masatoshi Kimura [:emk] from comment #11)
> > (In reply to Justin Dolske [:Dolske] from comment #6)
> > > I think the main reason for adding a UI pref was concern (a bit omgchange)
> > > that you might be expecting something to be autorestored so it does
> > > something useful (notifying you of new mail, chat, playing music, etc.) But
> > > we soon made pinned tabs always restore (bug 674452), which is really a
> > > better way to support such usecases.
> > 
> > Will pinned tabs work with e10s?
> 
> Are you asking if bug 674452 works under e10s? It should work for normal
> startup restoration. I'm not sure about thew behavior after a content
> process crash. Mike, any idea?

After a content process crash, we do not automatically restore app tabs. This is bug 1257417, and (at least part of) the rationale is the same for not restoring background tabs - we don't want to automatically restore the content that might have caused the crash in the first place, which puts the user into a crash loop.
Flags: needinfo?(mconley)
(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #5)
> > To be clear, are you saying you would both remove the preferences page
> > checkbox *and* introduce code that ignores the preference when set? I prefer
> > that because IMO not loading all tabs is a net win. We do not have data on
> > how many people actually have the preference toggled so it is impossible to
> > gauge the user impact.
> 
> Yeah, because just removing the checkbox won't help almost anyone because it
> does not reset their pref, just removes from the UI. So we would only be
> avoiding new users from getting into that situation, but not fixing the
> problem for the existing users.

There seem to be a couple of problems with removing the pref at the moment:

1. some users want the other behaviour, and we don't know how many so it's hard to decide what to do. The followup is to measure how many people are using the non-default setting.

2. we haven't shipped e10s to everyone yet

Eventually I think we should remove this pref and instead implement something like bug 1260254.
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #19)
> (In reply to :Felipe Gomes (needinfo me!) from comment #7)
> > (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #5)
> > > To be clear, are you saying you would both remove the preferences page
> > > checkbox *and* introduce code that ignores the preference when set? I prefer
> > > that because IMO not loading all tabs is a net win. We do not have data on
> > > how many people actually have the preference toggled so it is impossible to
> > > gauge the user impact.
> > 
> > Yeah, because just removing the checkbox won't help almost anyone because it
> > does not reset their pref, just removes from the UI. So we would only be
> > avoiding new users from getting into that situation, but not fixing the
> > problem for the existing users.
> 
> There seem to be a couple of problems with removing the pref at the moment:
> 
> 1. some users want the other behaviour, and we don't know how many so it's
> hard to decide what to do. The followup is to measure how many people are
> using the non-default setting.
> 
> 2. we haven't shipped e10s to everyone yet
> 
> Eventually I think we should remove this pref and instead implement
> something like bug 1260254.

A pref + bug 1260254(with a pref too) would be ideal(& hopefully landed soon)
So, this landed and removed the UI, but the fundamental question of what happens to users who already had already disabled this pref doesn't seem to have been addressed. (Last part of comment 6 and comment 7.) Either I'm missing something, or this needs to be reopened / follow-up'd to fix it properly?
Flags: needinfo?(dolske) → needinfo?(blassey.bugs)
The conclusion was to file a follow up and deal with it in the e10s migration code
Flags: needinfo?(blassey.bugs)
Depends on: 1260460
Jim: what was the "brokenness" this was reported for? See end of comment 6 -- it seems to work fine for me. While we still probably want to eventually remove this UI... If it's not actually breaking E10S and the perf impact in bug 1037179 is still being sorted out, we shouldn't rush into this.
Flags: needinfo?(jmathies)
(In reply to Justin Dolske [:Dolske] from comment #23)
> Jim: what was the "brokenness" this was reported for? See end of comment 6
> -- it seems to work fine for me. While we still probably want to eventually
> remove this UI... If it's not actually breaking E10S and the perf impact in
> bug 1037179 is still being sorted out, we shouldn't rush into this.

Yes bug 1037179 was the trigger for this bug.
Flags: needinfo?(jmathies)
Brad, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?
Flags: needinfo?(blassey.bugs)
I think that's up to Jeff
Flags: needinfo?(blassey.bugs) → needinfo?(jgriffiths)
(In reply to Chris Peterson [:cpeterson] from comment #25)
> Brad, should this fix be uplifted to Aurora 47 in preparation for our e10s
> experiments on Beta 47?

I would prefer that, barring any objections from relman.
Flags: needinfo?(jgriffiths)
Comment on attachment 8733363 [details] [diff] [review]
restoreTabsOnDemance_option.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: possible to set a non default pref that will give terrible performance with e10s
[Describe test coverage new/current, TreeHerder]: on nightly
[Risks and why]: risk is a user will have to use about:config to change this pref
[String/UUID change made/needed]: removing strings
Attachment #8733363 - Flags: approval-mozilla-aurora?
Note for sheriffs: when landing, this patch should be used as it doesn't contain the string removal that landed on central.

For relman: I requested approval for bug 1260460 too and the two should ride together.
Attachment #8739241 - Flags: review+
Comment on attachment 8733363 [details] [diff] [review]
restoreTabsOnDemance_option.patch

e10s related, Aurora47+
Attachment #8733363 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I have reproduced this bug on Nightly 48.0a1 (2016-03-17) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Nightly 48.0a1!

Build ID: 20160412050029
User Agent: Mozilla/5.0 (X11; Linux i686; rv:48.0) Gecko/20100101 Firefox/48.0

Also the bug’s fix is now verified on Latest Developer Edition 47.0a2!

Build ID: 20160412004019
User Agent: Mozilla/5.0 (X11; Linux i686; rv:47.0) Gecko/20100101 Firefox/47.0
QA Whiteboard: [bugday-20160413]
I have reproduced this bug with Firefox Nightly 48.0a1 (Build ID: 20160317030235) on 
Windows 8.1, 64-bit.

Verified as fixed with Firefox beta 47.0b1 (Build ID: 20160425095909)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0

Verified as fixed with Firefox Developer edition 48.0a2 (Build ID: 20160429004052)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

As this bug is also verified on Linux (Comment 32), I am marking this as verified!
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20160413] → [bugday-20160413][bugday-20160427]
should we document this change in a release note (it's a fairly prominent user facing pref)?
relnote-firefox: --- → ?
oops, it already is in the release notes: "The browser.sessionstore.restore_on_demand preference has been reset to its default value (true) to avoid e10s performance problems"
Depends on: 1280501
You need to log in before you can comment on or make changes to this bug.