Bug 189375 - document.open() should not propagate URLs to non-fully active documents
Summary: document.open() should not propagate URLs to non-fully active documents
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:
Blocks:
 
Reported: 2018-09-06 14:32 PDT by Timothy Gu
Modified: 2018-09-27 09:32 PDT (History)
12 users (show)

See Also:


Attachments
WIP Patch (5.29 KB, patch)
2018-09-26 12:43 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.69 MB, application/zip)
2018-09-26 13:42 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.47 MB, application/zip)
2018-09-26 14:38 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.16 MB, application/zip)
2018-09-26 14:52 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews200 for win-future (12.90 MB, application/zip)
2018-09-26 14:55 PDT, EWS Watchlist
no flags Details
Patch (10.15 KB, patch)
2018-09-26 15:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.12 KB, patch)
2018-09-27 09:15 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 Timothy Gu 2018-09-06 14:32:13 PDT
In https://github.com/whatwg/html/issues/3885 and https://github.com/whatwg/html/pull/3946, the HTML spec was changed so that URLs are no longer propagated to non-fully active documents during a document.open(), and the URL update becomes more like history.replaceState().

A test is available at https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url.window.js.
Comment 1 Radar WebKit Bug Importer 2018-09-09 13:33:29 PDT
<rdar://problem/44282755>
Comment 2 Chris Dumez 2018-09-26 12:43:55 PDT
Created attachment 350884 [details]
WIP Patch
Comment 3 EWS Watchlist 2018-09-26 13:42:53 PDT
Comment on attachment 350884 [details]
WIP Patch

Attachment 350884 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9359229

New failing tests:
fast/dom/resource-locations-in-created-html-document.html
http/tests/security/frame-loading-via-document-write-async-delegates.html
http/tests/security/frame-loading-via-document-write.html
Comment 4 EWS Watchlist 2018-09-26 13:42:55 PDT
Created attachment 350894 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 EWS Watchlist 2018-09-26 14:38:03 PDT
Comment on attachment 350884 [details]
WIP Patch

Attachment 350884 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9359755

New failing tests:
fast/dom/resource-locations-in-created-html-document.html
http/tests/security/frame-loading-via-document-write.html
http/tests/security/frame-loading-via-document-write-async-delegates.html
Comment 6 EWS Watchlist 2018-09-26 14:38:05 PDT
Created attachment 350902 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 7 EWS Watchlist 2018-09-26 14:52:34 PDT
Comment on attachment 350884 [details]
WIP Patch

Attachment 350884 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9359593

New failing tests:
fast/dom/resource-locations-in-created-html-document.html
http/tests/security/frame-loading-via-document-write-async-delegates.html
http/tests/security/frame-loading-via-document-write.html
Comment 8 EWS Watchlist 2018-09-26 14:52:35 PDT
Created attachment 350907 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 EWS Watchlist 2018-09-26 14:55:28 PDT
Comment on attachment 350884 [details]
WIP Patch

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

