951272 - Convert SpecialPowers.addPermission usage to SpecialPowers.pushPermissions

Status: NEW

(Testing :: Mochitest, defect)

Description

[Bobby Holley (:bholley)]

See bug 946815 comment 21.

There are two possible outcomes here:
(1) We determine that the async behavior is a bug, and fix it in the platform.
(2) We determine that the async behavior is expected. If so, we make sure that the platform exposes the appropriate notification mechanisms, and then alter the SpecialPowers API so that consumers can pass a callback to SpecialPowers.{add,remove}Permission which will only be invoked when the permission change is guaranteed to have taken effect.

Once this bug is sorted, we can land the test changes from bug 946815.

Comment 1

[Bobby Holley (:bholley)]

The first step here is to determine whether the async behavior is a bug. Andrea, can you tell us?

Comment 2

[Martijn Wargers (dead)]

Regarding (1), I think that is bug 907502 (which is probably a wontfix, not sure).
Regarding (2), pushPermissions is what you want to use:
http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#607

Comment 3

[Bobby Holley (:bholley)]

Oh ok - great!

Why do we even have the addPermission API? Is there a compelling use-case, or can we remove it? At the very least, we should document this footgun.

Comment 4

[Martijn Wargers (dead)]

I think the addPermission API came when stuff was rewritten to use the specialPowers api.
Then, it was recognised that for the oop it does work asynchronously, which is why the pushPrefEnv/pushPermissions methods were invented.
The current usage of addPermission, etc, is something that needs to be rewritten to use pushPermissions.
Joel might have some more/better info on this.

Comment 5

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

we really should be using push/pop permissions as much as possible, we had a similar discussion in bug 907502.

I don't know of any reason to keep the addPermissions api,

Comment 6

[Martijn Wargers (dead)]

I'm morphing this bug into the rewrite to SpecialPowers.pushPermissions goal.
Here are the existing callers of addPermission: http://mxr.mozilla.org/mozilla-central/search?string=specialPowers.addpermission
Mochitests need to be converted.

I'm not sure about mochitest-chrome and browser-chrome, though. 
It seems less urgent, but the rewrite would make sure that the permissions would always be reset correctly after the test has been run, so I guess it would be better to convert those too.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

We should do this. The async nature of addPermission may have caused many intermittent failures. For example bug 994292 and bug 995438.

% git grep SpecialPowers.addPermission -- \*test\*|wc -l
193