Bug 75713 – Cleanup 304 handing after r102602

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

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.

[Nate Chapin]

Created attachment 121463 [details]
patch

[Adam Barth]

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?

[Alexey Proskuryakov]

Comment on attachment 121463 [details]
patch

I vaguely feel that a change like this should come with a few new ASSERTs.

[Nate Chapin]

(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 :)

[Nate Chapin]

(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.

[Nate Chapin]

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().

[WebKit Review Bot]

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

[Nate Chapin]

Created attachment 122805 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 122805 [details]
Patch for landing

Clearing flags on attachment: 122805

Committed r105226: <http://trac.webkit.org/changeset/105226>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.