859401 - pushPermissions missing PROMPT_ACTION case

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[David Chan [:dchan]]

http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#516

The original permission is checked against ALLOW_ACTION and DENY_ACTION. If the original permission is PROMPT_ACTION, then the callback will reset / remove the permission from the DB.

Comment 1

[David Chan [:dchan]]

Attachment: add prompt_action case

Try push
https://tbpl.mozilla.org/?tree=Try&rev=a274ecc092da

Comment 2

[David Chan [:dchan]]

Correct try run here, previous one was only for opt builds
https://tbpl.mozilla.org/?tree=Try&rev=19a88599c0ea

Comment 3

[David Chan [:dchan]]

Apparently I suck at managing mercurial queues and using try. Actual try push... that has the patch and runs the correct tests.

https://tbpl.mozilla.org/?tree=Try&rev=2bacffffb416

Sorry for the bug spam

Comment 4

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

Comment on attachment 734814 [details] [diff] [review]
add prompt_action case

Review of attachment 734814 [details] [diff] [review]:
-----------------------------------------------------------------

this is missing the work that is needed to revert the permission.  The original code for addPermission supported UNKNOWN, ALLOW, DENY.  Because of this it is easy.  We need to ensure that this is handled in more areas than just the original check.

Please ensure we have the proper value here:
http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#524

As well as the cleanup stuff here:
http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#533

I would also like to make sure we are sending the right values to the permissions manager.

Comment 5

[David Chan [:dchan]]

Oops, completely missed that part of the code. How do you recommend handling "allow"? Should this be changed to a 3 state flag or should I add another argument for PROMPT_ACTION.

My intent with this bug was to make sure that pushPermissions reset the original perm correctly. However as you point out, there is a whole use case missing.

Comment 6

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

I am not a permissions expert, but I would like to see us maybe add more logic into the original value and what we are setting.  Right now what is in there is based off of the addPermissions logic.  

Is there a chance that PROMPT_ACTION maps to UNKNOWN somehow?

Comment 7

[David Chan [:dchan]]

PROMPT_ACTION has a specific meaning in a couple areas of code. I've only seen PROMPT_ACTION being handled for b2g geolocation and desktop-notifications.  There are some other permissions that will result in a prompt when requested, but I don't know exactly how it works [1].

I think the only workflow test that would use PROMPT_ACTION is to test that ContentPermissionPrompt.js and PermissionPromptService.js pop up a prompt.

jonas: Any thoughts on how to add handling of PROMPT_ACTION in specialpowers?


http://mxr.mozilla.org/mozilla-central/source/b2g/components/ContentPermissionPrompt.js#44
http://mxr.mozilla.org/mozilla-central/source/dom/permission/PermissionPromptService.js#80

[1] - http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#37

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The attached patch seems correct. We should reset the permission manager to the state it was in before pushPermissions were called. In fact, you could even call nsIPermissionManager.testExactPermissionFromPrincipal to get the currently stored value and just set originalValue to that.

UNKNOWN and PROMPT are different things. UNKNOWN means "use default behavior" which for some APIs is to allow, for some to prompt and for some to deny.

Comment 9

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

maybe we are fine with this patch as per comment #8 ?

Comment 10

[Martijn Wargers (dead)]

Attachment: patch

Yeah, the patch is fine. I wanted to add some automated tests for this and encountered some other issues in the code, which this patch contains.
See this part:
-          cleeanupTodo.value = originalValue;
+          cleanupTodo.value = originalValue;
+          cleanupTodo.permission = originalValue;

Maybe it's an issue that those 2 test files need to be run after each other? But I don't know if there would be another, better way of detecting whether TestRunner is resetting the permissions correctly.

Comment 11

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

Comment on attachment 750142 [details] [diff] [review]
patch

Review of attachment 750142 [details] [diff] [review]:
-----------------------------------------------------------------

I really don't like to have tests which depend on each other.  Is there a way we could just call the flushPermissions() instead of depending on the end of the test to do it?

Comment 12

[Martijn Wargers (dead)]

Attachment: patch

Yeah, thanks, that works well.

Comment 13

[Martijn Wargers (dead)]

Comment on attachment 750566 [details] [diff] [review]
patch

This is not working in b2g, so I'll use SpecialPowers.addPermission

Comment 14

[Martijn Wargers (dead)]

Ok, addPermission and pushPermissions aren't allowing arguments other than true/false. So I can't use it to add permission values like PROMPT_ACTION.
For the test (and also for a test in bug 868439), I need this feature, so I'll update the patch with that.

Comment 15

[Martijn Wargers (dead)]

Attachment: patch

Ok, this should work in b2g (haven't tried that yet).
This added the feature for addPermission/pushPermissions that you can add PROMPT_ACTION/UNKNOWN_ACTION/DENY_ACTION/ALLOW_ACTION from nsiPermissionManager instead of true/false.

I have used it in the automated test for the addPermission case and I'll need it in the case of pushPermissions for bug 868439. But perhaps you want to have some more testing for it?

Comment 16

[Martijn Wargers (dead)]

Attachment: patch

Comment 17

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

Comment on attachment 750614 [details] [diff] [review]
patch

Review of attachment 750614 [details] [diff] [review]:
-----------------------------------------------------------------

this seems a lot better, thanks for adding this into the system.

Comment 18

[Martijn Wargers (dead)]

Attachment: patch for checkin

I removed some code in the test file that I forgot to remove:
var ioService = SpecialPowers.Cc["@mozilla.org/network/io-service;1"]
                             .getService(SpecialPowers.Ci.nsIIOService);

This isn't used by the test.

I guess I should ask for review for this minor change, officially, right?

Btw, I tested this patch, running all the mochitests on b2g.

Comment 19

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

Comment on attachment 750942 [details] [diff] [review]
patch for checkin

Review of attachment 750942 [details] [diff] [review]:
-----------------------------------------------------------------

have you ensured this works on android and desktop as well.  A try server should be able to help :)

Comment 20

[Martijn Wargers (dead)]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=cf22afdee0e9

Comment 21

[Martijn Wargers (dead)]

Tryserver green, adding checkin-needed keyword.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/321c5d4f88aa

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/321c5d4f88aa