Bug 236952 – Permission API: handle non-fully active documents

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Marcos Caceres]

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.

[Marcos Caceres]

Created attachment 452717 [details]
Patch

[EWS Watchlist]

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

[youenn fablet]

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?

[Marcos Caceres]

(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?

[youenn fablet]

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?

[Marcos Caceres]

Created attachment 452777 [details]
Patch

[Marcos Caceres]

(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.

[youenn fablet]

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

[Marcos Caceres]

Created attachment 452832 [details]
Patch

[youenn fablet]

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.

[Marcos Caceres]

Created attachment 452834 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/89288354>