Bug 53919

Summary: Reference-counting for WKBundlePageResourceLoadClient::willSendRequestForFrame seems unclear/wrong
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: WebKit2Assignee: Brian Weinstein <bweinstein>
Status: RESOLVED FIXED    
Severity: Normal CC: bweinstein, mjs, pfeldman, sam, sullivan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://trac.webkit.org/browser/trunk/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.cpp?rev=77713#L184
Attachments:
Description Flags
[PATCH] Fix
darin: review+
[PATCH] Crash fix
andersca: review-
[PATCH] crash Fix (Take 2) sam: review+

Description Adam Roben (:aroben) 2011-02-07 08:45:19 PST
InjectedBundlePageLoaderClient::willSendRequestForFrame, which calls WKBundlePageLoaderClient::willSendRequestForFrame (see URL), refs the result. This implies that willSendRequestForFrame should return a WKURLRequestRef whose refcount hasn't been incremented. But that makes returning a newly-constructed WKURLRequestRef very hard to do without causing a leak, since it requires the client to release the WKURLRequestRef *after* returning it from willSendRequestForFrame.

Probably the best way to fix this is to give willSendRequestForFrame Create semantics, just like WKPageUIClient::createPage. InjectedBundlePageLoaderClient::willSendRequestForFrame should adopt the WKURLRequestRef instead of reffing it. We may want to rename willSendRequestForFrame if we do this.
Comment 1 Adam Roben (:aroben) 2011-02-07 08:45:57 PST
<rdar://problem/8966020>
Comment 2 Adam Roben (:aroben) 2011-02-08 05:55:18 PST
The code has moved to WKBundlePageResourceLoadClient::willSendRequestForFrame, but the bug still exists. :-(
Comment 3 Brian Weinstein 2011-03-04 16:07:53 PST
Created attachment 84822 [details]
[PATCH] Fix
Comment 4 Brian Weinstein 2011-03-04 16:36:43 PST
Landed in r80392.
Comment 5 Pavel Feldman 2011-03-05 03:08:09 PST
This change broke WK2 layout tests (existing early after 20 crashes). Do you want to roll it out?
Comment 6 Brian Weinstein 2011-03-05 10:50:51 PST
(In reply to comment #5)
> This change broke WK2 layout tests (existing early after 20 crashes). Do you want to roll it out?

It looks like the issue is I didn't touch WebKitTestRunner. I'm looking into a fix now.
Comment 7 Brian Weinstein 2011-03-05 11:06:55 PST
Created attachment 84871 [details]
[PATCH] Crash fix
Comment 8 Anders Carlsson 2011-03-05 11:13:15 PST
Comment on attachment 84871 [details]
[PATCH] Crash fix

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:348
> +    WKRetain(returnedRequest);

This is incorrect. The other willSendRequestForFrame should do the WKRetain.
Comment 9 Brian Weinstein 2011-03-05 11:21:08 PST
Created attachment 84872 [details]
[PATCH] crash Fix (Take 2)
Comment 10 Brian Weinstein 2011-03-05 11:50:10 PST
Landed crash fix in r80427.