1257554 - [e10s] Remove restore on demand checkbox

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Jim Mathies [:jimm]]

Attachment: 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.

Comment 1

[Jim Mathies [:jimm]]

a clarification: restore on demand is the default setting and works under e10s. unchecking this checkbox produces broken behavior.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Dolske, this is blocking e10s, can you help us get an assignee?

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: restoreTabsOnDemance_option.patch

Comment 4

[:Felipe Gomes (needinfo for replies!)]

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)

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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. 

Comment 6

[Justin Dolske [:Dolske]]

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?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

(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.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5276ca75c7f6

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5276ca75c7f6

Comment 10

[Francesco Lodolo [:flod]]

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.

Comment 11

[Masatoshi Kimura [:emk]]

(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?

Comment 12

[Francesco Lodolo [:flod]]

(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.

Comment 13

[Willy_ Foo_Foo]

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?

Comment 14

[Willy_ Foo_Foo]

Alice and others advocate for a mass redirection of staff from Firefox and e10s to Servo. Instead of 100 working on Firefox and 10 on Servo, do the opposite.

I agree too that instead of removing user's choice focus on what made its so great.

sorry for the off-topic comment.

Comment 15

[Jim Mathies [:jimm]]

(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?

Comment 16

[Luke]

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).

Comment 17

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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?

Comment 18

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 19

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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.

Comment 20

[Willy_ Foo_Foo]

(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)

Comment 21

[Justin Dolske [:Dolske]]

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?

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

The conclusion was to file a follow up and deal with it in the e10s migration code

Comment 23

[Justin Dolske [:Dolske]]

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.

Comment 24

[Jim Mathies [:jimm]]

(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.

Comment 25

[Chris Peterson [:cpeterson]]

Brad, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

I think that's up to Jeff

Comment 27

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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.

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 29

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch for aurora (excludes the strings removals)

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.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8733363 [details] [diff] [review]
restoreTabsOnDemance_option.patch

e10s related, Aurora47+

Comment 31

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/81882fedce8a

Comment 32

[Khalid Syfullah Zaman [:khalid32]]

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

Comment 33

[Mohammad Maruf Islam]

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!

Comment 34

[[:philipp]]

should we document this change in a release note (it's a fairly prominent user facing pref)?

Comment 35

[[:philipp]]

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"