Open Bug 951272 Opened 11 years ago Updated 2 years ago

Convert SpecialPowers.addPermission usage to SpecialPowers.pushPermissions


(Testing :: Mochitest, defect)



(Not tracked)


(Reporter: bholley, Unassigned)




(Whiteboard: [lang=js])

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.
The first step here is to determine whether the async behavior is a bug. Andrea, can you tell us?
Flags: needinfo?(amarchesini)
Regarding (1), I think that is bug 907502 (which is probably a wontfix, not sure).
Regarding (2), pushPermissions is what you want to use:
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.
Flags: needinfo?(amarchesini) → needinfo?(martijn.martijn)
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.
Flags: needinfo?(martijn.martijn) → needinfo?(jmaher)
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,
Flags: needinfo?(jmaher)
I'm morphing this bug into the rewrite to SpecialPowers.pushPermissions goal.
Here are the existing callers of 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.
Component: General → Mochitest
Summary: SpecialPowers.addPermission does not always take immediate effect → Convert SpecialPowers.addPermission usage to SpecialPowers.pushPermissions
Mentor: martijn.martijn
Whiteboard: [lang=js]
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
Mentor: martijn.martijn
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.