1431194 - SpecialPowers pushPrefEnv times out when clearing pref if original value was restored by the test

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

[Julian Descottes [:jdescottes]]

STRs:

- assuming we have "my-pref", bool pref with default value of false
- in a test use:
    SpecialPowers.pushPrefEnv({"set": [["my-pref", true]]}, callback);
- in the same test set the pref manually
    Services.prefs.setBoolPref("my-pref", false)

The test will then time out when trying to cleanup. pushPrefEnv assumes the cleanup actions will have an observable effect and can easily hang.

Comment 1

[Julian Descottes [:jdescottes]]

I have a patch ready that seems to fix the issue, however I can't reproduce it in isolation in the unit test for SpecialPowers.
testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPrefEnv.html

I'm not sure why I can't trigger the bug in the unit test, but we can use a minimal mochitest to verify the issue: 

  const {utils: Cu} = Components;
  Cu.import("resource://gre/modules/Services.jsm");
  waitForExplicitFinish();

  add_task(async function () {
    await new Promise(resolve => {
      let options = {"set": [["devtools.toolbox.splitconsoleEnabled", true]]};
      SpecialPowers.pushPrefEnv(options, resolve);
    });

    Services.prefs.setBoolPref("devtools.toolbox.splitconsoleEnabled", false);
    ok(true, "Pass");
  });

This test currently hangs, waiting for the observer at https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/testing/specialpowers/content/specialpowersAPI.js#1213

If you remove the setBoolPref, the test finishes.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1431194 - avoid timeouts when clearing prefs set via pushPrefEnv;

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

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1431194 - [WIP] add unit test for flushPrefEnv timeout

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

Comment 4

[Julian Descottes [:jdescottes]]

Joel, can you take a look at the patches attached here and let me know what you think. Note that I am still blocked with the unit test as explained in the previous comment, but maybe you know why I have a different behavior in the unit test than in a mochitest?

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8943373 [details]
Bug 1431194 - avoid timeouts when clearing prefs set via pushPrefEnv;

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

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8943374 [details]
Bug 1431194 - [WIP] add unit test for flushPrefEnv timeout

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

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8943373 [details]
Bug 1431194 - avoid timeouts when clearing prefs set via pushPrefEnv;

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

Comment 8

[Julian Descottes [:jdescottes]]

Figured out the issue with the test, needed to setTimeout 0 after manually setting the pref, otherwise the flush was catching the event coming from the set, instead of catching the event coming from the cleanup.

Will check the impact on existing tests and submit for review if try is fine.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8943373 [details]
Bug 1431194 - avoid timeouts when clearing prefs set via pushPrefEnv;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213698/diff/3-4/

Comment 10

[Julian Descottes [:jdescottes]]

Try is green at https://treeherder.mozilla.org/#/jobs?repo=try&revision=652472a1b9e4d262d9a806ae091961533376d02f

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8943373 [details]
Bug 1431194 - avoid timeouts when clearing prefs set via pushPrefEnv;

https://reviewboard.mozilla.org/r/213698/#review219686

overall this looks good- I know there are a few bugs hidden in pushPrefEnv, this fixes one of them, maybe a couple :)

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPrefEnv.html:222
(Diff revision 4)
> +function test13() {
> +  // Try to flush preferences. Since test.cleanup has manually been set to false, there
> +  // will not be any visible update. Check that the flush does not timeout.
> +  SpecialPowers.flushPrefEnv(() => {
> +    ok(true, 'flushPrefEnv did not time out');
> +    SimpleTest.finish();

should we verify the value of test.cleanup==false?

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8943373 [details]
Bug 1431194 - avoid timeouts when clearing prefs set via pushPrefEnv;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213698/diff/4-5/

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8943373 [details]
Bug 1431194 - avoid timeouts when clearing prefs set via pushPrefEnv;

https://reviewboard.mozilla.org/r/213698/#review219686

> should we verify the value of test.cleanup==false?

Thanks for the review! Yes doesn't hurt to check this before ending the test.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f746bffde0ff
avoid timeouts when clearing prefs set via pushPrefEnv;r=jmaher

Comment 15

[Andrei Ciure[:aciure]]

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