WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
89468
[Qt] KURL assert at fast/loader/opaque-base-url.html
https://bugs.webkit.org/show_bug.cgi?id=89468
Summary
[Qt] KURL assert at fast/loader/opaque-base-url.html
Balazs Kelemen
Reported
2012-06-19 07:33:10 PDT
Local debug test session, desktop Ubuntu 11.10, WK2
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Balazs Kelemen
Comment 1
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
Balazs Kelemen
Comment 2
2012-06-26 09:31:36 PDT
Created
attachment 149544
[details]
Patch
Alexey Proskuryakov
Comment 3
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.
Balazs Kelemen
Comment 4
2012-06-27 04:27:45 PDT
Created
attachment 149725
[details]
Patch
Simon Hausmann
Comment 5
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.
Balazs Kelemen
Comment 6
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.
Balazs Kelemen
Comment 7
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
>
Balazs Kelemen
Comment 8
2012-06-28 00:47:11 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug