REGRESSION (r146239): Reproducible crash in WebCore::DocumentLoader::responseReceived

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dieter Komendera]

Tested with WebKit nightly r146302 on 10.8.3

To reproduce:

* open https://rubygems.org/gems/git-svn-mirror in a new tab
* close that tab
* open https://rubygems.org/gems/git-svn-mirror again

0   com.apple.WebCore             	0x000000010f9c1706 WebCore::DocumentLoader::responseReceived(WebCore::CachedResource*, WebCore::ResourceResponse const&) + 246
1   com.apple.WebCore             	0x000000010f85f8e5 WebCore::CachedRawResource::didAddClient(WebCore::CachedResourceClient*) + 661
2   com.apple.WebCore             	0x000000010f8636a1 WebCore::CachedResource::switchClientsToRevalidatedResource() + 641
3   com.apple.WebCore             	0x00000001101108f5 WebCore::MemoryCache::revalidationSucceeded(WebCore::CachedResource*, WebCore::ResourceResponse const&) + 325
4   com.apple.WebCore             	0x0000000110404e2f WebCore::SubresourceLoader::didReceiveResponse(WebCore::ResourceResponse const&) + 95
5   com.apple.WebCore             	0x000000011030ec2e -[WebCoreResourceHandleAsDelegate connection:didReceiveResponse:] + 334
6   com.apple.Foundation          	0x00007fff85fc2528 __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke_0 + 28
7   com.apple.Foundation          	0x00007fff85fc246c -[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:] + 227
8   com.apple.Foundation          	0x00007fff85fc2368 -[NSURLConnectionInternal _withActiveConnectionAndDelegate:] + 63
9   com.apple.Foundation          	0x00007fff85fc2691 _NSURLConnectionDidReceiveResponse + 82

[Dieter Komendera]

Created attachment 194052 [details]
full crash report

[Alexey Proskuryakov]

<rdar://problem/13462658>

[Alexey Proskuryakov]

Bisecting says r146238-146246, with r146239 being the only relevant change in this range.

Nate, Antti, could you please take a look?

[Nate Chapin]

Created attachment 194370 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 194370 [details]
patch

What ensures that the identifier won't stay 0? We hate when identifiers are 0.

Perhaps a ChangeLog explanation would help.

[Nate Chapin]

(In reply to comment #5)
> (From update of attachment 194370 [details])
> What ensures that the identifier won't stay 0? We hate when identifiers are 0.
> 
> Perhaps a ChangeLog explanation would help.

I'm trying to figure out how to properly ensure that, because it looks like 0 identifiers where happening in this case before it switched to crashing.

The problem is that, by the time responseReceived() is called in the 304 case, we've already swapped to the revalidated CachedResource, which means we've lost our connection to the proper identifier.

[Nate Chapin]

Created attachment 194390 [details]
Attempt to prevent identifier of 0

Ok, I *think* this does the job.  In a normal load, we set a main resource's identifier on the CachedRawResource when we receive a response, but we don't do so in the revalidation case.  Since a resource can't have multiple revalidations at once and can't be revalidated if it's still loading, it should be safe to copy the identifier to the revalidated resource, even though that overwrites its old identifier.

[Brady Eidson]

Comment on attachment 194390 [details]
Attempt to prevent identifier of 0

View in context: https://bugs.webkit.org/attachment.cgi?id=194390&action=review

r+ with the following change

> Source/WebCore/loader/cache/CachedRawResource.h:69
> +    virtual void switchClientsToRevalidatedResource();

OVERRIDE

[Nate Chapin]

Created attachment 194566 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 194566 [details]
Patch for landing

Clearing flags on attachment: 194566

Committed r146626: <http://trac.webkit.org/changeset/146626>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.