Bug 23609 - Do not append trailing slash to urls with no path for KURL::parse
Summary: Do not append trailing slash to urls with no path for KURL::parse
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-28 20:16 PST by Adam Treat
Modified: 2009-01-29 06:40 PST (History)
4 users (show)

See Also:


Attachments
Do not append trailing slash in parse (1.44 KB, patch)
2009-01-28 20:17 PST, Adam Treat
ap: review-
Details | Formatted Diff | Diff
Makes KURLQt.cpp use two-arg ctor (1.34 KB, patch)
2009-01-29 06:24 PST, Adam Treat
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Treat 2009-01-28 20:16:25 PST
The KURL class has recently been changed in revision r40242 to assert in the KURL single argument constructors when the string passed in does not match the result of the parse.  The theory is that KURL::parse method should be idempotent.  However, the KURL::parse method appends a trailing slash to a perfectly valid URL that has no path.  This results in triggering the ASSERT in KURL ctor by passing in a string of "http://google.com" for instance.

Looking at the history it seems KURL has been adding this trailing slash since WebKit was originally created from KHTML.  With this history I am not sure the right thing to do is to remove this appending of the trailing slash, but if the assert is to stay then this is what is required.
Comment 1 Adam Treat 2009-01-28 20:17:30 PST
Created attachment 27136 [details]
Do not append trailing slash in parse

Remove the append of the trailing slash.
Comment 2 Adam Treat 2009-01-28 20:19:54 PST
The other option is to remove or edit the assert to take into account the trailing slash I suppose.
Comment 3 Alexey Proskuryakov 2009-01-28 22:40:38 PST
Where is the single argument constructor called from? If it's called with a string that is not already an output of KURL::parse algorithm, then it is likely that an arbitrary string can be passed via this code path, so a two-argument constructor should be used.

This assertion usually fires on seemingly innocuous things like this one, but uncovers real bugs at call sites nonetheless.
Comment 4 Alexey Proskuryakov 2009-01-28 22:43:07 PST
Comment on attachment 27136 [details]
Do not append trailing slash in parse

Marking r- for now, because it's more likely to be a bug at call site.
Comment 5 Adam Treat 2009-01-29 05:50:48 PST
It is being called from KURLQt.cpp for the QUrl conversion constructor.  Perhaps I should add a special new constructor to KURL for usage by PLATFORM(QT) that will forgo the parsing entirely since QUrl already accomplishes this?
Comment 6 Alexey Proskuryakov 2009-01-29 06:23:30 PST
For posterity: as discussed on IRC, I think that if any constructor were to expect QUrl output to be properly encoded, then KURL::parse should be replaced with an appropriate QUrl method to avoid surprises during re-parsing.

For now, it's probably best to just use two-argument KURL constructor with a null base.
Comment 7 Adam Treat 2009-01-29 06:24:52 PST
Created attachment 27148 [details]
Makes KURLQt.cpp use two-arg ctor

After discussion with ap it was determined that KURLQt should use the two-arg ctor for implicit conversion of QUrl to KURL.
Comment 8 Alexey Proskuryakov 2009-01-29 06:26:49 PST
Comment on attachment 27148 [details]
Makes KURLQt.cpp use two-arg ctor

r=me
Comment 9 Adam Treat 2009-01-29 06:40:06 PST
Fixed with r40362.