Bug 28473 - Crashes on sites with lots of images
Summary: Crashes on sites with lots of images
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
Depends on:
Reported: 2009-08-19 14:42 PDT by David Levin
Modified: 2009-08-20 22:57 PDT (History)
3 users (show)

See Also:

Proposed fix. (5.60 KB, patch)
2009-08-19 15:00 PDT, David Levin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2009-08-19 14:42:07 PDT
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

Note that this requires a pretty full cache to repro.

Comment 1 David Levin 2009-08-19 14:52:40 PDT
One site that seemed to expose this is http://madbadger.net/index.php
Comment 2 David Levin 2009-08-19 15:00:53 PDT
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.
Comment 3 Darin Adler 2009-08-19 15:54:02 PDT
Comment on attachment 35148 [details]
Proposed fix.

> +        better reflect its expanded roll.


This patch looks good to me. Antti may have some thoughts. He's sort of an expert on the cache/loader.
Comment 4 Dimitri Glazkov (Google) 2009-08-19 16:06:46 PDT
Dude, I can't believe you figured this one out.
Comment 5 Alexey Proskuryakov 2009-08-19 17:12:19 PDT
Comment on attachment 35148 [details]
Proposed fix.

+        better reflect its expanded roll.


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.
Comment 6 David Levin 2009-08-19 17:13:15 PDT
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.
Comment 7 Antti Koivisto 2009-08-19 21:09:46 PDT
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).
Comment 8 Alexey Proskuryakov 2009-08-20 22:57:40 PDT
Bugzilla has experienced amnesia today. This bug was fixed by Dave in <http://trac.webkit.org/changeset/47602>.