Bug 12479

Summary: ASSERTION FAILURE: resource->inCache() in WebCore::Cache::remove
Product: WebKit Reporter: Matt Lilek <dev+webkit>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, mitz, timothy
Priority: P1 Keywords: HasReduction, InRadar
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Reduction
none
Simpler reduction
none
Replace the ASSERT with a check and allow simultaneous reloads mjs: review+

Description Matt Lilek 2007-01-30 09:31:48 PST
I've only seen this assertion failure on Colloquy's web plugin, but it's producable by reloading that page a few times.

ASSERTION FAILED: resource->inCache()
(/Users/matt/Code/WebKit/WebCore/loader/Cache.cpp:163 void WebCore::Cache::remove(WebCore::CachedResource*))

Thread 0 Crashed:
0   com.apple.WebCore        	0x011545c0 WebCore::Cache::remove(WebCore::CachedResource*) + 104 (Cache.cpp:163)
1   com.apple.WebCore        	0x01159c4c WebCore::Loader::didFail(WebCore::SubresourceLoader*, WebCore::ResourceError const&) + 448 (loader.cpp:135)
2   com.apple.WebCore        	0x014f3bb4 WebCore::SubresourceLoader::didFail(WebCore::ResourceError const&) + 212 (SubresourceLoader.cpp:186)
3   com.apple.WebCore        	0x014f1e98 WebCore::ResourceLoader::didFail(WebCore::ResourceHandle*, WebCore::ResourceError const&) + 68
4   com.apple.WebCore        	0x014c7fdc -[WebCoreResourceHandleAsDelegate connection:didFailWithError:] + 164 (ResourceHandleMac.mm:368)
5   com.apple.Foundation     	0x929ba110 -[NSURLConnection(NSURLConnectionInternal) _sendDidFailCallback] + 100
6   com.apple.Foundation     	0x9298fab8 -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks] + 556
7   com.apple.Foundation     	0x9298f810 _sendCallbacks + 156
Comment 1 mitz 2007-01-30 10:08:00 PST
Heh, just when I close bug 11380 :)
Comment 2 Matt Lilek 2007-01-30 11:51:50 PST
D'oh! I totally missed that.
Comment 3 Mark Rowe (bdash) 2007-02-01 18:41:39 PST
<rdar://problem/4971219>
Comment 4 mitz 2007-02-07 11:21:31 PST
Bug 12652 has steps to reproduce.
Comment 5 Matt Lilek 2007-02-07 22:17:01 PST
Created attachment 13044 [details]
Reduction

1. Load the testcase.
2. From Safari's Debug menu, open the Caches window and tick the "Disable WebCore Caches" checkbox.
3. Click ASSERT!
4. WebKit prints the above assert.
Comment 6 mitz 2007-02-16 15:01:10 PST
Created attachment 13206 [details]
Simpler reduction

Just hit Command-R to trigger the ASSERT.

When you reload, the first Iframe creates a cached resource for the image and it begins to load. Then the second Iframe needs the same URL, but since it's a reload it wants a fresh copy, so it checks if there is a cached resource, and seeing that there is one (put there by the first Iframe), it removes it from the cache. Now the loader learns that the loading failed (since the file doesn't exist) so it wants to remove the resource from the cache, and that's when the assert is hit (because the second frame has already removed it).
Comment 7 mitz 2007-02-16 15:38:59 PST
The reason this doesn't happen with, say, two images with the same src in the same document is that a DocLoader maintains a list of reloaded URLs, so it will not issue multiple requests to load the same URL.

I can think of two ways to address this bug:

1) Say that it's okay that separate documents (which can be in different iframes in the same parent document or in two different windows) reload separately, remove the ASSERT, and in the case that the resource is not in the cache skip straight to the canDelete() check. This will have the side effect of actually doing two loads of the same resource in parallel instead of having the second frame/window join in on the reload that's in progress.

2) Manage the "reloaded URLs" or "reloading URLs" list/status in the Cache. Clients will need to tell the cache that they want a resource reloaded, and the cache will see if a reload of the URL is in progress. If so, it will hand back the reloading resource. Otherwise, it will remove an existing resource (if any) and create a new one. The part I'm unclear about here is when a resource should transition from "reloading" to "loaded" status.
Comment 8 mitz 2007-02-16 15:40:58 PST
(In reply to comment #7)
> This will have the side effect of
> actually doing two loads of the same resource in parallel instead of having the
> second frame/window join in on the reload that's in progress.

That's how WebKit behaves currently, of course.
Comment 9 mitz 2007-02-17 02:41:29 PST
Created attachment 13210 [details]
Replace the ASSERT with a check and allow simultaneous reloads

Includes change log and a layout test (for debug builds only). Implements option 1) from comment #7.
Comment 10 Maciej Stachowiak 2007-02-17 08:23:59 PST
Comment on attachment 13210 [details]
Replace the ASSERT with a check and allow simultaneous reloads

r=me
Comment 11 Adele Peterson 2007-02-17 11:14:02 PST
Committed revision 19683.