Bug 211007 – Prepare to remove automatic URL->String conversion operators

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Prepare to remove automatic URL->String conversion operators

[Alex Christensen]

Created attachment 397534 [details]
Patch

[Alex Christensen]

Created attachment 397537 [details]
Patch

[Darin Adler]

Comment on attachment 397537 [details]
Patch

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

> Source/WTF/wtf/URL.cpp:317
> +    return WTF::protocolIsJavaScript(string());

I think a better implementation of this would be:

    return protocolIs("javascript");

> Source/WebCore/bindings/IDLTypes.h:35
>  #include <wtf/text/WTFString.h>

Can remove this since we are including URL.h now.

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:533
> -            if (!document->page()->cookieJar().getRawCookies(*document, URL({ }, url), rawCookiesForURLInDocument))
> +            if (!document->page()->cookieJar().getRawCookies(*document, url, rawCookiesForURLInDocument))

Wow, nice to fix this! Now I am happier with the change after seeing this.

> Source/WebCore/loader/DocumentLoader.cpp:1514
> -        if (auto subresource = this->subresource({ { }, handle->url() }))
> +        if (auto subresource = this->subresource(handle->url()))

Another!

> Source/WebCore/loader/TextTrackLoader.cpp:151
> -    ResourceRequest resourceRequest(m_document.completeURL(url));
> +    ResourceRequest resourceRequest(m_document.completeURL(url.string()));

This seems really peculiar and is worth revisiting to see if it’s correct. I agree that the patch does not change behavior, but this seems wrong.

> Source/WebCore/workers/WorkerScriptLoader.cpp:64
> -            m_responseURL = URL { URL { }, scriptResource->responseURL };
> +            m_responseURL = scriptResource->responseURL;

Another of these "throw away URL and recreate it" cases.

> Source/WebKit/WebProcess/WebPage/WebFrame.cpp:187
> -        ResourceRequest(URL(URL(), url())),
> +        ResourceRequest(url()),

And ... *another*!

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:630
> -    URL mainFrameURL = { URL(), mainFrame.url() };
> +    URL mainFrameURL = mainFrame.url();

And ... *another*. OK, totally sold.

[Alex Christensen]

(In reply to Darin Adler from comment #3)
> Comment on attachment 397537 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397537&action=review
> 
> > Source/WTF/wtf/URL.cpp:317
> > +    return WTF::protocolIsJavaScript(string());
> 
> I think a better implementation of this would be:
> 
>     return protocolIs("javascript");
That's what I thought too, but that failed some tests.  WTF::protocolIsJavaScript is needed to return true for invalid URLs that are valid JS URLs.

> > Source/WebCore/bindings/IDLTypes.h:35
> >  #include <wtf/text/WTFString.h>
> 
> Can remove this since we are including URL.h now.
Yes.

> > Source/WebCore/loader/TextTrackLoader.cpp:151
> > -    ResourceRequest resourceRequest(m_document.completeURL(url));
> > +    ResourceRequest resourceRequest(m_document.completeURL(url.string()));
> 
> This seems really peculiar and is worth revisiting to see if it’s correct. I
> agree that the patch does not change behavior, but this seems wrong.
I agree.  I added a FIXME comment.

[Alex Christensen]

Created attachment 397577 [details]
Patch

[Alex Christensen]

Created attachment 397579 [details]
patch with export macro

[EWS]

Patch 397577 does not build

[EWS]

Committed r260709: <https://trac.webkit.org/changeset/260709>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397579 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62376149>

[Darin Adler]

(In reply to Alex Christensen from comment #4)
> That's what I thought too, but that failed some tests. 
> WTF::protocolIsJavaScript is needed to return true for invalid URLs that are
> valid JS URLs.

Sounds dangerously subtle. We may need to consider how to redefine how these calls work for invalid URLs to protect people in the future.

[Darin Adler]

(In reply to Darin Adler from comment #10)
> (In reply to Alex Christensen from comment #4)
> > That's what I thought too, but that failed some tests. 
> > WTF::protocolIsJavaScript is needed to return true for invalid URLs that are
> > valid JS URLs.
> 
> Sounds dangerously subtle. We may need to consider how to redefine how these
> calls work for invalid URLs to protect people in the future.

I think we might want to change URL parsing so that JavaScript URLs are always "valid" but don’t have any parts other than the protocol. I’d like to hear your thoughts on this.

[Alex Christensen]

https://trac.webkit.org/changeset/260765/webkit

Changing JavaScript URL parsing would likely cause incompatibilities

[Darin Adler]

(In reply to Alex Christensen from comment #12)
> Changing JavaScript URL parsing would likely cause incompatibilities

I’m proposing changes to JavaScript URL validity rules, not to other aspects of parsing. Are you *sure* that would cause incompatibilities? Can we at least try it?

Our URL::protocolIs("javascript") and URL::protocolIsJavaScript are dangerous as long as we have many URLs that will execute JavaScript, but return false to those. At least we should remove them!

[Alexey Proskuryakov]

*** Bug 211086 has been marked as a duplicate of this bug. ***