1197461 - Implement Permissions.revoke

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Biru [:poiru]]

This is now part of the spec. We should implement it.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: permissions_revoke

Comment 2

[Marco Castelluccio [:marco]]

Attachment: permissions_revoke_tests

Comment 3

[Biru [:poiru]]

Comment on attachment 8716134 [details] [diff] [review]
permissions_revoke

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

::: dom/permission/Permissions.cpp
@@ +156,5 @@
> +    aRv.Throw(rv);
> +    return nullptr;
> +  }
> +
> +  RefPtr<Promise> promise = Promise::Create(global, aRv);

Could you do this as early as possible and change the code above to return rejected promise instead of `nullptr`?

Comment 4

[Biru [:poiru]]

Comment on attachment 8716135 [details] [diff] [review]
permissions_revoke_tests

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

Thanks for doing this!

Comment 5

[Marco Castelluccio [:marco]]

(In reply to Birunthan Mohanathas [:poiru] from comment #3)
> Comment on attachment 8716134 [details] [diff] [review]
> permissions_revoke
> 
> Review of attachment 8716134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/permission/Permissions.cpp
> @@ +156,5 @@
> > +    aRv.Throw(rv);
> > +    return nullptr;
> > +  }
> > +
> > +  RefPtr<Promise> promise = Promise::Create(global, aRv);
> 
> Could you do this as early as possible and change the code above to return
> rejected promise instead of `nullptr`?

From the spec it was unclear to me when the function should throw and when it should
reject the promise.
I'll update the patch.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: permissions_revoke

Comment 7

