53919 – Reference-counting for WKBundlePageResourceLoadClient::willSendRequestForFrame seems unclear/wrong

RESOLVED FIXED 53919

Reference-counting for WKBundlePageResourceLoadClient::willSendRequestForFrame seems unclear/wrong

https://bugs.webkit.org/show_bug.cgi?id=53919

Summary Reference-counting for WKBundlePageResourceLoadClient::willSendRequestForFram...

Adam Roben (:aroben)

Reported 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.

Attachments
[PATCH] Fix (1.77 KB, patch) 2011-03-04 16:07 PST, Brian Weinstein	darin: review+	Details Formatted Diff Diff
[PATCH] Crash fix (1.83 KB, patch) 2011-03-05 11:06 PST, Brian Weinstein	andersca: review-	Details Formatted Diff Diff
[PATCH] crash Fix (Take 2) (1.13 KB, patch) 2011-03-05 11:21 PST, Brian Weinstein	sam: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Adam Roben (:aroben)

Comment 1 2011-02-07 08:45:57 PST

<rdar://problem/8966020>

Adam Roben (:aroben)

Comment 2 2011-02-08 05:55:18 PST

The code has moved to WKBundlePageResourceLoadClient::willSendRequestForFrame, but the bug still exists. :-(

Brian Weinstein

Comment 3 2011-03-04 16:07:53 PST

Created attachment 84822 [details] [PATCH] Fix

Brian Weinstein

Comment 4 2011-03-04 16:36:43 PST

Landed in r80392.

Pavel Feldman

Comment 5 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?

Brian Weinstein

Comment 6 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.

Brian Weinstein

Comment 7 2011-03-05 11:06:55 PST

Created attachment 84871 [details] [PATCH] Crash fix

Anders Carlsson

Comment 8 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.

Brian Weinstein

Comment 9 2011-03-05 11:21:08 PST

Created attachment 84872 [details] [PATCH] crash Fix (Take 2)

Brian Weinstein

Comment 10 2011-03-05 11:50:10 PST

Landed crash fix in r80427.

Note You need to log in before you can comment on or make changes to this bug.