Bug 190595

Summary: window.navigator should not become null after the window loses its browsing context
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, cdumez, commit-queue, darin, ews-watchlist, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 190539    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews204 for win-future
none
Patch none

Chris Dumez
Reported 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.
Attachments
Patch (20.19 KB, patch)
2018-10-15 15:25 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (19.87 KB, patch)
2018-10-15 16:22 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2018-10-15 15:25:41 PDT
Created attachment 352391 [details] Patch
Chris Dumez
Comment 2 2018-10-15 16:22:36 PDT
Created attachment 352399 [details] Patch
EWS Watchlist
Comment 3 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
EWS Watchlist
Comment 4 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
Chris Dumez
Comment 5 2018-10-16 08:43:18 PDT
Created attachment 352458 [details] Patch
WebKit Commit Bot
Comment 6 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>
WebKit Commit Bot
Comment 7 2018-10-16 09:24:21 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 8 2018-10-16 09:25:33 PDT
<rdar://problem/45308081>
Darin Adler
Comment 9 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.
Chris Dumez
Comment 10 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
Chris Dumez
Comment 11 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.
Darin Adler
Comment 12 2018-10-29 09:37:26 PDT
Great. Sounds like everything was fine and I just didn’t see explanation in change log.
Note You need to log in before you can comment on or make changes to this bug.