New failing tests:
fast/dom/resource-locations-in-created-html-document.html
http/tests/security/frame-loading-via-document-write.html
http/tests/security/frame-loading-via-document-write-async-delegates.html
Comment 10 EWS Watchlist 2018-09-26 14:55:39 PDT
Created attachment 350908 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 11 Chris Dumez 2018-09-26 15:57:40 PDT
Created attachment 350919 [details]
Patch
Comment 12 Alex Christensen 2018-09-27 09:10:18 PDT
Comment on attachment 350919 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:2699
> +    if (responsibleDocument && isFullyActive()) {
>          setURL(responsibleDocument->url());

What is the URL if this branch is not taken?

> Source/WebCore/dom/Document.idl:110
> +    [CEReactions, CallWith=ResponsibleDocument, ImplementedAs=openForBindings, MayThrowException] Document open(optional DOMString unused1, optional DOMString unused2); // both arguments are ignored.

This change seems unrelated.  Why is it in the same patch?

> LayoutTests/ChangeLog:10
> +        failing in Firefox and is now passing in Firefox.

What about Chrome?

> LayoutTests/imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url.window-expected.txt:3
> +FAIL document.open() does not change document's URL (active but not fully active document) null is not an object (evaluating 'childWin.location.href')

What about this one?
Comment 13 youenn fablet 2018-09-27 09:10:30 PDT
Comment on attachment 350919 [details]
Patch

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

> LayoutTests/fast/dom/resource-locations-in-created-html-document.html:15
> +        if (path == '/test')

s/==/===/

> LayoutTests/imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url.window-expected.txt:3
> +FAIL document.open() does not change document's URL (active but not fully active document) null is not an object (evaluating 'childWin.location.href')

Do you know why we are failing this test?
Comment 14 Chris Dumez 2018-09-27 09:13:39 PDT
Comment on attachment 350919 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:2699
>>          setURL(responsibleDocument->url());
> 
> What is the URL if this branch is not taken?

Whatever it was previously, we just do not override it, per spec.

>> Source/WebCore/dom/Document.idl:110
>> +    [CEReactions, CallWith=ResponsibleDocument, ImplementedAs=openForBindings, MayThrowException] Document open(optional DOMString unused1, optional DOMString unused2); // both arguments are ignored.
> 
> This change seems unrelated.  Why is it in the same patch?

Technically no behavior change, just clean up of the Document.open() that was made in the spec at the same time.

>> LayoutTests/ChangeLog:10
>> +        failing in Firefox and is now passing in Firefox.
> 
> What about Chrome?

Chrome still has the old behavior but they are working on aligning as well.

>>> LayoutTests/imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url.window-expected.txt:3
>>> +FAIL document.open() does not change document's URL (active but not fully active document) null is not an object (evaluating 'childWin.location.href')
>> 
>> What about this one?
> 
> Do you know why we are failing this test?

I looked into it and the failure seems unrelated. WebKit returned null for window.location after a frame is detached, which is not as per specification.
Comment 15 Chris Dumez 2018-09-27 09:15:07 PDT
Created attachment 350966 [details]
Patch
Comment 16 Timothy Gu 2018-09-27 09:15:32 PDT
Comment on attachment 350919 [details]
Patch

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

>>> LayoutTests/ChangeLog:10
>>> +        failing in Firefox and is now passing in Firefox.
>> 
>> What about Chrome?
> 
> Chrome still has the old behavior but they are working on aligning as well.

We are working on fixing the same. See https://chromium-review.googlesource.com/c/chromium/src/+/1188643.

Should this say "Safari" instead of "Firefox"?
Comment 17 Chris Dumez 2018-09-27 09:17:14 PDT
(In reply to Timothy Gu from comment #16)
> Comment on attachment 350919 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350919&action=review
> 
> >>> LayoutTests/ChangeLog:10
> >>> +        failing in Firefox and is now passing in Firefox.
> >> 
> >> What about Chrome?
> > 
> > Chrome still has the old behavior but they are working on aligning as well.
> 
> We are working on fixing the same. See
> https://chromium-review.googlesource.com/c/chromium/src/+/1188643.
> 
> Should this say "Safari" instead of "Firefox"?

No, I tested in Firefox. The test in question was failing in Firefox and the new version of the test is now passing in Firefox. So basically, we aligned behavior with the spec and Firefox, and we updated the test accordingly.
Comment 18 Timothy Gu 2018-09-27 09:20:24 PDT
(In reply to Chris Dumez from comment #17)
> (In reply to Timothy Gu from comment #16)
> > Comment on attachment 350919 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=350919&action=review
> > 
> > >>> LayoutTests/ChangeLog:10
> > >>> +        failing in Firefox and is now passing in Firefox.
> > >> 
> > >> What about Chrome?
> > > 
> > > Chrome still has the old behavior but they are working on aligning as well.
> > 
> > We are working on fixing the same. See
> > https://chromium-review.googlesource.com/c/chromium/src/+/1188643.
> > 
> > Should this say "Safari" instead of "Firefox"?
> 
> No, I tested in Firefox. The test in question was failing in Firefox and the
> new version of the test is now passing in Firefox. So basically, we aligned
> behavior with the spec and Firefox, and we updated the test accordingly.

Makes sense
Comment 19 Timothy Gu 2018-09-27 09:21:29 PDT
(In reply to Chris Dumez from comment #14)

> >>> LayoutTests/imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url.window-expected.txt:3
> >>> +FAIL document.open() does not change document's URL (active but not fully active document) null is not an object (evaluating 'childWin.location.href')
> >> 
> >> What about this one?
> > 
> > Do you know why we are failing this test?
> 
> I looked into it and the failure seems unrelated. WebKit returned null for
> window.location after a frame is detached, which is not as per specification.

See https://github.com/whatwg/html/issues/3959.
Comment 20 Chris Dumez 2018-09-27 09:31:58 PDT
Comment on attachment 350966 [details]
Patch

Clearing flags on attachment: 350966

Committed r236550: <https://trac.webkit.org/changeset/236550>
Comment 21 Chris Dumez 2018-09-27 09:32:00 PDT
All reviewed patches have been landed.  Closing bug.