1553748 - consider disabling "browser.tabs.unloadOnLowMemory" for Marionette / geckodriver

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

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

Mike or Gabriele, do you think that this will be fine? I don't see that other test jobs are setting it to false.

Comment 1

[Gabriele Svelto [:gsvelto]]

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.

Comment 2

[Jan Henning [:JanH]]

disabling tab unloading for all tests

We did the same thing for Fennec a while ago (bug 1268177) because otherwise we ended up unloading the test harness tab itself while it was in background.

Comment 3

[Andreas Tolfsen ❲:ato❳]

I’m all for making tests more predictable!

Note that browser-chrome tests are bootstrapped with Marionette,
so as gsvelto pointed out the tests that specifically need the tabs
to unload under low memory conditions would have to reset that pref
before they run. If this is a runtime pref I think that should be
just fine, since Marionette does not override user prefs (from
user.js) when it starts.

Ideally Marionette would detect when a tab has been unloaded and
deal with invalid browser references in a more graceful way. But
since we have no current intention of fixing that, and considering
that probably everyone running tests would like more predictable
test conditions, this seems like a really good idea.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #3)

Note that browser-chrome tests are bootstrapped with Marionette,
so as gsvelto pointed out the tests that specifically need the tabs
to unload under low memory conditions would have to reset that pref
before they run. If this is a runtime pref I think that should be

Actually not. Marionette is only used to install two extensions, but that's all.

Given that Marionette is not using the prepared profiles under testing/profiles the fix on this bug will be Marionette only, and I will file a separate bug for all the other test harnesses later today.

Comment 5

[Gabriele Svelto [:gsvelto]]

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.

Comment 6

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

Clearing needinfo, since it seems like gsvelto has this covered.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:automatedtester, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Please note that this bug is about Marionette / Geckodriver only. I filed bug 1557692 which covers all the other tests.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

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

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Attachment: Bug 1553748 - [marionette] Firefox should not unload tabs when available memory is running low. r=#webdriver

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0812921d7c2
[marionette] Firefox should not unload tabs when available memory is running low. r=webdriver-reviewers,automatedtester

Comment 12

[Andreea Pavel [:apavel]]

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

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

We also would like to have it for the 68 release. Please uplift this test-only change. Thanks.

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6fa17e5be147