Bug 190595 - window.navigator should not become null after the window loses its browsing context
Summary: window.navigator should not become null after the window loses its browsing c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 190539
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-15 13:00 PDT by Chris Dumez
Modified: 2018-10-29 09:37 PDT (History)
9 users (show)

See Also:


Attachments
Patch (20.19 KB, patch)
2018-10-15 15:25 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (19.87 KB, patch)
2018-10-15 16:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.71 MB, application/zip)
2018-10-15 18:44 PDT, EWS Watchlist
no flags Details
Patch (19.71 KB, patch)
2018-10-16 08:43 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 Chris Dumez 2018-10-15 13:00:11 PDT
window.navigator should not become null after the window loses its browsing context. This WebKit behavior does not match the HTML specification nor the behavior of other browsers.
Comment 1 Chris Dumez 2018-10-15 15:25:41 PDT
Created attachment 352391 [details]
Patch
Comment 2 Chris Dumez 2018-10-15 16:22:36 PDT
Created attachment 352399 [details]
Patch
Comment 3 EWS Watchlist 2018-10-15 18:44:24 PDT
Comment on attachment 352399 [details]
Patch

Attachment 352399 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9604412

New failing tests:
http/tests/dom/same-origin-detached-window-properties.html
Comment 4 EWS Watchlist 2018-10-15 18:44:35 PDT
Created attachment 352420 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 5 Chris Dumez 2018-10-16 08:43:18 PDT
Created attachment 352458 [details]
Patch
Comment 6 WebKit Commit Bot 2018-10-16 09:24:19 PDT
Comment on attachment 352458 [details]
Patch

Clearing flags on attachment: 352458

Committed r237185: <https://trac.webkit.org/changeset/237185>
Comment 7 WebKit Commit Bot 2018-10-16 09:24:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2018-10-16 09:25:33 PDT
<rdar://problem/45308081>
Comment 9 Darin Adler 2018-10-26 20:23:18 PDT
Comment on attachment 352458 [details]
Patch

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

> Source/WebCore/Modules/geolocation/NavigatorGeolocation.cpp:77
> -    if (!m_geolocation && frame())
> -        m_geolocation = Geolocation::create(frame()->document());
> +    if (!m_geolocation)
> +        m_geolocation = Geolocation::create(window() ? window()->document() : nullptr);
>      return m_geolocation.get();

Where’s the test coverage for this change? Also, change log makes no mention of navigator.geolocation nor does it say why this unrelated change is in this patch.
Comment 10 Chris Dumez 2018-10-29 09:04:53 PDT
Comment on attachment 352458 [details]
Patch

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

>> Source/WebCore/Modules/geolocation/NavigatorGeolocation.cpp:77
>>      return m_geolocation.get();
> 
> Where’s the test coverage for this change? Also, change log makes no mention of navigator.geolocation nor does it say why this unrelated change is in this patch.

The change is intentional, not only should window.navigator not be null when the window gets detached, the Navigator object should also look correct (i.e. non null properties). As per the Geolocation spec [1], the geolocation property on the Navigator object is not nullable and therefore can never return null.
Looks like my earlier iteration of this patch contained test coverage for this change but I decided to drop it because it failed on Windows (I am assuming Geolocation is disabled on Windows port?) and I did not want to have to maintain platform-specific test expectations. I will look into adding coverage for this back though given your comment.

[1] https://www.w3.org/TR/geolocation-API/#geolocation_interface
Comment 11 Chris Dumez 2018-10-29 09:06:37 PDT
Comment on attachment 352458 [details]
Patch

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

> LayoutTests/http/tests/dom/same-origin-detached-window-properties-expected.txt:76
> +PASS !!w.navigator.geolocation is true

Oh nevermind, it is navigator.gamepads that I dropped from my latest patch iteration because of Windows. The test coverage for navigator.geolocation is right here.
Comment 12 Darin Adler 2018-10-29 09:37:26 PDT
Great. Sounds like everything was fine and I just didn’t see explanation in change log.