Closed Bug 859401 Opened 7 years ago Closed 7 years ago

pushPermissions missing PROMPT_ACTION case


(Testing :: Mochitest, defect)

Not set


(Not tracked)



(Reporter: dchanm+bugzilla, Assigned: martijn.martijn)




(1 file, 5 obsolete files)

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.
Attached patch add prompt_action case (obsolete) — Splinter Review
Try push
Assignee: nobody → dchan+bugzilla
Attachment #734814 - Flags: review?(jmaher)
Correct try run here, previous one was only for opt builds
Apparently I suck at managing mercurial queues and using try. Actual try push... that has the patch and runs the correct tests.

Sorry for the bug spam
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:

As well as the cleanup stuff here:

I would also like to make sure we are sending the right values to the permissions manager.
Attachment #734814 - Flags: review?(jmaher) → review-
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.
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?
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?

[1] -
Flags: needinfo?(jonas)
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.
Flags: needinfo?(jonas)
Blocks: 868439
maybe we are fine with this patch as per comment #8 ?
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #750142 - Flags: review?(jmaher)
Comment on attachment 750142 [details] [diff] [review]

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?
Attachment #750142 - Flags: review?(jmaher) → review-
Attached patch patch (obsolete) — Splinter Review
Yeah, thanks, that works well.
Attachment #750142 - Attachment is obsolete: true
Attachment #750566 - Flags: review?(jmaher)
Comment on attachment 750566 [details] [diff] [review]

This is not working in b2g, so I'll use SpecialPowers.addPermission
Attachment #750566 - Flags: review?(jmaher)
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.
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #750566 - Attachment is obsolete: true
Attachment #750608 - Flags: review?(jmaher)
Attachment #750608 - Flags: review?(jmaher)
Attached patch patch (obsolete) — Splinter Review
Attachment #750608 - Attachment is obsolete: true
Attachment #750614 - Flags: review?(jmaher)
Comment on attachment 750614 [details] [diff] [review]

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

this seems a lot better, thanks for adding this into the system.
Attachment #750614 - Flags: review?(jmaher) → review+
Assignee: dchan+bugzilla → nobody
I removed some code in the test file that I forgot to remove:
var ioService = SpecialPowers.Cc[";1"]

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.
Attachment #734814 - Attachment is obsolete: true
Attachment #750614 - Attachment is obsolete: true
Attachment #750942 - Flags: review?(jmaher)
Assignee: nobody → martijn.martijn
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 :)
Attachment #750942 - Flags: review?(jmaher) → review+
Tryserver green, adding checkin-needed keyword.
Keywords: checkin-needed
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.