RESOLVED FIXED 72762
Null derefs in successful resource revalidation after r100311 
https://bugs.webkit.org/show_bug.cgi?id=72762
Summary Null derefs in successful resource revalidation after r100311
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-
DetailsFormatted DiffDiff
Better fix + test (5.18 KB, patch)
2011-11-30 11:49 PST, Nate Chapin 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
attempt #3 (5.16 KB, patch)
2011-12-07 14:49 PST, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Finish, rather than cancel, 304s (5.86 KB, patch)
2011-12-08 14:27 PST, Nate Chapin 		no flags
DetailsFormatted DiffDiff
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.