907502 - Make addPermission, removePermission, set***Prefs and clearUserPref methods synchronous

Status: RESOLVED INVALID

(Testing :: Mochitest, defect)

Description

[Martijn Wargers (dead)]

Attachment: specialpowersapi.diff

In b2g land (or also on desktop?), addPermission, removePermission, set***Prefs and clearUserPref methods are asynchronous, you can't rely on the permission/preference being set immediately afterwards (although from my observation, I haven't seen any problems related to that in b2g).

By spinning the event loop, you could make those methods synchronous, see patch (haven't tried the patch in b2g yet, on desktop it seems to work fine).

This would help immensely for bug 902686 and in general where these methods are used, because then, they wouldn't have to be rewritten to use pushPermissions/pushPrefEnv.

Comment 1

[Martijn Wargers (dead)]

See bug 621363, comment 4:
"
IMHO, alas, we should change these tests.  Spinning a nested event loop here is dangerous and seems like a recipe for intermittent orange.  We could add some kind of platform support for setting prefs+perms "immediately" in the child, but that doesn't seem wise because (i) we don't actually want those APIs to be used; (ii) not waiting for observer notifications also seems like a recipe for orange; and (iii) maintenance burden.
"

So I guess that means this is a no-go then?

Comment 2

[Martijn Wargers (dead)]

Chris Jones is not employed by Mozilla anymore.

I just need someone to confirm, this is not a good solution (so this bug can become WONTFIX).
Bonus points if someone could explain to me why spinning a nested event loop is dangerous an a recipe for intermittent orange.

Comment 3

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

I would rather we fix the tests to use pushPrefEnv since that is designed to wait for confirmation before returning.

Comment 4

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #0)
> In b2g land (or also on desktop?), addPermission, removePermission,
> set***Prefs and clearUserPref methods are asynchronous, you can't rely on
> the permission/preference being set immediately afterwards (although from my
> observation, I haven't seen any problems related to that in b2g).

ok, dom/tests/mochitest/general/test_bug629535.html is a test that needs the asynchronous pref settings.

Comment 5

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Joel, any comment on the event loop? We do something similar in Mozmill for the waitFor(), IIRC. 

Since this sort of thing is a common technique for making a non-blocking call block, I'd love to know what the issue is there too.

Comment 6

[Josh Matthews [:jdm]]

Nested event loops can introduce strange behaviour, such as JS error reporters that swallow exceptions since they see JS code running. Whenever possible, we should avoid introducing more of them.

Comment 7

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

I agree with jdm.  Sometimes you can't avoid it, but I try really hard to avoid it whenever possible.

Comment 8

[Martijn Wargers (dead)]

I don't think this is wanted (see comment 6), so I'll close this bug.

Comment 9

[Martijn Wargers (dead)]

Bug 951272 is about conversion to pushPermissions and bug 1056851 to pushPrefEnv.