Closed Bug 907502 Opened 7 years ago Closed 6 years ago

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

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: martijn.martijn, Unassigned)

Details

Attachments

(1 file)

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.
Attachment #793232 - Flags: review?(jmaher)
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?
Attachment #793232 - Flags: review?(jmaher)
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.
I would rather we fix the tests to use pushPrefEnv since that is designed to wait for confirmation before returning.
(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.
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.
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.
I agree with jdm.  Sometimes you can't avoid it, but I try really hard to avoid it whenever possible.
I don't think this is wanted (see comment 6), so I'll close this bug.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
Bug 951272 is about conversion to pushPermissions and bug 1056851 to pushPrefEnv.
You need to log in before you can comment on or make changes to this bug.