1557692 - Consider disabling "browser.tabs.unloadOnLowMemory" for all tests (Mochitest, Reftest, ...)

Status: RESOLVED FIXED

(Testing :: General, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Similar to bug 1553748 which handles that for Marionette and geckodriver.

With bug 675539 we got the tab unload feature in case of low memory conditions, which means that any open tab could potentially been unloaded. Currently this can causing an invalid browser reference in Marionette, and tests will fail.

Main reason is that we do not track any events on tabs which are currently not selected. And with the behavior the underlying tab's content just disappears.

I wonder if we should just disable this preference for the time being when Marionette is enabled. This would also include any tests as run via geckodriver.

https://searchfox.org/mozilla-central/rev/952521e6164ddffa3f34bc8cfa5a81afc5b859c4/browser/app/profile/firefox.js#509-510

(In reply to Gabriele Svelto [:gsvelto] from bug 1553748 comment #1)

I'm all for disabling tab unloading for all tests with the exception of browser/modules/test/browser/browser_TabUnloader.js which might enable it manually. It's a mechanism that might make tests non-deterministic so it's better having it off unless we want to test it explicitly.

(In reply to Gabriele Svelto [:gsvelto] from bug 1553748 comment #5)

Just one note: right now the TabUnloader.init() reads the browser.tabs.unloadOnLowMemory on startup and doesn't observe it any other way. So if we want to flip it at runtime a small change will be needed.

Geoff, could you or someone else have a look at this?

Comment 1

[Geoff Brown [:gbrown]]

Andrew, any interest?

Comment 2

[Andrew Halberstadt [:ahal]]

This seems pretty important, but I'm not going to have time to do this any time before the end of H1. Maybe we can have a triage session in Whistler to try and figure out which P2's need to be bumped to P1 at the start of H2.

Comment 3

[Geoff Brown [:gbrown]]

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

I think the earlier changes for Marionette effectively disable unloadOnLowMemory for all desktop tests, including mochitests and reftests. Those changes wouldn't apply to Android (geckoview) mochitests and reftests, but it looks like the TabUnloader is not used by geckoview -- no worries there.

:whimboo - Is there a remaining concern?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Note that Marionette's profile including the preferences isn't used here. Mochitests define the preferences and as such would have to set this one itself. Marionette is only used for Mochitest/Reftest to bootstrap the necessary extensions.

Also note that the tab unloader got disabled a while ago (see bug 1558930), and it' unclear when it will be enabled again (bug 1587762). But to be on the safe bed, it would be good to get the preference set to false under testing/profiles.

Comment 5

[Geoff Brown [:gbrown]]

Thanks. I didn't notice that it had been disabled and that was confusing my interpretation of the try results.

Comment 6

[Geoff Brown [:gbrown]]

Attachment: Bug 1557692 - Set browser.tabs.unloadOnLowMemory=false during tests; r=

Comment 7

[Gabriele Svelto [:gsvelto]]

Note that we disabled the feature globally because of regressions that need to be addressed, but we're likely to flip it on again when possible so it's good to get this fixed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa2c925d7b7c
Set browser.tabs.unloadOnLowMemory=false during tests; r=whimboo

Comment 9

[Andreea Pavel [:apavel]]

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