Bug 72762

Summary:

Null derefs in successful resource revalidation after r100311

Product:

WebKit

Reporter:

Nate Chapin <japhet>

Component:

WebCore Misc.

Assignee:

Nate Chapin <japhet>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, ap, dglazkov, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	abarth: review+, webkit.review.bot: commit-queue-
Better fix + test	webkit.review.bot: commit-queue-
attempt #3	none
Finish, rather than cancel, 304s	none

Nate Chapin

Reported 2011-11-18 14:49:30 PST

Reported at http://code.google.com/p/chromium/issues/detail?id=104747 There's nothing to stop SubresourceLoaders from getting cancelled in the middle of successful revalidation. This can lead to CachedResource::m_resourceToRevalidate getting nulled out before expected. I think the solution is to return to the pre-100311 technique: Ensure SubresourceLoader enters its finishing state when we get a 304 in didReceiveResponse.

Attachments
patch (1.51 KB, patch) 2011-11-18 14:56 PST, Nate Chapin	abarth: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Better fix + test (5.18 KB, patch) 2011-11-30 11:49 PST, Nate Chapin	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
attempt #3 (5.16 KB, patch) 2011-12-07 14:49 PST, Nate Chapin	no flags	Details Formatted Diff Diff
Finish, rather than cancel, 304s (5.86 KB, patch) 2011-12-08 14:27 PST, Nate Chapin	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Nate Chapin

Comment 1 2011-11-18 14:56:39 PST

Created attachment 115876 [details] patch

Adam Barth

Comment 2 2011-11-18 15:14:11 PST

Comment on attachment 115876 [details] patch Ok. It's unfortunate we don't have a test for this change.

WebKit Review Bot

Comment 3 2011-11-18 15:24:53 PST

Comment on attachment 115876 [details] patch Attachment 115876 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10517259 New failing tests: http/tests/inspector/network/network-disable-cache-memory.html http/tests/inspector/resource-tree/resource-tree-reload.html

Nate Chapin

Comment 4 2011-11-18 16:29:18 PST

Comment on attachment 115876 [details] patch Oops, the uploaded patch has unintended side effects. I'm going to sit on this until after thanksgiving, since I'm on vacation next week.

Nate Chapin

Comment 5 2011-11-30 11:49:34 PST

Created attachment 117241 [details] Better fix + test

WebKit Review Bot

Comment 6 2011-11-30 21:16:32 PST

Comment on attachment 117241 [details] Better fix + test Attachment 117241 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10687469 New failing tests: http/tests/inspector/resource-har-conversion.html

Nate Chapin

Comment 7 2011-12-06 15:14:08 PST

Comment on attachment 117241 [details] Better fix + test Working on finding a patch that doesn't regress tests.....

Nate Chapin

Comment 8 2011-12-07 14:49:14 PST

Created attachment 118281 [details] attempt #3

Adam Barth

Comment 9 2011-12-08 12:07:01 PST

Comment on attachment 118281 [details] attempt #3 Nate and I talked about this patch via chat. We think it might be better to introduce a Revaldiating state so that we don't need to call cancel() in the revalidating case.

Nate Chapin

Comment 10 2011-12-08 14:27:43 PST

Created attachment 118471 [details] Finish, rather than cancel, 304s

WebKit Review Bot

Comment 11 2011-12-12 10:27:49 PST

Comment on attachment 118471 [details] Finish, rather than cancel, 304s Clearing flags on attachment: 118471 Committed r102602: <http://trac.webkit.org/changeset/102602>

WebKit Review Bot

Comment 12 2011-12-12 10:27:54 PST

All reviewed patches have been landed. Closing bug.

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