Bug 28473 – Crashes on sites with lots of images

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[David Levin]

This occurs as a result of http://trac.webkit.org/changeset/42896

The problem is that m_resourceToRevalidate::m_isBeingRevalidated is false while CachedResource is still referring to it, so it may get deleted before Cache::revalidationSucceeded calls CachedResource::clearResourceToRevalidate.

Specially, m_resourceToRevalidate::didAddClient may result in a call to m_resourceToRevalidate::removeClient which may call Cache::prune and delete m_resourceToRevalidate.
 
Here's a typical call stack for the crash in chromium:
	WebCore::CachedResource::clearResourceToRevalidate()  Line 313
 	WebCore::Cache::revalidationSucceeded
 	WebCore::Loader::Host::didReceiveResponse
 	WebCore::SubresourceLoader::didReceiveResponse
 	WebCore::ResourceLoader::didReceiveResponse
 	WebCore::ResourceHandleInternal::didReceiveResponse

Note that this requires a pretty full cache to repro.

http://code.google.com/p/chromium/issues/detail?id=15375

[David Levin]

One site that seemed to expose this is http://madbadger.net/index.php

[David Levin]

Created attachment 35148 [details]
Proposed fix.

Free free to r- for lack of tests :) but I wanted to get feedback about the patch without that part.

For the layout tests, here's what I'm thinking about (approximately and it will take a bit of tweaking for me to hit these conditions precisely):
  Add a new cache model (or see if I can do it with existing models) and allow it to be selected from LayoutTestController.
  Load a bunch of images to hit the boundary and expose this issue.

Feel free to give feedback on that approach as well.

[Darin Adler]

Comment on attachment 35148 [details]
Proposed fix.

> +        better reflect its expanded roll.

role

This patch looks good to me. Antti may have some thoughts. He's sort of an expert on the cache/loader.

[Dimitri Glazkov (Google)]

Dude, I can't believe you figured this one out.

[Alexey Proskuryakov]

Comment on attachment 35148 [details]
Proposed fix.

+        better reflect its expanded roll.

role

I think the patch is very good. The only concern I have is that using CachedResourceHandle just as a protector is abuse - we discussed some other approaches on IRC, maybe they will lead to a better solution.

Also, making cache testable may be out of scope for this patch. But that's something we sorely lack, for sure.

[David Levin]

Comment on attachment 35148 [details]
Proposed fix.

I'll try to find another way to do this without abusing CachedResourceHandle.

A few ideas mentioned by ap:
* move switchToRevalidatedResource() before the original resource back to cache
* there are several checks in pruneDeadResources() already, maybe we could add another check there?
* zero out m_resourceToRevalidate at the time it's deleted using a parent pointer.

[Antti Koivisto]

What Alexey said. Looks great except for the questionable use of CachedResourceHandle which shouldn't be used as a stack variable (it would be nice to turn CachedResources to real refcounted class though).

[Alexey Proskuryakov]

Bugzilla has experienced amnesia today. This bug was fixed by Dave in <http://trac.webkit.org/changeset/47602>.