Turn off tab unloading on release

RESOLVED FIXED in Firefox 68

Status

()

defect
P1
normal
RESOLVED FIXED
Last month
14 days ago

People

(Reporter: gsvelto, Assigned: gsvelto)

Tracking

unspecified
Firefox 69
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67+ wontfix, firefox68 fixed, firefox69 fixed)

Details

(Whiteboard: [pascal:drc67])

Attachments

(1 attachment)

We need to turn off tab unloading on release due to the flaws in the low-memory detection mechanism described in bug 1558554 comment 5. Setting the browser.tabs.unloadOnLowMemory preference to false is sufficient to achieve this.

Since we don't know how many users might be affected it's safer to turn off the mechanism and turn it on back again only when we're sure memory tracking works as expected.

Assignee: nobody → gsvelto
Blocks: 1558554, 675539
Priority: -- → P1

Gabriele, do you thing that this is an issue that would deserve a 67.0.3? Can you precise the impact to users and if this affect a lot of people (maybe as part of an uplift request actually)? Thanks

Flags: needinfo?(gsvelto)
Whiteboard: [pascal:drc67]

Unfortunately I don't know how widespread the issue is and it's because we don't know that I feel it would be safer to turn it off. The impact is obviously that background tabs are unloaded when they shouldn't be and this leads to annoying behaviour for users (especially when they have a confcall / other audio stuff in a background tab). I don't think this is bad enough to deserve a chemspill on its own but if we plan on doing one for other reasons it could tag along.

Flags: needinfo?(gsvelto)

Would you mind providing the patch here and filling out the uplift request?

Flags: needinfo?(gsvelto)
Status: NEW → ASSIGNED
Flags: needinfo?(gsvelto)

Comment on attachment 9072192 [details]
Bug 1558930 - Disable tab unloading when memory is running low

Beta/Release Uplift Approval Request

  • User impact if declined: Background tabs might be unloaded even when the system has plenty of memory available.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): We only flip a pref to disable tab unloading
  • String changes made/needed: None
Attachment #9072192 - Flags: approval-mozilla-release?
Attachment #9072192 - Flags: approval-mozilla-beta?
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28cfd20eeb70
Disable tab unloading when memory is running low r=mconley

Comment on attachment 9072192 [details]
Bug 1558930 - Disable tab unloading when memory is running low

approved for 68.0b11, thanks

Attachment #9072192 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69

Since this was filed upon Pascal's request we had two security releases where this fix could have been along for the ride, presumably. Has this been considered seriously? Why is the approval request still pending?

Flags: needinfo?(pascalc)
Flags: needinfo?(jcristau)

We're not taking ride alongs in chemspills.

Flags: needinfo?(pascalc)
Flags: needinfo?(jcristau)

So are there other opportunities for this to be fixed in 67, or does this mean wontfix?

Flags: needinfo?(jcristau)

Likely wontfix unless something comes up that drives a dot release in the next week.

Flags: needinfo?(jcristau)

Not going to ship another 67 dot release at this point unless it's a chemspill.

Attachment #9072192 - Flags: approval-mozilla-release? → approval-mozilla-release-

This might be relevant because it can take weeks or months for some people to take an update (and if for any reason we pause updates it could be delayed days or a week): This pref change can be pushed out via Normandy I presume. From discussion in Whistler, people are complaining about it aggressively unloading tabs that are active but not at the moment in the foreground, including video calls. (Correct?) Gabriele, it might help to expand on the impact and reports here

Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
Flags: needinfo?(gsvelto)

Yes, the biggest issue seems to be that we can unload tabs with active WebRTC sessions (bug 1554547). Worse than that, because of the way the Windows API we're using is behaving, this can happen on machines with plenty of available memory. I couldn't find a way to reproduce consistently but I saw it happen once on my machine so I don't know how widespread the problem is. That's the main reason why I decided to turn it off. Since we don't know the real extent of the problem we might underestimate it and preventing OOM crashes is no use if the users are annoyed in the first place.

Flags: needinfo?(gsvelto)
Flags: needinfo?(jcristau) → needinfo?(pascalc)

I am ok to have it disabled via Normandy for 67.

Flags: needinfo?(pascalc)
Flags: needinfo?(lhenry)
You need to log in before you can comment on or make changes to this bug.