Callback with error if doc is not fully active #97

marcoscaceres · 2021-07-26T03:14:15Z

Closes #96

The following tasks have been completed:

Modified Web platform tests

Implementation commitment:

WebKit (https://bugs.webkit.org/show_bug.cgi?id=228319)
Chromium (https://bugs.chromium.org/p/chromium/issues/detail?id=1233329)
Gecko https://phabricator.services.mozilla.com/D117273

Preview | Diff

marcoscaceres · 2021-07-26T13:15:48Z

Gecko patch: https://phabricator.services.mozilla.com/D117273

reillyeon · 2021-07-27T01:01:52Z

I've filed an issue against Chromium for this and updated the summary. Any chance the test you've added in the Gecko patch could be added to WPT instead?

reillyeon

This is one of the first instances I've seen where the behavior of calling a method on a non-fully active document is well-defined. It is something I always need to handle in Chromium (because non-fully active documents in Blink have a rather conspicuous null pointer) but haven't ever specified.

marcoscaceres · 2021-07-27T07:30:26Z

Thanks @reillyeon! And I again have to give a huge shout out to @rakina and co. for producing that TAG guide and for her guidance - and to @saschanaz for a super detailed review over on the Gecko side, who caught the potential issue when used with promises.

I'll see if I can create a WPT test. The challenge I've been facing is that the page requires permission to use the API. But I'll see if Webdriver, via WPT, lets me grant a page permission to use the API... Oh! might be in luck!

 await test_driver.set_permission({name: 'geolocation'}, 'granted');

Re non fully active in general: yeah, I've been finding really weird behavior... not sure if you are following the Permission's one, where (unsurprisingly) Chrome and Firefox are producing different results:

w3c/permissions#162 (comment)

These are a lot of fun to work on tho! So please ping if there are other APIs that could use a look over.

marcoscaceres · 2021-07-27T08:22:40Z

Ok, WPT is up web-platform-tests/wpt#29799 - however, I'm unsure who to tag for review? @reillyeon can you suggest anyone (or review it?).

That reminds me... we need to find new folks to review the tests. The current set listed over at WPT don't appear to be very active in this space as they might have moved onto other things.

marcoscaceres · 2021-07-27T08:27:40Z

Filed WebKit bug https://bugs.webkit.org/show_bug.cgi?id=228319

saschanaz · 2021-07-27T10:41:21Z

Ok, WPT is up web-platform-tests/wpt#29799 - however, I'm unsure who to tag for review? @reillyeon can you suggest anyone (or review it?).

That reminds me... we need to find new folks to review the tests. The current set listed over at WPT don't appear to be very active in this space as they might have moved onto other things.

It can be included the gecko patch so that it can be automatically upstreamed.

marcoscaceres · 2021-07-27T22:27:37Z

@saschanaz wrote:

It can be included the gecko patch so that it can be automatically upstreamed.

I was hoping to do that, but Gecko doesn't implement test_driver.set_permission() yet 😢. I could still bundle it in, if you think it's still worth while.

saschanaz · 2021-07-27T22:35:59Z

Oops!

@jgraham Are we planning to implement test_driver.set_permission() in the foreseeable future?

… not fully active r=saschanaz Spec change w3c/geolocation#97 Differential Revision: https://phabricator.services.mozilla.com/D117273

marcoscaceres · 2021-08-24T21:58:21Z

Good news! WebKit patch inbound for this too! https://bugs.webkit.org/show_bug.cgi?id=228319 🎉

https://bugs.webkit.org/show_bug.cgi?id=228319 <rdar://problem/81450315> Reviewed by Ryosuke Niwa. LayoutTests/imported/w3c: * web-platform-tests/geolocation-API/PositionOptions.https-expected.txt: Rebaseline WPT test now that more checks are passing. * web-platform-tests/geolocation-API/non-fully-active.https-expected.txt: Added. * web-platform-tests/geolocation-API/non-fully-active.https.html: Added. * web-platform-tests/geolocation-API/resources/iframe.html: Added. Import test coverage from WPT web-platform-tests/wpt#29799. * web-platform-tests/resources/testdriver-vendor.js: (window.test_driver_internal.set_permission): Add support for test_driver.set_permission("geolocation", "granted") so that Geolocation WPT tests can run with our infrastructure. Source/WebCore: Test: imported/w3c/web-platform-tests/geolocation-API/non-fully-active.https.html * Modules/geolocation/Geolocation.cpp: (WebCore::Geolocation::getCurrentPosition): (WebCore::Geolocation::watchPosition): Schedule a task to call the error callback if the document is not fully active, as per the specification: - w3c/geolocation#96 - w3c/geolocation#97 * bindings/js/JSDOMConvertCallbacks.h: (WebCore::Converter<IDLCallbackFunction<T>>::convert): (WebCore::Converter<IDLCallbackInterface<T>>::convert): Make sure we use the incumbent global object when constructing callback functions / interfaces, as per the Web IDL specification: - https://heycam.github.io/webidl/#es-callback-interface - https://heycam.github.io/webidl/#es-callback-function Without this, the geolocation API would be unable to call its error callback when in a detached frame because the callback's global object would be the geolocation object's global object. * dom/IdleCallbackController.cpp: (WebCore::IdleCallbackController::invokeIdleCallbacks): Make sure we don't fire requestIdleCallback callbacks in detached iframes, to maintain pre-existing behavior and keep layout tests passing. I had to make this change because callback interfaces / functions are now using a different global object and can now get called in cases when they previously couldn't. * dom/TaskSource.h: As Geolocation task source for the HTML5 event loop, as per the Geolocation specification. LayoutTests: Update / rebaseline existing layout tests to reflect behavior change. * fast/dom/Geolocation/callback-to-deleted-context-expected.txt: * fast/dom/Geolocation/callback-to-deleted-context.html: * fast/dom/Geolocation/disconnected-frame-already-expected.txt: * fast/dom/Geolocation/disconnected-frame-already.html: * fast/dom/Geolocation/disconnected-frame-expected.txt: * fast/dom/Geolocation/disconnected-frame-permission-denied-expected.txt: * fast/dom/Geolocation/disconnected-frame-permission-denied.html: * fast/dom/Geolocation/disconnected-frame.html: * fast/dom/Geolocation/resources/callback-to-deleted-context-inner1.html: * fast/events/detached-svg-parent-window-events-expected.txt: * fast/events/detached-svg-parent-window-events.html: Canonical link: https://commits.webkit.org/240893@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@281520 268f45cc-cd09-0410-ab3c-d52691b4dbfc

marcoscaceres · 2021-08-24T22:04:26Z

@reillyeon, could you by chance point me to how fully active checks are done in Chromium? With a little guidance, maybe I can slap together a Chromium patch? ☺️

reillyeon · 2021-08-24T22:11:45Z

I left notes on how to do this on https://bugs.chromium.org/p/chromium/issues/detail?id=1233329.

https://bugs.webkit.org/show_bug.cgi?id=228319 <rdar://problem/81450315> Reviewed by Ryosuke Niwa. LayoutTests/imported/w3c: * web-platform-tests/geolocation-API/PositionOptions.https-expected.txt: Rebaseline WPT test now that more checks are passing. * web-platform-tests/geolocation-API/non-fully-active.https-expected.txt: Added. * web-platform-tests/geolocation-API/non-fully-active.https.html: Added. * web-platform-tests/geolocation-API/resources/iframe.html: Added. Import test coverage from WPT web-platform-tests/wpt#29799. * web-platform-tests/resources/testdriver-vendor.js: (window.test_driver_internal.set_permission): Add support for test_driver.set_permission("geolocation", "granted") so that Geolocation WPT tests can run with our infrastructure. Source/WebCore: Test: imported/w3c/web-platform-tests/geolocation-API/non-fully-active.https.html * Modules/geolocation/Geolocation.cpp: (WebCore::Geolocation::getCurrentPosition): (WebCore::Geolocation::watchPosition): Schedule a task to call the error callback if the document is not fully active, as per the specification: - w3c/geolocation#96 - w3c/geolocation#97 * bindings/js/JSDOMConvertCallbacks.h: (WebCore::Converter<IDLCallbackFunction<T>>::convert): (WebCore::Converter<IDLCallbackInterface<T>>::convert): Make sure we use the incumbent global object when constructing callback functions / interfaces, as per the Web IDL specification: - https://heycam.github.io/webidl/#es-callback-interface - https://heycam.github.io/webidl/#es-callback-function Without this, the geolocation API would be unable to call its error callback when in a detached frame because the callback's global object would be the geolocation object's global object. * dom/IdleCallbackController.cpp: (WebCore::IdleCallbackController::invokeIdleCallbacks): Make sure we don't fire requestIdleCallback callbacks in detached iframes, to maintain pre-existing behavior and keep layout tests passing. I had to make this change because callback interfaces / functions are now using a different global object and can now get called in cases when they previously couldn't. * dom/TaskSource.h: As Geolocation task source for the HTML5 event loop, as per the Geolocation specification. LayoutTests: Update / rebaseline existing layout tests to reflect behavior change. * fast/dom/Geolocation/callback-to-deleted-context-expected.txt: * fast/dom/Geolocation/callback-to-deleted-context.html: * fast/dom/Geolocation/disconnected-frame-already-expected.txt: * fast/dom/Geolocation/disconnected-frame-already.html: * fast/dom/Geolocation/disconnected-frame-expected.txt: * fast/dom/Geolocation/disconnected-frame-permission-denied-expected.txt: * fast/dom/Geolocation/disconnected-frame-permission-denied.html: * fast/dom/Geolocation/disconnected-frame.html: * fast/dom/Geolocation/resources/callback-to-deleted-context-inner1.html: * fast/events/detached-svg-parent-window-events-expected.txt: * fast/events/detached-svg-parent-window-events.html: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@281520 268f45cc-cd09-0410-ab3c-d52691b4dbfc

https://bugs.webkit.org/show_bug.cgi?id=228319 <rdar://problem/81450315> Reviewed by Ryosuke Niwa. LayoutTests/imported/w3c: * web-platform-tests/geolocation-API/non-fully-active.https-expected.txt: Rebaseline WPT test that is now passing. Source/WebCore: Test: imported/w3c/web-platform-tests/geolocation-API/non-fully-active.https.html * Modules/geolocation/Geolocation.cpp: (WebCore::Geolocation::getCurrentPosition): (WebCore::Geolocation::watchPosition): Schedule a task to call the error callback if the document is not fully active, as per the specification: - w3c/geolocation#96 - w3c/geolocation#97 * dom/TaskSource.h: As Geolocation task source for the HTML5 event loop, as per the Geolocation specification. LayoutTests: Update existing layout tests to reflect behavior change. * fast/dom/Geolocation/disconnected-frame-already-expected.txt: * fast/dom/Geolocation/disconnected-frame-already.html: Aligns test assertions with the spec. Gecko also passes it. * TestExpectations: Unskip the WPT test. Canonical link: https://commits.webkit.org/246504@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@288707 268f45cc-cd09-0410-ab3c-d52691b4dbfc

https://bugs.webkit.org/show_bug.cgi?id=228319 <rdar://problem/81450315> Reviewed by Ryosuke Niwa. LayoutTests/imported/w3c: * web-platform-tests/geolocation-API/non-fully-active.https-expected.txt: Rebaseline WPT test that is now passing. Source/WebCore: Test: imported/w3c/web-platform-tests/geolocation-API/non-fully-active.https.html * Modules/geolocation/Geolocation.cpp: (WebCore::Geolocation::getCurrentPosition): (WebCore::Geolocation::watchPosition): Schedule a task to call the error callback if the document is not fully active, as per the specification: - w3c/geolocation#96 - w3c/geolocation#97 * dom/TaskSource.h: As Geolocation task source for the HTML5 event loop, as per the Geolocation specification. LayoutTests: Update existing layout tests to reflect behavior change. * fast/dom/Geolocation/disconnected-frame-already-expected.txt: * fast/dom/Geolocation/disconnected-frame-already.html: Aligns test assertions with the spec. Gecko also passes it. * TestExpectations: Unskip the WPT test. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@288707 268f45cc-cd09-0410-ab3c-d52691b4dbfc

marcoscaceres added 2 commits July 26, 2021 13:13

Callback with error if doc is not fully active

a14276f

fixup

b92947d

marcoscaceres mentioned this pull request Jul 26, 2021

Error when method are called on not fully active docs #96

Closed

marcoscaceres requested a review from reillyeon July 26, 2021 13:15

reillyeon approved these changes Jul 27, 2021

View reviewed changes

marcoscaceres mentioned this pull request Jul 27, 2021

Add non-fully active doc test for Geolocation web-platform-tests/wpt#29799

Merged

marcoscaceres merged commit de05002 into gh-pages Jul 27, 2021

marcoscaceres deleted the fully_active_again branch July 27, 2021 08:35

marcoscaceres mentioned this pull request Aug 12, 2021

Republish as CR #93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Callback with error if doc is not fully active #97

Callback with error if doc is not fully active #97

marcoscaceres commented Jul 26, 2021 •

edited

Loading

marcoscaceres commented Jul 26, 2021

reillyeon commented Jul 27, 2021

reillyeon left a comment

marcoscaceres commented Jul 27, 2021 •

edited

Loading

marcoscaceres commented Jul 27, 2021

marcoscaceres commented Jul 27, 2021

saschanaz commented Jul 27, 2021

marcoscaceres commented Jul 27, 2021

saschanaz commented Jul 27, 2021

marcoscaceres commented Aug 24, 2021

marcoscaceres commented Aug 24, 2021

reillyeon commented Aug 24, 2021

Callback with error if doc is not fully active #97

Callback with error if doc is not fully active #97

Conversation

marcoscaceres commented Jul 26, 2021 • edited Loading

marcoscaceres commented Jul 26, 2021

reillyeon commented Jul 27, 2021

reillyeon left a comment

Choose a reason for hiding this comment

marcoscaceres commented Jul 27, 2021 • edited Loading

marcoscaceres commented Jul 27, 2021

marcoscaceres commented Jul 27, 2021

saschanaz commented Jul 27, 2021

marcoscaceres commented Jul 27, 2021

saschanaz commented Jul 27, 2021

marcoscaceres commented Aug 24, 2021

marcoscaceres commented Aug 24, 2021

reillyeon commented Aug 24, 2021

marcoscaceres commented Jul 26, 2021 •

edited

Loading

marcoscaceres commented Jul 27, 2021 •

edited

Loading