Bug 93788 - [WK2] toAPI(const WebCore::ResourceResponse& response) should check response for being NULL
Summary: [WK2] toAPI(const WebCore::ResourceResponse& response) should check response ...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-12 14:08 PDT by Mikhail Pozdnyakov
Modified: 2012-08-14 00:54 PDT (History)
3 users (show)

See Also:


Attachments
patch (1.67 KB, patch)
2012-08-12 14:18 PDT, Mikhail Pozdnyakov
sam: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2012-08-12 14:08:55 PDT
toAPI(const WebCore::ResourceResponse& response) currently does not check input response for being NULL and 
always returns new WKURLResponseRef value. It is impossible to detect from the resulting WKURLResponseRef value whether the initial WebCore::ResourceResponse was NULL or not, so crashes are possible. 

Just faced with such crash working on bug42332, giving an extract from crash log:

STDERR: 12  0x7ffb70512c1b WTR::InjectedBundlePage::willSendRequestForFrame(OpaqueWKBundlePage const*, OpaqueWKBundleFrame const*, unsigned long, OpaqueWKURLRequest const*, OpaqueWKURLResponse const*)
STDERR: 13  0x7ffb70511076 WTR::InjectedBundlePage::willSendRequestForFrame(OpaqueWKBundlePage const*, OpaqueWKBundleFrame const*, unsigned long, OpaqueWKURLRequest const*, OpaqueWKURLResponse const*, void const*)
STDERR: 14  0x7ffbc0e6a7fb WebKit::InjectedBundlePageResourceLoadClient::willSendRequestForFrame(WebKit::WebPage*, WebKit::WebFrame*, unsigned long, WebCore::ResourceRequest&, WebCore::ResourceResponse const&)
STDERR: 15  0x7ffbc0e97c1c WebKit::WebFrameLoaderClient::dispatchWillSendRequest(WebCore::DocumentLoader*, unsigned long, WebCore::ResourceRequest&, WebCore::ResourceResponse const&)
STDERR: 16  0x7ffbbc21d84e WebCore::ResourceLoadNotifier::dispatchWillSendRequest(WebCore::DocumentLoader*, unsigned long, WebCore::ResourceRequest&, WebCore::ResourceResponse const&)
STDERR: 17  0x7ffbbc21d454 WebCore::ResourceLoadNotifier::willSendRequest(WebCore::ResourceLoader*, WebCore::ResourceRequest&, WebCore::ResourceResponse const&)
Comment 1 Mikhail Pozdnyakov 2012-08-12 14:18:11 PDT
Created attachment 157909 [details]
patch
Comment 2 Alexey Proskuryakov 2012-08-12 15:52:32 PDT
Does this crash occur when working with a null URL here?

    WKRetainPtr<WKURLRef> url = adoptWK(WKURLRequestCopyURL(request));
    WKRetainPtr<WKStringRef> host = adoptWK(WKURLCopyHostName(url.get()));

I guess I don't yet see the difference between getting a null WKURLRequestRef and a null WKURLRef.
Comment 3 Mikhail Pozdnyakov 2012-08-13 01:17:27 PDT
(In reply to comment #2)
> Does this crash occur when working with a null URL here?
> 
>     WKRetainPtr<WKURLRef> url = adoptWK(WKURLRequestCopyURL(request));
>     WKRetainPtr<WKStringRef> host = adoptWK(WKURLCopyHostName(url.get()));
> 
> I guess I don't yet see the difference between getting a null WKURLRequestRef and a null WKURLRef.

Not exactly, in case when WebCore::ResourceRequest is NULL crash will be already here "WKURLRequestCopyURL(request)", I have example of crashing in
WKURLResponseCopyURL which has very same code with WKURLRequestCopyURL:

STDERR: 1   0x7f2183fcba03
STDERR: 2   0x7f21869194c0
STDERR: 3   0x7f2187110a84 WTF::RefPtr<WTF::StringImpl>::operator!() const
STDERR: 4   0x7f2187110664 WTF::String::isNull() const
STDERR: 5   0x7f2187155441 WTF::operator!(WTF::String const&)
STDERR: 6   0x7f2187155693 WebKit::toCopiedURLAPI(WTF::String const&)
STDERR: 7   0x7f2187159e76 WKURLResponseCopyURL
STDERR: 8   0x7f2138396bae
STDERR: 9   0x7f2138399855 WTR::InjectedBundlePage::willSendRequestForFrame(OpaqueWKBundlePage const*, OpaqueWKBundleFrame const*, unsigned long, OpaqueWKURLRequest const*, OpaqueWKURLResponse const*)
Comment 4 Alexey Proskuryakov 2012-08-13 09:35:38 PDT
I think that we need to get Sam's opinion on the intended design here.
Comment 5 Sam Weinig 2012-08-13 10:28:30 PDT
Comment on attachment 157909 [details]
patch

It would be easier to evaluate the choice here, if you had a test case showing this happening in your patch. r-ing for now due to lack of tst case.
Comment 6 Mikhail Pozdnyakov 2012-08-14 00:54:42 PDT
(In reply to comment #5)
> (From update of attachment 157909 [details])
> It would be easier to evaluate the choice here, if you had a test case showing this happening in your patch. r-ing for now due to lack of tst case.

It used to crash on existing http/tests/loading/redirect-methods.html, however crashes are not observed anymore since I have rebased to the latest master. Will figure out the changes.. Closing so far, will reopen if find the issue again.