Bug 75713 - Cleanup 304 handing after r102602
Summary: Cleanup 304 handing after r102602
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: 2012-01-06 10:19 PST by Nate Chapin
Modified: 2012-01-17 18:07 PST (History)
2 users (show)

See Also:


Attachments
patch (4.87 KB, patch)
2012-01-06 12:12 PST, Nate Chapin
abarth: review+
Details | Formatted Diff | Diff
Are these ASSERTs enough? (5.84 KB, patch)
2012-01-10 16:21 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (6.14 KB, patch)
2012-01-17 13:30 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 2012-01-06 10:19:24 PST
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.
Comment 1 Nate Chapin 2012-01-06 12:12:52 PST
Created attachment 121463 [details]
patch
Comment 2 Adam Barth 2012-01-09 14:36:16 PST
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 3 Alexey Proskuryakov 2012-01-09 15:51:02 PST
Comment on attachment 121463 [details]
patch

I vaguely feel that a change like this should come with a few new ASSERTs.
Comment 4 Nate Chapin 2012-01-09 15:53:37 PST
(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 :)
Comment 5 Nate Chapin 2012-01-10 09:57:21 PST
(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.
Comment 6 Nate Chapin 2012-01-10 16:21:43 PST
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 7 WebKit Review Bot 2012-01-17 12:43:17 PST
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
Comment 8 Nate Chapin 2012-01-17 13:30:42 PST
Created attachment 122805 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-01-17 18:07:02 PST
Comment on attachment 122805 [details]
Patch for landing

Clearing flags on attachment: 122805

Committed r105226: <http://trac.webkit.org/changeset/105226>
Comment 10 WebKit Review Bot 2012-01-17 18:07:07 PST
All reviewed patches have been landed.  Closing bug.