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

RESOLVED FIXED in Firefox 59

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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.
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?
Flags: needinfo?(jmaher)
Attachment #8943374 - Attachment is obsolete: true
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.
Flags: needinfo?(jmaher)
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?
Attachment #8943373 - Flags: review?(jmaher) → review+
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.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f746bffde0ff
avoid timeouts when clearing prefs set via pushPrefEnv;r=jmaher
https://hg.mozilla.org/mozilla-central/rev/f746bffde0ff
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.