Bug 189375 – document.open() should not propagate URLs to non-fully active documents

• Component: DOM
• Status: RESOLVED FIXED

[Timothy Gu]

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.

[Radar WebKit Bug Importer]

<rdar://problem/44282755>

[Chris Dumez]

Created attachment 350884 [details]
WIP Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 350919 [details]
Patch

[Alex Christensen]

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?

[youenn fablet]

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?

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 350966 [details]
Patch

[Timothy Gu]

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"?

[Chris Dumez]

(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.

[Timothy Gu]

(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

[Timothy Gu]

(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.

[Chris Dumez]

Comment on attachment 350966 [details]
Patch

Clearing flags on attachment: 350966

Committed r236550: <https://trac.webkit.org/changeset/236550>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.