Bug 61201

Summary: Audit all uses of KURL::prettyURL
Product: WebKit Reporter: WebKit Review Bot <webkit.review.bot>
Component: PlatformAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, arv, commit-queue, darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description WebKit Review Bot 2011-05-20 11:19:43 PDT
Audit all uses of KURL::prettyURL
Requested by abarth on #webkit.
Comment 1 Alexey Proskuryakov 2011-05-20 11:39:27 PDT
I think that it has its place when displaying a URL to the user, but not necessarily in DOM code.
Comment 2 Adam Barth 2011-05-24 19:41:29 PDT
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() + "/";
Comment 3 Adam Barth 2011-05-26 22:25:33 PDT
AFACT, all these callers are wrong.
Comment 4 Adam Barth 2011-05-26 22:33:11 PDT
Created attachment 95116 [details]
Patch
Comment 5 Darin Adler 2011-05-26 22:36:58 PDT
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?
Comment 6 Early Warning System Bot 2011-05-26 22:50:05 PDT
Comment on attachment 95116 [details]
Patch

Attachment 95116 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8734961
Comment 7 Adam Barth 2011-05-26 22:55:30 PDT
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.)
Comment 8 Adam Barth 2011-05-26 22:58:07 PDT
Created attachment 95117 [details]
Patch
Comment 9 Adam Barth 2011-05-26 23:00:44 PDT
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)
Comment 10 Alexey Proskuryakov 2011-05-27 00:35:02 PDT
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.
Comment 11 Adam Barth 2011-05-27 00:40:47 PDT
> 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.
Comment 12 Alexey Proskuryakov 2011-05-27 15:15:20 PDT
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?
Comment 13 Adam Barth 2011-05-27 15:20:56 PDT
(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.
Comment 14 Adam Barth 2011-05-27 16:14:18 PDT
Created attachment 95225 [details]
Patch for landing
Comment 15 Adam Barth 2011-05-28 12:59:22 PDT
Committed r87623: <http://trac.webkit.org/changeset/87623>