Summary: | Prepare to remove automatic URL->String conversion operators | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aboxhall, alecflett, apinheiro, beidson, benjamin, calvaris, cdumez, cfleizach, cmarcelo, darin, dino, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, hi, japhet, jcraig, jdiggs, jer.noble, joepeck, jsbell, kangil.han, keith_miller, Lawrence.j, macpherson, mark.lam, menard, mifenton, mkwst, msaboff, pdr, philipj, saam, sabouhallawa, samuel_white, schenney, sergio, toyoshim, tzagallo, webkit-bug-importer, yutak | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Alex Christensen
2020-04-24 18:43:29 PDT
Created attachment 397534 [details]
Patch
Created attachment 397537 [details]
Patch
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. (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. Created attachment 397577 [details]
Patch
Created attachment 397579 [details]
patch with export macro
Patch 397577 does not build Committed r260709: <https://trac.webkit.org/changeset/260709> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397579 [details]. (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. (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. https://trac.webkit.org/changeset/260765/webkit Changing JavaScript URL parsing would likely cause incompatibilities (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! *** Bug 211086 has been marked as a duplicate of this bug. *** |