Bug 112811 - REGRESSION (r146239): Reproducible crash in WebCore::DocumentLoader::responseReceived
Summary: REGRESSION (r146239): Reproducible crash in WebCore::DocumentLoader::response...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.8
: P1 Normal
Assignee: Nate Chapin
URL: https://rubygems.org/gems/git-svn-mirror
Keywords: InRadar, Regression
Depends on: 112722
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-20 07:43 PDT by Dieter Komendera
Modified: 2013-03-22 10:07 PDT (History)
5 users (show)

See Also:


Attachments
full crash report (58.10 KB, text/plain)
2013-03-20 07:44 PDT, Dieter Komendera
no flags Details
patch (4.98 KB, patch)
2013-03-21 16:03 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Attempt to prevent identifier of 0 (6.37 KB, patch)
2013-03-21 17:07 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (6.46 KB, patch)
2013-03-22 09:18 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dieter Komendera 2013-03-20 07:43:29 PDT
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
Comment 1 Dieter Komendera 2013-03-20 07:44:13 PDT
Created attachment 194052 [details]
full crash report
Comment 2 Alexey Proskuryakov 2013-03-20 09:36:13 PDT
<rdar://problem/13462658>
Comment 3 Alexey Proskuryakov 2013-03-20 09:43:05 PDT
Bisecting says r146238-146246, with r146239 being the only relevant change in this range.

Nate, Antti, could you please take a look?
Comment 4 Nate Chapin 2013-03-21 16:03:32 PDT
Created attachment 194370 [details]
patch
Comment 5 Alexey Proskuryakov 2013-03-21 16:10:31 PDT
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.
Comment 6 Nate Chapin 2013-03-21 16:14:57 PDT
(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.
Comment 7 Nate Chapin 2013-03-21 17:07:42 PDT
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.
Comment 8 Brady Eidson 2013-03-21 17:13:29 PDT
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
Comment 9 Nate Chapin 2013-03-22 09:18:25 PDT
Created attachment 194566 [details]
Patch for landing
Comment 10 WebKit Review Bot 2013-03-22 10:07:38 PDT
Comment on attachment 194566 [details]
Patch for landing

Clearing flags on attachment: 194566

Committed r146626: <http://trac.webkit.org/changeset/146626>
Comment 11 WebKit Review Bot 2013-03-22 10:07:42 PDT
All reviewed patches have been landed.  Closing bug.