[Marco Castelluccio [:marco]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=765d3d89dd58

Comment 8

[Marco Castelluccio [:marco]]

> Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...

Who else should review the patch?

Comment 9

[Biru [:poiru]]

Comment on attachment 8716895 [details] [diff] [review]
permissions_revoke

baku, can you review (at least) the WebIDL change?

Comment 10

[Marco Castelluccio [:marco]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c046a81ab5a373e8ce2c2a8c16aca43eedf4914f
Bug 1197461 - Implement Permissions.revoke. r=poiru,baku

https://hg.mozilla.org/integration/mozilla-inbound/rev/d776c61906c17234f60c8df54ed4f913959eb5eb
Bug 1197461 - Tests for Permissions.revoke. r=poiru,baku

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for various test failures in 

Push with failing jobs: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d776c61906c1
E.g. Windows 8 x64 opt failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=21721604&repo=mozilla-inbound

10:18:48     INFO -  127 INFO TEST-START | dom/permission/tests/test_permissions_api.html
10:18:49     INFO -  TEST-INFO | started process screenshot
10:18:49     INFO -  TEST-INFO | screenshot: exit 0
10:18:49     INFO -  128 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | query should have thrown TypeError for 'foobarbaz'
10:18:49     INFO -  129 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | query should have thrown TypeError for 'midi'
10:18:49     INFO -  130 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | query should have rejected for userVisible push
10:18:49     INFO -  131 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'geolocation'
10:18:49     INFO -  132 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'notifications'
10:18:49     INFO -  133 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'push'
10:18:49     INFO -  134 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'geolocation'
10:18:49     INFO -  135 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'notifications'
10:18:49     INFO -  136 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'push'
10:18:49     INFO -  137 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'geolocation'
10:18:49     INFO -  138 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'notifications'
10:18:49     INFO -  139 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'push'
10:18:49     INFO -  140 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'geolocation'
10:18:49     INFO -  141 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'notifications'
10:18:49     INFO -  142 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | correct state for 'push'
10:18:49     INFO -  143 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | state changed for 'geolocation'
10:18:49     INFO -  144 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | state changed for 'geolocation'
10:18:49     INFO -  145 INFO TEST-PASS | dom/permission/tests/test_permissions_api.html | invalid query should have rejected
10:18:49     INFO -  146 INFO TEST-UNEXPECTED-FAIL | dom/permission/tests/test_permissions_api.html | revoke should have thrown TypeError for 'foobarbaz' - got "NS_ERROR_UNEXPECTED", expected "TypeError"
10:18:49     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
10:18:49     INFO -      revokeUnsupportedPermissions/</<@dom/permission/tests/test_permissions_api.html:57:16
10:18:49     INFO -      Promise*revokeUnsupportedPermissions/<@dom/permission/tests/test_permissions_api.html:55:5
10:18:49     INFO -      revokeUnsupportedPermissions@dom/permission/tests/test_permissions_api.html:54:22
10:18:49     INFO -      promise callback*runTests@dom/permission/tests/test_permissions_api.html:134:3
10:18:49     INFO -      onload@dom/permission/tests/test_permissions_api.html:1:1
10:18:49     INFO -  Not taking screenshot here: see the one that was previously logged
10:18:49     INFO -  147 INFO TEST-UNEXPECTED-FAIL | dom/permission/tests/test_permissions_api.html | revoke should have thrown TypeError for 'midi' - got "NS_ERROR_UNEXPECTED", expected "TypeError"
10:18:49     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
10:18:49     INFO -      revokeUnsupportedPermissions/</<@dom/permission/tests/test_permissions_api.html:57:16
10:18:49     INFO -      Promise*revokeUnsupportedPermissions/<@dom/permission/tests/test_permissions_api.html:55:5
10:18:49     INFO -      revokeUnsupportedPermissions@dom/permission/tests/test_permissions_api.html:54:22
10:18:49     INFO -      promise callback*runTests@dom/permission/tests/test_permissions_api.html:134:3
10:18:49     INFO -      onload@dom/permission/tests/test_permissions_api.html:1:1
10:18:49     INFO -  Not taking screenshot here: see the one that was previously logged
10:18:49     INFO -  148 INFO TEST-UNEXPECTED-FAIL | dom/permission/tests/test_permissions_api.html | Unexpected error TypeError: 'name' member of PermissionDescriptor 'midi' is not a valid value for enumeration PermissionName.
10:18:49     INFO -      runTests/<@dom/permission/tests/test_permissions_api.html:153:7
10:18:49     INFO -      promise callback*runTests@dom/permission/tests/test_permissions_api.html:134:3
10:18:49     INFO -      onload@dom/permission/tests/test_permissions_api.html:1:1

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24876669d491

Comment 13

[Marco Castelluccio [:marco]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1aaff5d559f

Comment 14

[Marco Castelluccio [:marco]]

There was a failure caused by the latest changes to the patch, I've had to move the call to PermissionDescriptor::init() before creating the promise (an assertion was failing because there was a pending exception).

Comment 15

[Marco Castelluccio [:marco]]

Attachment: permissions_revoke

Comment 16

[Marco Castelluccio [:marco]]

Unfortunately there's still a failure on e10s

> [Child 19092] ###!!! ASSERTION: Cannot perform action in content process!: 'Error', file extensions/cookie/nsPermissionManager.cpp, line 1813

http://hg.mozilla.org/mozilla-central/file/709f559b5406/extensions/cookie/nsPermissionManager.cpp#l1813

Comment 17

[Marco Castelluccio [:marco]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75b24125b054

Comment 18

[Marco Castelluccio [:marco]]

Attachment: permissions_revoke_e10s

Comment 19

[Marco Castelluccio [:marco]]

Attachment: permissions_revoke_e10s

Comment 20

[Biru [:poiru]]

Comment on attachment 8725680 [details] [diff] [review]
permissions_revoke_e10s

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

::: dom/permission/Permissions.cpp
@@ +179,5 @@
> +    // Permissions can't be removed from the content process. Send a message
> +    // to the parent; `ContentParent::RecvRemovePermission` will call
> +    // `RemovePermission`.
> +    ContentChild::GetSingleton()->SendRemovePermission(
> +      IPC::Principal(document->NodePrincipal()), nsDependentCString(permissionType), &rv);

Ideally I think this should be async (note that we would need to resolve the promise after the async message has been handled). I'm fine this being synchronous for now, but we may have to revisit this when we add worker support.

Comment 21

[Marco Castelluccio [:marco]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e978b825517d6d06d1071d179d5f0d89a069733c
Bug 1197461 - Implement Permissions.revoke. r=poiru,baku

https://hg.mozilla.org/integration/mozilla-inbound/rev/59b8d765123ce48d13db01a00a3d3fc628ad67e8
Bug 1197461 - e10s support for Permissions.revoke. r=poiru

https://hg.mozilla.org/integration/mozilla-inbound/rev/990eeaa39c299b43c06b5b9b04951c8cf8b8b421
Bug 1197461 - Tests for Permissions.revoke. r=poiru,baku

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e978b825517d
https://hg.mozilla.org/mozilla-central/rev/59b8d765123c
https://hg.mozilla.org/mozilla-central/rev/990eeaa39c29

Comment 23

[Chris Mills [:cmills]]

I've added this method to the MDN docs, including entries and browser compat listings on all related pages:

https://developer.mozilla.org/en-US/docs/Web/API/Permissions_API
https://developer.mozilla.org/en-US/docs/Web/API/Permissions
https://developer.mozilla.org/en-US/docs/Web/API/Permissions/revoke

I also added a new feature to my simple demo to show revoke() in use:

https://chrisdavidmills.github.io/location-finder-permissions-api/
https://developer.mozilla.org/en-US/docs/Web/API/Permissions_API/Using_the_Permissions_API

I've also updated the relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/47#Others

Two questions:

1. Am I using the method wrong in my demo? The state always seems to return "prompt", even after I've revoked the permission.
2. Have I written a reasonable description of when the TypeError exception occurs (see the revoke page, above)? I wasn't sure I'd got this right.

A tech review of the new content would be great, when you get a moment. Thanks!

Comment 24

[Mounir Lamouri (:mounir)]

Regarding your first question, Chris, this is correct. permissions.revoke() will revert the permission to its default state if it was previously granted. Usually, the default state is "prompt" :)

Comment 25

[Chris Mills [:cmills]]

(In reply to Mounir Lamouri (:mounir) from comment #24)
> Regarding your first question, Chris, this is correct. permissions.revoke()
> will revert the permission to its default state if it was previously
> granted. Usually, the default state is "prompt" :)

Great, thanks for the clarification Mounir! I've updated my descriptions a bit in light of this.

Comment 26

[Marco Castelluccio [:marco]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #23)
> 2. Have I written a reasonable description of when the TypeError exception
> occurs (see the revoke page, above)? I wasn't sure I'd got this right.

Sounds good to me.
The exception will be thrown also if the permission doesn't exist or is unsupported (e.g. 'midi', or 'push' with userVisibleOnly).
The same is true for Permissions.query().

On https://developer.mozilla.org/en-US/docs/Web/API/Permissions/revoke, the URL to the specification is wrong (https://w3c.github.io/permissions/https://w3c.github.io/permissions/#dom-permissions-revoke), I don't know if it's wrong in other pages as well.

Comment 27

[Chris Mills [:cmills]]

(In reply to Marco Castelluccio [:marco] from comment #26)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #23)
> > 2. Have I written a reasonable description of when the TypeError exception
> > occurs (see the revoke page, above)? I wasn't sure I'd got this right.
> 
> Sounds good to me.
> The exception will be thrown also if the permission doesn't exist or is
> unsupported (e.g. 'midi', or 'push' with userVisibleOnly).
> The same is true for Permissions.query().

Ah, cool - thanks. I've updated the exceptions parts of those pages.

> On https://developer.mozilla.org/en-US/docs/Web/API/Permissions/revoke, the
> URL to the specification is wrong
> (https://w3c.github.io/permissions/https://w3c.github.io/permissions/#dom-
> permissions-revoke), I don't know if it's wrong in other pages as well.

Hrm, don't know how I managed to get that wrong. Thanks for the fix ;-)

Comment 28

[Jeffrey Yasskin]

The specification isn't clear about what happens when you have, for example, {name:"midi", sysex:false} permission and you try to revoke {name:"midi", sysex:true}. What does Firefox's implementation do? How about the opposite set of sysex values?