Closed Bug 1553748 Opened 5 years ago Closed 5 years ago

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

Categories

(Remote Protocol :: Marionette, defect, P1)

67 Branch
defect

Tracking

(firefox67 wontfix, firefox67.0.1 wontfix, firefox68 fixed, firefox69 fixed)

RESOLVED FIXED
mozilla69
Tracking Status
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file)

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.

Flags: needinfo?(mconley)
Flags: needinfo?(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.

Flags: needinfo?(gsvelto)

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.

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.

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

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.

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

Flags: needinfo?(mconley)
Blocks: 1534702

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(dburns)
See Also: → 1557692

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

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: -- → P1
Version: unspecified → 67 Branch
Flags: needinfo?(dburns)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0812921d7c2
[marionette] Firefox should not unload tabs when available memory is running low. r=webdriver-reviewers,automatedtester
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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

Whiteboard: [checkin-needed-beta]
Whiteboard: [checkin-needed-beta]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: