I think there's a better way to fix the problem of CachedResource use-after-free in 304 cases than trac.webkit.org/changeset/102602. The fundamental reason the revalidating CachedResource gets deleted prematurely is that clearResourceToRevalidate() gets called re-entrantly from switchClientsToRevalidatedResource(), so m_resourceToRevalidate gets nulled, and that's the only item in canDelete() that's causing us to return false. Ensuring clearResourceToRevalidate() doesn't get called during switchClientsToRevalidatedResource() should make problems go away and be marginally more readable.
Created attachment 121463 [details] patch
Comment on attachment 121463 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=121463&action=review > Source/WebCore/loader/cache/CachedResource.cpp:546 > + m_switchingClientsToRevalidatedResource = true; Generally I'm not a big fan of this pattern, mostly because I've seen what it's done to FrameLoader. If you think this is the right thing to do here, then I guess it's ok. > Source/WebCore/loader/cache/CachedResource.h:304 > + bool m_switchingClientsToRevalidatedResource : 1; The : 1 here isn't going to do that much good if there aren't other members to pack it with. Maybe skip for now?
Comment on attachment 121463 [details] patch I vaguely feel that a change like this should come with a few new ASSERTs.
(In reply to comment #2) > (From update of attachment 121463 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121463&action=review > > > Source/WebCore/loader/cache/CachedResource.cpp:546 > > + m_switchingClientsToRevalidatedResource = true; > > Generally I'm not a big fan of this pattern, mostly because I've seen what it's done to FrameLoader. If you think this is the right thing to do here, then I guess it's ok. Basically, I feel like it's the least evil of the options. I don't like it either, so I'll take another look and see if I can find something slightly more palatable. > > > Source/WebCore/loader/cache/CachedResource.h:304 > > + bool m_switchingClientsToRevalidatedResource : 1; > > The : 1 here isn't going to do that much good if there aren't other members to pack it with. Maybe skip for now? Good point, I was on auto-pilot when I did that. (In reply to comment #3) > (From update of attachment 121463 [details]) > I vaguely feel that a change like this should come with a few new ASSERTs. Good point, I'll see what I can do :)
(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 121463 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=121463&action=review > > > > > Source/WebCore/loader/cache/CachedResource.cpp:546 > > > + m_switchingClientsToRevalidatedResource = true; > > > > Generally I'm not a big fan of this pattern, mostly because I've seen what it's done to FrameLoader. If you think this is the right thing to do here, then I guess it's ok. > > Basically, I feel like it's the least evil of the options. I don't like it either, so I'll take another look and see if I can find something slightly more palatable. I haven't found any good alternative to the variable to prevent undesirable reentrancy. I think the right solution is to just make CachedResource RefCounted. If no one objects, I'll address comments and land this patch, then open a new bug to standardize CachedResource's lifetime management.
Created attachment 121934 [details] Are these ASSERTs enough? Moved the definition of m_switchingClientsToRevalidatedResource next to other 1-bit bools. Added ASSERTs in CachedResource::switchClientsToRevalidatedResource() and MemoryCache::revalidationSucceeded().
Comment on attachment 121934 [details] Are these ASSERTs enough? Rejecting attachment 121934 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: re/loader/SubresourceLoader.cpp Hunk #1 FAILED at 157. Hunk #2 succeeded at 265 (offset 13 lines). 1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/loader/SubresourceLoader.cpp.rej patching file Source/WebCore/loader/SubresourceLoader.h Hunk #1 FAILED at 77. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/loader/SubresourceLoader.h.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/11267280
Created attachment 122805 [details] Patch for landing
Comment on attachment 122805 [details] Patch for landing Clearing flags on attachment: 122805 Committed r105226: <http://trac.webkit.org/changeset/105226>
All reviewed patches have been landed. Closing bug.