Bug 228319 - Geolocation API should callback with error if doc is not fully active
Summary: Geolocation API should callback with error if doc is not fully active
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari 14
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-27 01:26 PDT by Marcos Caceres
Modified: 2021-08-24 15:03 PDT (History)
13 users (show)

See Also:


Attachments
WIP patch (10.60 KB, patch)
2021-08-23 17:12 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP patch (19.35 KB, patch)
2021-08-24 09:17 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (25.49 KB, patch)
2021-08-24 09:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (25.89 KB, patch)
2021-08-24 11:50 PDT, Chris Dumez
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 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? 🙏
Comment 1 Radar WebKit Bug Importer 2021-08-03 01:27:21 PDT
<rdar://problem/81450315>
Comment 2 Chris Dumez 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?
Comment 3 Chris Dumez 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");
Comment 4 Chris Dumez 2021-08-23 17:12:56 PDT
Created attachment 436254 [details]
WIP patch
Comment 5 EWS Watchlist 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
Comment 6 Chris Dumez 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.
Comment 7 Marcos Caceres 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.
Comment 8 Marcos Caceres 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.
Comment 9 Chris Dumez 2021-08-24 09:17:12 PDT
Created attachment 436293 [details]
WIP patch
Comment 10 Chris Dumez 2021-08-24 09:42:01 PDT
Created attachment 436297 [details]
Patch
Comment 11 Chris Dumez 2021-08-24 11:50:49 PDT
Created attachment 436315 [details]
Patch
Comment 12 EWS 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].