791881 - SpecialPowers.setPref and Services.prefs.getPref aren't in sync on child process

Status: NEW

(Core :: DOM: Core & HTML, defect)

Description

[Gregor Wagner [:gwagner]]

We see some races using following code in a child process (mochitest on otoro):
SpecialPowers.setBoolPref("xxx", false);


SpecialPowers.getBoolPref("xxx");
Services.prefs.getBoolPref("xxx");

According to bent, SpecialPowers uses a sync approach and Services an async approach.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Gregor Wagner [:gwagner] from comment #0)
> According to bent, SpecialPowers uses a sync approach and Services an async
> approach.

Not quite... SpecialPowers sends sync messages to the parent, but then the message that the parent sends to the child to update the child's pref set is an async message. Due to the way IPDL messages work that async message won't be seen until after the child has returned to the event loop.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Isn't this why pushPrefEnv exists?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Not as far as I can tell. pushPrefEnv() just pushes prefs so that they can be popped off automatically at the end of the test, right? There is some fancy setTimeout() madness there to try to wait for all of the parent process changes to trickle down, so maybe you're right?

However, it's really not obvious that setBoolPref() is "wrong" and pushPrefEnv() is "right" here. We should either fix setBoolPref() to do the change synchronously or remove the sync API here entirely.

Comment 5

[Gregor Wagner [:gwagner]]

tryserver is not happy: https://tbpl.mozilla.org/?tree=Try&rev=cb307323bc79

Comment 6

[Josh Matthews [:jdm]]

The origin of pushPrefEnv came from my investigations that revealed this problem. pushPrefEnv takes a callback that executes once the child process receives a notification that the pref is updated, making it the correct thing to use. My patch that made setFooPref spin the event loop until the same thing happened was denied.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Tests aren't allowed to use set*Pref() directly.

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> Tests aren't allowed to use set*Pref() directly.

Hm I guess I don't understand. How should we use it instead?
From MDN: SpecialPowers is a set of APIs available to Mochitest tests.
If we can't use SpecialPowers.set*Pref we have to rewrite many tests.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> Tests aren't allowed to use set*Pref() directly.

http://mxr.mozilla.org/mozilla-central/search?string=SpecialPowers\.set.*Pref&regexp=1&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Gregor Wagner [:gwagner] from comment #8)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> > Tests aren't allowed to use set*Pref() directly.
> 
> Hm I guess I don't understand. How should we use it instead?

pushPrefEnv().

> From MDN: SpecialPowers is a set of APIs available to Mochitest tests.
> If we can't use SpecialPowers.set*Pref we have to rewrite many tests.

The plan was to remove all those and then remove set*().

Comment 11

[Martijn Wargers (dead)]

I've filed bug 1056851 for conversion to SpecialPowers.pushPrefEnv.
I guess this bug could be used for that, but I think it would be better to have a new bug for it.
But it would mean this bug can closed. Is it ok to close this bug?

Comment 12

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.