Bug 53919

Summary:

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

Product:

WebKit

Reporter:

Adam Roben (:aroben) <aroben>

Component:

WebKit2

Assignee:

Brian Weinstein <bweinstein>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bweinstein, mjs, pfeldman, sam, sullivan

Priority:

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+

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.