Summary: | [WK2] toAPI(const WebCore::ResourceResponse& response) should check response for being NULL | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||
Component: | WebKit2 | Assignee: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||
Status: | RESOLVED WONTFIX | ||||||
Severity: | Normal | CC: | ap, kenneth, sam | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Mikhail Pozdnyakov
2012-08-12 14:08:55 PDT
Created attachment 157909 [details]
patch
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. (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*) I think that we need to get Sam's opinion on the intended design here. 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.
(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. |