Bug 190595 – window.navigator should not become null after the window loses its browsing context

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 352391 [details]
Patch

[Chris Dumez]

Created attachment 352399 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 352458 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 352458 [details]
Patch

Clearing flags on attachment: 352458

Committed r237185: <https://trac.webkit.org/changeset/237185>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45308081>

[Darin Adler]

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.

[Chris Dumez]

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

[Chris Dumez]

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.

[Darin Adler]

Great. Sounds like everything was fine and I just didn’t see explanation in change log.