Bug 72762 - Null derefs in successful resource revalidation after r100311
Summary: Null derefs in successful resource revalidation after r100311
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-18 14:49 PST by Nate Chapin
Modified: 2011-12-12 10:27 PST (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 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.
Comment 1 Nate Chapin 2011-11-18 14:56:39 PST
Created attachment 115876 [details]
patch
Comment 2 Adam Barth 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.
Comment 3 WebKit Review Bot 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
Comment 4 Nate Chapin 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.
Comment 5 Nate Chapin 2011-11-30 11:49:34 PST
Created attachment 117241 [details]
Better fix + test
Comment 6 WebKit Review Bot 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
Comment 7 Nate Chapin 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.....
Comment 8 Nate Chapin 2011-12-07 14:49:14 PST
Created attachment 118281 [details]
attempt #3
Comment 9 Adam Barth 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.
Comment 10 Nate Chapin 2011-12-08 14:27:43 PST
Created attachment 118471 [details]
Finish, rather than cancel, 304s
Comment 11 WebKit Review Bot 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>
Comment 12 WebKit Review Bot 2011-12-12 10:27:54 PST
All reviewed patches have been landed.  Closing bug.