Bug 209174

Created attachment 393738 [details] Patch

Created attachment 393957 [details] Patch

Two tests are failing: imported/w3c/web-platform-tests/html/dom/usvstring-reflection.https.html gets some more PASS. I believe the issue was that we were comparing "about:blank" with "about:blank#xxx" and we are now no longer checking the hash. Seems like an improvement. http/tests/dom/window-open-about-uppercase-blank-and-access-document.html is now failing and is specifically trying to do window.open("about:BLANK"). Both Firefox and Chrome pass the test but with different behaviours. They treat about:BLANK as somehow different from about:blank though. For instance Chrome will end-up loading "about:blank#blocked". Probably the navigation starts with "about:blank", tries to go to "about:BLANK" which is blocked. I guess we could make a DOMWindow::open specific fix here, or preserve for now SecurityPolicy::shouldInheritSecurityOriginFromOwner behavior.

Created attachment 393971 [details] Patch

Created attachment 393976 [details] Patch

Created attachment 394076 [details] Patch

Comment on attachment 394076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394076&action=review > Source/WTF/wtf/URL.h:262 > WTF_EXPORT_PRIVATE const URL& blankURL(); I think we should rename this.

Comment on attachment 394076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394076&action=review >> Source/WTF/wtf/URL.h:262 >> WTF_EXPORT_PRIVATE const URL& blankURL(); > > I think we should rename this. Agreed, it should be aboutBlankURL probably.

Committed r258769: <https://trac.webkit.org/changeset/258769> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394076 [details].

<rdar://problem/60692601>

Comment on attachment 394076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394076&action=review > Source/WTF/wtf/URL.cpp:1026 > + return protocolIsAbout() && path() == "blank"; Case sensitive. On purpose? I see FIXME that implies it is. But also, I would like to see test coverage. > Source/WTF/wtf/URL.cpp:1031 > + return protocolIsAbout() && path() == "srcdoc"; Ditto.

Comment on attachment 394076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394076&action=review > Source/WTF/wtf/URL.cpp:878 > + static NeverDestroyed<URL> staticAboutSrcDocURL(URL(), "about:srcdoc"); I wonder if we should include const for things like this. static const NeverDestroyed<URL> ... Not sure if this has enough value to be worth including the extra keyword. > Source/WebCore/html/HTMLFrameElementBase.cpp:156 > + return WTF::aboutSrcDocURL(); Should not need a WTF prefix. Normally for WTF we are putting "using" in the header itself. I would like to stay consistent one way or the other. > Source/WebCore/loader/FrameLoader.cpp:3714 > - if (!equalLettersIgnoringASCIICase(url.string(), "about:srcdoc")) > + if (!url.isAboutSrcDoc()) Here we changed behavior. Do we have test coverage? > Source/WebCore/page/SecurityPolicy.cpp:170 > + // FIXME: We also allow some URLs like "about:BLANK". We should probably block navigation to these URLs, see rdar://problem/57966056. > + return url.isEmpty() || url.isAboutBlank() || url.isAboutSrcDoc() || equalIgnoringASCIICase(url.string(), WTF::blankURL()); Here we preserved this old behavior, and the FIXME suggests changing bahvior. Do we have test coverage either way? > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:82 > - && !equalIgnoringASCIICase(openerUrl.string(), WTF::blankURL())) { > + && !openerUrl.isAboutBlank()) { Here we changed behavior. Do we have test coverage?

> > Source/WebCore/html/HTMLFrameElementBase.cpp:156 > > + return WTF::aboutSrcDocURL(); > > Should not need a WTF prefix. Normally for WTF we are putting "using" in the > header itself. I would like to stay consistent one way or the other. Will fix it. > > Source/WebCore/loader/FrameLoader.cpp:3714 > > - if (!equalLettersIgnoringASCIICase(url.string(), "about:srcdoc")) > > + if (!url.isAboutSrcDoc()) > > Here we changed behavior. Do we have test coverage? I do not think we are changing behavior since this is an early return. We are early returning more often but, in the cases where we were not, ownerElement->hasAttributeWithoutSynchronization(srcdocAttr) would catch the other cases. > > Source/WebCore/page/SecurityPolicy.cpp:170 > > + // FIXME: We also allow some URLs like "about:BLANK". We should probably block navigation to these URLs, see rdar://problem/57966056. > > + return url.isEmpty() || url.isAboutBlank() || url.isAboutSrcDoc() || equalIgnoringASCIICase(url.string(), WTF::blankURL()); > > Here we preserved this old behavior, and the FIXME suggests changing > bahvior. Do we have test coverage either way? We have test coverage here: http/tests/dom/window-open-about-uppercase-blank-and-access-document.html. > > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:82 > > - && !equalIgnoringASCIICase(openerUrl.string(), WTF::blankURL())) { > > + && !openerUrl.isAboutBlank()) { > > Here we changed behavior. Do we have test coverage? Right. John, do you know how we can exercise that code path?