Bug 89468 - [Qt] KURL assert at fast/loader/opaque-base-url.html
Summary: [Qt] KURL assert at fast/loader/opaque-base-url.html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords: LayoutTestFailure, Qt, QtTriaged
Depends on:
Blocks: 79668
  Show dependency treegraph
 
Reported: 2012-06-19 07:33 PDT by Balazs Kelemen
Modified: 2012-06-28 00:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.07 KB, patch)
2012-06-26 09:31 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (1.72 KB, patch)
2012-06-27 04:27 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2012-06-19 07:33:10 PDT
Local debug test session, desktop Ubuntu 11.10, WK2
Comment 1 Balazs Kelemen 2012-06-19 07:33:28 PDT
crash log for WebKitTestRunner (pid 24658):
STDOUT: <empty>
STDERR: ASSERTION FAILED: url.isEmpty() || isSchemeFirstChar(url[0])

Source/WebCore/platform/KURL.cpp(315) : void WebCore::checkEncodedString(const WTF::String&)
_ZN7WebCore4KURL5parseERKN3WTF6StringE+0x29
_ZN7WebCore4KURLC1ENS_18ParsedURLStringTagERKN3WTF6StringE+0x33
Comment 2 Balazs Kelemen 2012-06-26 09:31:36 PDT
Created attachment 149544 [details]
Patch
Comment 3 Alexey Proskuryakov 2012-06-26 21:04:46 PDT
Comment on attachment 149544 [details]
Patch

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

> Source/WebKit2/Shared/qt/WebCoreArgumentCodersQt.cpp:58
> -    request.setURL(KURL(WebCore::ParsedURLString, url));
> +    if (url.isNull())
> +        request.setURL(KURL());
> +    else
> +        request.setURL(KURL(WebCore::ParsedURLString, url));

It's not introduced by this patch, but it's not right to use this constructor form in argument decoders. For a message sent from WebProcess to UI Process, we cannot trust the content - it can well be malicious.

ParsedURLString constructor can only be used when we know that the string came from KURL::string(), and the object was valid.
Comment 4 Balazs Kelemen 2012-06-27 04:27:45 PDT
Created attachment 149725 [details]
Patch
Comment 5 Simon Hausmann 2012-06-27 22:28:53 PDT
Comment on attachment 149725 [details]
Patch

Yeah, this seems to be in line with what the other ports are using.

However at the same time this is an example of unnecessary code duplication between ports that could easily be cleaned up. Mac and Win implementations using CFNetwork appear to just encode and decode the entire underlying dictionary. Gtk, Efl and Qt ports are either just storing the URL or (in the case of the Gtk port) also more meta-data.

I think it would be really nice to _share_ a non-CFNetwork based implementation that saves/restores more properties of ResourceRequestBase and leaves room for a platform specific properties in the ResourceRequest sub-class.

Then issues like these are less likely to happen because there's more code coverage through the other ports.
Comment 6 Balazs Kelemen 2012-06-28 00:45:02 PDT
(In reply to comment #5)
> (From update of attachment 149725 [details])
> Yeah, this seems to be in line with what the other ports are using.
> 
> However at the same time this is an example of unnecessary code duplication between ports that could easily be cleaned up. Mac and Win implementations using CFNetwork appear to just encode and decode the entire underlying dictionary. Gtk, Efl and Qt ports are either just storing the URL or (in the case of the Gtk port) also more meta-data.
> 
> I think it would be really nice to _share_ a non-CFNetwork based implementation that saves/restores more properties of ResourceRequestBase and leaves room for a platform specific properties in the ResourceRequest sub-class.
> 
> Then issues like these are less likely to happen because there's more code coverage through the other ports.

Ok, than I'm going to land this for now and later on I will look into the idea of sharing this across ports.
Comment 7 Balazs Kelemen 2012-06-28 00:47:03 PDT
Comment on attachment 149725 [details]
Patch

Clearing flags on attachment: 149725

Committed r121416: <http://trac.webkit.org/changeset/121416>
Comment 8 Balazs Kelemen 2012-06-28 00:47:11 PDT
All reviewed patches have been landed.  Closing bug.