Bug 23609 – Do not append trailing slash to urls with no path for KURL::parse

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Treat]

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.

[Adam Treat]

Created attachment 27136 [details]
Do not append trailing slash in parse

Remove the append of the trailing slash.

[Adam Treat]

The other option is to remove or edit the assert to take into account the trailing slash I suppose.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Adam Treat]

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?

[Alexey Proskuryakov]

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.

[Adam Treat]

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.

[Alexey Proskuryakov]

Comment on attachment 27148 [details]
Makes KURLQt.cpp use two-arg ctor

r=me

[Adam Treat]

Fixed with r40362.