Bug 236952 - Permission API: handle non-fully active documents
Summary: Permission API: handle non-fully active documents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: https://github.com/w3c/permissions/pu...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-20 23:09 PST by Marcos Caceres
Modified: 2022-02-22 05:37 PST (History)
4 users (show)

See Also:


Attachments
Patch (8.67 KB, patch)
2022-02-21 00:52 PST, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (8.82 KB, patch)
2022-02-21 14:48 PST, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (9.00 KB, patch)
2022-02-21 23:58 PST, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (8.97 KB, patch)
2022-02-22 00:14 PST, Marcos Caceres
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcos Caceres 2022-02-20 23:09:37 PST
When calling query() from a a non-fully active document, it should rejects with a InvalidStateError. 

Similarly, non-fully active documents shouldn't receive events.
Comment 1 Marcos Caceres 2022-02-21 00:52:38 PST
Created attachment 452717 [details]
Patch
Comment 2 EWS Watchlist 2022-02-21 00:54:18 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 3 youenn fablet 2022-02-21 01:07:40 PST
Comment on attachment 452717 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452717&action=review

> Source/WebCore/Modules/permissions/Permissions.cpp:88
> +    if (is<Document>(context) && !downcast<Document>(*context).isFullyActive()) {

https://w3c.github.io/permissions/#query-method is not clear about this check, is there a corresponding PR that should be merged?
Are other browsers already implementing this check?

> Source/WebCore/Modules/permissions/Permissions.cpp:94
>          promise.reject(Exception { InvalidStateError, "The context is invalid"_s });

Maybe we should do this check before the fully active one.

> LayoutTests/imported/w3c/ChangeLog:10
> +        * web-platform-tests/permissions/resources/empty.html: Added.

Are these new WPT tests that need to be upstreamed or are they already merged?

> LayoutTests/imported/w3c/web-platform-tests/permissions/non-fully-active.https-expected.txt:6
> +TIMEOUT Permission change events shouldn't fire on non-fully active document Test timed out

We usually do not like tests to timeout. Can the test be updated to check for onchange and fail early if not present?
Comment 4 Marcos Caceres 2022-02-21 01:10:09 PST
(In reply to youenn fablet from comment #3)
> Comment on attachment 452717 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452717&action=review
> 
> > Source/WebCore/Modules/permissions/Permissions.cpp:88
> > +    if (is<Document>(context) && !downcast<Document>(*context).isFullyActive()) {
> 
> https://w3c.github.io/permissions/#query-method is not clear about this
> check, is there a corresponding PR that should be merged?

Sorry I should have pasted the link:
https://github.com/w3c/permissions/pull/365

> Are other browsers already implementing this check?

See links to other implementations. 
 
> > Source/WebCore/Modules/permissions/Permissions.cpp:94
> >          promise.reject(Exception { InvalidStateError, "The context is invalid"_s });
> 
> Maybe we should do this check before the fully active one.
> 
> > LayoutTests/imported/w3c/ChangeLog:10
> > +        * web-platform-tests/permissions/resources/empty.html: Added.
> 
> Are these new WPT tests that need to be upstreamed or are they already
> merged?

PR at https://github.com/web-platform-tests/wpt/pull/32921

> > LayoutTests/imported/w3c/web-platform-tests/permissions/non-fully-active.https-expected.txt:6
> > +TIMEOUT Permission change events shouldn't fire on non-fully active document Test timed out
> 
> We usually do not like tests to timeout. Can the test be updated to check
> for onchange and fail early if not present?
Comment 5 youenn fablet 2022-02-21 03:48:07 PST
OK, this sounds good to me, a few nits then:
- Can we mention the PRS in the change log?
- Can we update the test to fail early instead of timing out?
- Can we check that there is a context before checking whether active?
Comment 6 Marcos Caceres 2022-02-21 14:48:46 PST
Created attachment 452777 [details]
Patch
Comment 7 Marcos Caceres 2022-02-21 14:49:30 PST
(In reply to youenn fablet from comment #5)
> OK, this sounds good to me, a few nits then:
> - Can we mention the PRS in the change log?

Done.

> - Can we update the test to fail early instead of timing out?

Done.

> - Can we check that there is a context before checking whether active?

Done. And thanks! I was wondering about this also.
Comment 8 youenn fablet 2022-02-21 22:54:30 PST
Comment on attachment 452777 [details]
Patch

LGTM, a few comments below.

View in context: https://bugs.webkit.org/attachment.cgi?id=452777&action=review

> Source/WebCore/Modules/permissions/PermissionStatus.cpp:75
> +        if (!document.isFullyActive())

We have the new dynamicDowncast we could use:
if (auto* document = dynamicDowncast<Document>(context); !document->isFullyActive())
    return;

Also, this makes the case of iframes detached-then-reattached potentially complex to handle for developers.
Say we query permission before detaching (and get granted), we detach, we change permission to prompt, we reattach the frame and we query permission again.
The two permission objects would have different state. This seems somewhat inconvenient for web developers (although simpler for implementors).
There could be a case where web page hangs on the first permission object state.

> Source/WebCore/Modules/permissions/Permissions.cpp:92
> +    if (is<Document>(context) && !downcast<Document>(*context).isFullyActive()) {

Ditto for dynamicDowncast
Comment 9 Marcos Caceres 2022-02-21 23:58:44 PST
Created attachment 452832 [details]
Patch
Comment 10 youenn fablet 2022-02-22 00:03:44 PST
Comment on attachment 452832 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452832&action=review

> Source/WebCore/Modules/permissions/PermissionStatus.cpp:72
> +    auto* context = scriptExecutionContext();

Not really needed

> Source/WebCore/Modules/permissions/PermissionStatus.cpp:73
> +    if (auto document = dynamicDowncast<Document>(context); !document->isFullyActive())

I would use auto* to match Permissions::query.
Comment 11 Marcos Caceres 2022-02-22 00:14:18 PST
Created attachment 452834 [details]
Patch
Comment 12 EWS 2022-02-22 05:36:23 PST
Committed r290301 (247624@main): <https://commits.webkit.org/247624@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452834 [details].
Comment 13 Radar WebKit Bug Importer 2022-02-22 05:37:17 PST
<rdar://problem/89288354>