REOPENED 228319
Geolocation API should callback with error if doc is not fully active 
https://bugs.webkit.org/show_bug.cgi?id=228319
Summary Geolocation API should callback with error if doc is not fully active
Marcos Caceres
Reported 2021-07-27 01:26:51 PDT
When the document is not fully active the error callback passed to navigator.geolocation.getCurrentPosition() or watchPosition() is not invoked. This behavior can be reproduced by creating an iframe, capturing a reference to navigator.geolocation from its global scope, removing it from the DOM and then attempting to call one of these methods. The behavior of calling the error callback has been proposed in the specification. Specification issue: https://github.com/w3c/geolocation-api/issues/96 Specification pull request: https://github.com/w3c/geolocation-api/pull/97 Web platform test: https://github.com/web-platform-tests/wpt/pull/29799 PS: any chance of getting a "Geolocation" bug component in here in the bug tracker? 🙏
Attachments
WIP patch (10.60 KB, patch)
2021-08-23 17:12 PDT, Chris Dumez 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
WIP patch (19.35 KB, patch)
2021-08-24 09:17 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (25.49 KB, patch)
2021-08-24 09:42 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (25.89 KB, patch)
2021-08-24 11:50 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (8.75 KB, patch)
2022-01-19 07:15 PST, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2021-08-03 01:27:21 PDT
<rdar://problem/81450315>
Chris Dumez
Comment 2 2021-08-23 17:00:22 PDT
There are quite a few issues causing the new test not to run, besides Geolocation issues: - Lack of a <body> even though the test relies on document.body - After re-adding the iframe to the document, it does: ``` document.body.appendChild(iframe); iframe.contentWindow.opener = window; await new Promise((resolve) => window.addEventListener("load", resolve, { once: true }) ); ``` I think this should be: ``` document.body.appendChild(iframe); iframe.contentWindow.opener = window; await new Promise((resolve) => iframe.addEventListener("load", resolve, { once: true }) ); ``` Why would the main frame's load event fire again?
Chris Dumez
Comment 3 2021-08-23 17:07:48 PDT
(In reply to Chris Dumez from comment #2) > There are quite a few issues causing the new test not to run, besides > Geolocation issues: > - Lack of a <body> even though the test relies on document.body > - After re-adding the iframe to the document, it does: > ``` > document.body.appendChild(iframe); > iframe.contentWindow.opener = window; > await new Promise((resolve) => > window.addEventListener("load", resolve, { once: true }) > ); > ``` > > I think this should be: > ``` > document.body.appendChild(iframe); > iframe.contentWindow.opener = window; > await new Promise((resolve) => > iframe.addEventListener("load", resolve, { once: true }) > ); > ``` > > Why would the main frame's load event fire again? And I think we need this test change too: - assert_true(position, "Expected a position"); + assert_true(Boolean(position), "Expected a position");
Chris Dumez
Comment 4 2021-08-23 17:12:56 PDT
Created attachment 436254 [details] WIP patch
EWS Watchlist
Comment 5 2021-08-23 17:14:17 PDT
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
Chris Dumez
Comment 6 2021-08-23 17:16:00 PDT
The C++ side should be good. More work is needed to either make the WPT test run nicely with our infra or write our own test.
Marcos Caceres
Comment 7 2021-08-23 18:49:27 PDT
(In reply to Chris Dumez from comment #2) > There are quite a few issues causing the new test not to run, besides > Geolocation issues: > - Lack of a <body> even though the test relies on document.body > - After re-adding the iframe to the document, it does: > ``` > document.body.appendChild(iframe); > iframe.contentWindow.opener = window; > await new Promise((resolve) => > window.addEventListener("load", resolve, { once: true }) > ); > ``` > > I think this should be: > ``` > document.body.appendChild(iframe); > iframe.contentWindow.opener = window; > await new Promise((resolve) => > iframe.addEventListener("load", resolve, { once: true }) > ); > ``` > > Why would the main frame's load event fire again? Oops! Good catch! Let me know if you want me to fix this over on the WTP side.
Marcos Caceres
Comment 8 2021-08-23 18:58:22 PDT
Sent fix for WPT: https://github.com/web-platform-tests/wpt/pull/30143 Happy to fix anything else you spot.
Chris Dumez
Comment 9 2021-08-24 09:17:12 PDT
Created attachment 436293 [details] WIP patch
Chris Dumez
Comment 10 2021-08-24 09:42:01 PDT
Created attachment 436297 [details] Patch
Chris Dumez
Comment 11 2021-08-24 11:50:49 PDT
Created attachment 436315 [details] Patch
EWS
Comment 12 2021-08-24 15:03:41 PDT
Committed r281520 (240893@main): <https://commits.webkit.org/240893@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436315 [details].
Chris Dumez
Comment 13 2022-01-14 12:20:39 PST
Reopening since the change was reverted.
Alexey Shvayka
Comment 14 2022-01-19 07:15:36 PST
Created attachment 449481 [details] Patch Re-land on top of r288197.
Alexey Shvayka
Comment 15 2022-01-19 11:27:15 PST
Comment on attachment 449481 [details] Patch https://bugs.webkit.org/show_bug.cgi?id=235368 needs to be investigated first.
Alexey Shvayka
Comment 16 2022-01-27 15:01:21 PST
Comment on attachment 449481 [details] Patch r288307 seems alright.
EWS
Comment 17 2022-01-27 15:25:33 PST
Committed r288707 (246504@main): <https://commits.webkit.org/246504@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449481 [details].
Marcos Caceres
Comment 18 2022-12-04 20:50:08 PST
Reopening as it has regressed: the callbacks are no longer called.
Note You need to log in before you can comment on or make changes to this bug.