1268981 - Keep bfcache alive during session store form data test

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Jan Henning [:JanH]]

The session store form data navigation test (https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/test_session_form_data.html#195) only works when the bfcache is active, otherwise the inner value is not restored after going back.
As a memory pressure event will disable the bfcache for the current session, we need to make sure that the bfcache is alive while that test is running.

Comment 1

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1268981 - Make sure the bfcache is enabled during the mobile session store form data test. r=margaret

Without the bfcache, the inner value will not be restored after a page navigation, therefore we need to make sure that the bfcache isn't disabled by a stray memory pressure event before or during the test.

Review commit: https://reviewboard.mozilla.org/r/49901/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49901/

Comment 2

[:Margaret Leibovic]

Has this actually caused problems in practice? I'm wary to take speculative changes if we haven't experienced a problem.

Comment 3

[Jan Henning [:JanH]]

Since you're asking, apparently yes :-), see bug 1270335.

Plus some instances of this will have been masked by bug 1268177 - with that one fixed, the test will always keep going (unless we're OOM killed) until it hits the form data test which won't work without the bfcache.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8747460 [details]
MozReview Request: Bug 1268981 - Make sure the bfcache is enabled during the mobile session store form data test. r=margaret

https://reviewboard.mozilla.org/r/49901/#review47757

Well then! I have no problem landing this if it fixes a real problem. r+ with comments addressed.

::: mobile/android/tests/browser/chrome/test_session_form_data.html:253
(Diff revision 1)
>    // Remove the tab.
>    gBrowserApp.closeTab(gBrowserApp.getTabForBrowser(browser));
> +
> +  // Remove the bfcache memory pressure protection.
> +  Services.prefs.setIntPref("browser.sessionhistory.max_total_viewers", oldMaxTotalViewers);
> +  Services.prefs.setBoolPref("browser.sessionhistory.bfcacheIgnoreMemoryPressure", false);

Instead of remembering the old value, you should be able to just call `Services.prefs.clearUserPref` for both of these pref values.

It's confusing that the word "user" is in that method name, but really it just means "clear any pref value that was set while the app was running, restoring it to the default value".

You should also do this in a cleanup function that you register, e.g.:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/test_debugger_server.html?force=1#31

Comment 5

[Jan Henning [:JanH]]

Comment on attachment 8747460 [details]
MozReview Request: Bug 1268981 - Make sure the bfcache is enabled during the mobile session store form data test. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49901/diff/1-2/

Comment 6

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/49901/#review47757

> Instead of remembering the old value, you should be able to just call `Services.prefs.clearUserPref` for both of these pref values.
> 
> It's confusing that the word "user" is in that method name, but really it just means "clear any pref value that was set while the app was running, restoring it to the default value".
> 
> You should also do this in a cleanup function that you register, e.g.:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/test_debugger_server.html?force=1#31

Changed it. Since the memory observers changes the default value for the current session, I still need to explicitly set "max_history_viewers" to 1 at the beginning.

Comment 7

[Jan Henning [:JanH]]

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

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3e37c8cba109

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e37c8cba109