Bug 61201 – Audit all uses of KURL::prettyURL

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[WebKit Review Bot]

Audit all uses of KURL::prettyURL
Requested by abarth on #webkit.

[Alexey Proskuryakov]

I think that it has its place when displaying a URL to the user, but not necessarily in DOM code.

[Adam Barth]

Looks like prettyURL is mostly used for location.href and as a sandtrap to confuse code into having bugs:

./WebCore/page/Location.cpp:    return url.hasPath() ? url.prettyURL() : url.prettyURL() + "/";
./WebCore/page/Location.cpp:    return url.hasPath() ? url.prettyURL() : url.prettyURL() + "/";
./WebCore/platform/KURL.cpp:String KURL::prettyURL() const
./WebCore/platform/KURL.h:    String prettyURL() const;
./WebCore/platform/KURLGoogle.cpp:String KURL::prettyURL() const
./WebCore/platform/network/soup/ResourceHandleSoup.cpp:    GOwnPtr<SoupURI> soupURI(soup_uri_new(url.prettyURL().utf8().data()));
./WebCore/workers/WorkerLocation.cpp:    return m_url.hasPath() ? m_url.prettyURL() : m_url.prettyURL() + "/";
./WebCore/workers/WorkerLocation.cpp:    return m_url.hasPath() ? m_url.prettyURL() : m_url.prettyURL() + "/";
./WebKit/efl/ewk/ewk_frame.cpp:    hit_test->link.url = eina_stringshare_add(result.absoluteLinkURL().prettyURL().utf8().data());
./WebKit/efl/ewk/ewk_frame.cpp:    WTF::CString uri(sd->frame->document()->url().prettyURL().utf8());
./WebKit/efl/ewk/ewk_view.cpp:    priv->settings.user_stylesheet = eina_stringshare_add(url.prettyURL().utf8().data());
./WebKit/efl/ewk/ewk_view.cpp:        url.prettyURL().utf8().data(), referrer.utf8().data());
./WebKit/efl/ewk/ewk_view.cpp:        url.prettyURL().utf8().data(), mimeType.utf8().data());
./WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:            CString urlStr = url.prettyURL().utf8();
./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:    CString url = coreRequest.url().prettyURL().utf8();
./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:    CString url = coreRequest.url().prettyURL().utf8();
./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:    char* url = strdup(resourceRequest.url().prettyURL().utf8().data());
./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:    CString url = request.url().prettyURL().utf8();
./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:    ResourceError error("Error", -999, request.url().prettyURL(),
./WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:    return ResourceError("Error", WebKitErrorCannotUseRestrictedPort, request.url().prettyURL(),
./WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:            CString urlString = url.prettyURL().utf8();
./WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:    priv->uri = g_strdup(core(m_frame)->document()->url().prettyURL().utf8().data());
./WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:    priv->uri = g_strdup(core(m_frame)->loader()->activeDocumentLoader()->url().prettyURL().utf8().data());
./WebKit/gtk/webkit/webkitwebview.cpp:    String iconURL = iconDatabase().synchronousIconURLForPageURL(core(webView)->mainFrame()->document()->url().prettyURL());
./WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:        emit m_webPage->linkHovered(lastHoverURL.prettyURL(),
./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:        history->addHistoryEntry(loader->urlForHistory().prettyURL());
./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:    ResourceError error = ResourceError("QtNetwork", QNetworkReply::OperationCanceledError, request.url().prettyURL(),
./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:    return ResourceError("WebKitErrorDomain", WebKitErrorCannotUseRestrictedPort, request.url().prettyURL(),
./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:    // qDebug()<<" ++++++++++++++++ url is "<<url.prettyURL()<<", mime = "<<mimeTypeIn;
./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:    // qDebug()<<"------ Creating plugin in FrameLoaderClientQt::createPlugin for "<<url.prettyURL() << mimeType;
./WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:    // qDebug()<<"------\t url = "<<url.prettyURL();
./WebKit2/WebProcess/WebCoreSupport/qt/WebErrorsQt.cpp:    ResourceError error = ResourceError("QtNetwork", QNetworkReply::OperationCanceledError, request.url().prettyURL(),
./WebKit2/WebProcess/WebCoreSupport/qt/WebErrorsQt.cpp:    return ResourceError("WebKit", WebKitErrorCannotUseRestrictedPort, request.url().prettyURL(),
./WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:    return url.hasPath() ? url.prettyURL() : url.prettyURL() + "/";

[Adam Barth]

AFACT, all these callers are wrong.

[Adam Barth]

Created attachment 95116 [details]
Patch

[Darin Adler]

Comment on attachment 95116 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:-1204
> -    const KURL& url = frame->document()->url();
> -    return url.hasPath() ? url.prettyURL() : url.prettyURL() + "/";

What makes you sure we don’t need this “always make sure we have at least a /” logic?

[Early Warning System Bot]

Comment on attachment 95116 [details]
Patch

Attachment 95116 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8734961

[Adam Barth]

Comment on attachment 95116 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:-1204
>> -    return url.hasPath() ? url.prettyURL() : url.prettyURL() + "/";
> 
> What makes you sure we don’t need this “always make sure we have at least a /” logic?

The trailing / isn't required by the URI specs, nor by any URL parser I'm aware of.  (I also couldn't seem to actually create a document where document()->url()->string() was lacking the trailing slash.)

[Adam Barth]

Created attachment 95117 [details]
Patch

[Adam Barth]

e.g., data:text/html,%3Ca%20href=%22http://example.com%22%3E%3Cscript%3Ealert(document.body.firstElementChild.href)%3C/script%3E

has the trailing slash, as does <iframe src="http://example.com"> and reading document.documentURI inside the frame (which just grabs url().string()).

Safari Version 5.0.5 (6533.21.1)

[Alexey Proskuryakov]

Which of these call sites (if any) is responsible for displaying a pretty URL in Safari address bar?

I am unsure what is best for ResourceError URLs, which are strictly for human consumption.

[Adam Barth]

> Which of these call sites (if any) is responsible for displaying a pretty URL in Safari address bar?

Presumable none of them because I didn't change anything that effects WebKit1 on Mac!

> I am unsure what is best for ResourceError URLs, which are strictly for human consumption.

My thinking is that it's better to have these URLs work like every other URL in the web platform, but I agree that it's a matter of aesthetics.

[Alexey Proskuryakov]

Comment on attachment 95117 [details]
Patch

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

Yes - Safari probably relies upon code in NSURLExtras, which is independent of KURL.

> Source/WebCore/page/Location.cpp:69
> +    return url.hasPath() ? url.deprecatedString() : url.deprecatedString() + "/";

Why not add a FIXME link to <https://bugs.webkit.org/show_bug.cgi?id=30225> in case you don't resolve this immediately?

[Adam Barth]

(In reply to comment #12)
> (From update of attachment 95117 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95117&action=review
>
> > Source/WebCore/page/Location.cpp:69
> > +    return url.hasPath() ? url.deprecatedString() : url.deprecatedString() + "/";
> 
> Why not add a FIXME link to <https://bugs.webkit.org/show_bug.cgi?id=30225> in case you don't resolve this immediately?

Thanks.  Will do.

[Adam Barth]

Created attachment 95225 [details]
Patch for landing

[Adam Barth]

Committed r87623: <http://trac.webkit.org/changeset/87623>