Crash in WebCore::SubresourceLoader::releaseResources when connection fails

[Dimitris Apostolou]

Reproducibility: always

Steps:
1. Have Safari 5.2 installed.
2. Launch Skype 5.7
3. Click on Skype Home tab in the sidebar and immediately cut the internet connection (e.g. unplug ethernet cable or switch off Airport).

What happened:
3. Crash.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000144
0x03309aef in WebCore::SubresourceLoader::releaseResources ()
(gdb) bt
#0  0x03309aef in WebCore::SubresourceLoader::releaseResources ()
#1  0x0290844a in WebCore::ResourceLoader::didFail ()
#2  0x0290805d in WebCore::SubresourceLoader::didFail ()
#3  0x02907d6a in WebCore::ResourceLoader::didFail ()
#4  0x02907d07 in -[WebCoreResourceHandleAsDelegate connection:didFailWithError:] ()
#5  0x9388f652 in -[NSURLConnectionDelegateProxy connection:didFailWithError:] ()
#6  0x9388a3c7 in __51-[NSURLConnectionInternal _withErrorForConnection:]_block_invoke_0 ()
#7  0x93889fec in __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke_0 ()
#8  0x9388b222 in -[NSURLConnectionInternalConnection invokeForDelegate:] ()
#9  0x93889fa7 in -[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:] ()
#10 0x9388a342 in -[NSURLConnectionInternal _withErrorForConnection:] ()
#11 0x9388ad25 in _NSURLConnectionDidFail ()
#12 0x92e701c4 in ___delegate_didFail_block_invoke_0 ()
#13 0x92e6f3df in ___withDelegateAsync_block_invoke_0 ()
#14 0x92eb2da3 in __block_global_1 ()
#15 0x90ddb5c0 in CFArrayApplyFunction ()
#16 0x92eb3529 in RunloopBlockContext::perform ()
#17 0x92eb3495 in non-virtual thunk to RunloopBlockContext::multiplexerClientPerform() ()
#18 0x92d947a3 in MultiplexerSource::perform ()
#19 0x90dba83f in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ ()
#20 0x90dba269 in __CFRunLoopDoSources0 ()
#21 0x90ddfef6 in __CFRunLoopRun ()
#22 0x90ddf6ea in CFRunLoopRunSpecific ()
#23 0x90ddf55b in CFRunLoopRunInMode ()
#24 0x993da8a6 in RunCurrentEventLoopInMode ()
#25 0x993e2351 in ReceiveNextEventCommon ()
#26 0x993e21cc in BlockUntilNextEventMatchingListInMode ()
#27 0x95157302 in _DPSNextEvent ()
#28 0x95156b37 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#29 0x951529bc in -[NSApplication run] ()
#30 0x000e641b in SkypeApplicationMain [inlined] () at :85
#31 0x000e641b in main (argv=0x0, argc=485365392) at /Volumes/ServerHD2/buildagent/workspace/17915/SkypeMac/main.m:72

Expected result:
WebKit does not crash.

[Alexey Proskuryakov]

Nate, this sounds related to some of the refactorings you did. would you be willing to take a look?

There is a run-webkit-app script to easily run arbitrary apps against locally built WebKit.

[Dimitris Apostolou]

Bug is still here with today's ML seed (12A239)

[Alexey Proskuryakov]

<rdar://problem/11642828>

[Alexey Proskuryakov]

Nate, did you have a chance to look into this?

[Dimitris Apostolou]

Btw, I am going to Safari and WebKit lab tomorrow at WWDC with sample code that crashes it always and I hope guys there can fix it on spot :)

[Brady Eidson]

(In reply to comment #5)
> Btw, I am going to Safari and WebKit lab tomorrow at WWDC with sample code that crashes it always and I hope guys there can fix it on spot :)

Just to give you fair warning I think you misunderstand the point of the lab.  It's to get you help with bugs in your code, not for Apple to comment in any way on upcoming changes in their products.

[Alexey Proskuryakov]

Finding a workaround for your code would be fair game though.

[Nate Chapin]

(In reply to comment #4)
> Nate, did you have a chance to look into this?

Sorry, I was out sick early this week and fell behind. I'll get to this asap.

[Nate Chapin]

(In reply to comment #1)
> Nate, this sounds related to some of the refactorings you did. would you be willing to take a look?
> 
> There is a run-webkit-app script to easily run arbitrary apps against locally built WebKit.

I've thus far been unable to reproduce this via the steps provided, using both release and debug trunk webkit builds.

Is this specific to Skype 5.7? I believe I got 5.8.0.865 from skype.com, and I couldn't find older versions, so maybe that's why I can't reproduce. Did I miss something on skype.com, or is there a reputable source from which I can get 5.7?

[Dimitris Apostolou]

It reproduces always in 5.8 too. Basically it reproduces in any application that tries to load some html page and you cut the internet connection while it loads.

If it helps I can attach a test Xcode app that I have which crashes it always here.

Please make sure you are on Mountain Lion. It is very reproducible there.

[Dimitris Apostolou]

Created attachment 147670 [details]
Test app.

Attaching test app.

[Brady Eidson]

Actually in radar as <rdar://problem/11106873>

[Brady Eidson]

> - (void)webView:(WebView *)sender resource:(id)identifier didFailLoadingWithError:(NSError *)error fromDataSource:(WebDataSource *)dataSource;
> {
>     NSString *html = @"<html><body><h1>error</h1></body></html>";
>     [[self.webview mainFrame] loadHTMLString:html baseURL:[NSURL URLWithString:@"http://apps.skype.com"]];
> }


You are synchronously re-entering WebKit from inside a delegate callback.  Don't do that.  Put the loadHTMLString on a 0-delay.

[Dimitris Apostolou]

Yes, we got the same suggestion from the Finnish guy at WWDC WebKit lab.

We applied and no longer crash.

Thanks :)

[Nate Chapin]

(In reply to comment #13)
> > - (void)webView:(WebView *)sender resource:(id)identifier didFailLoadingWithError:(NSError *)error fromDataSource:(WebDataSource *)dataSource;
> > {
> >     NSString *html = @"<html><body><h1>error</h1></body></html>";
> >     [[self.webview mainFrame] loadHTMLString:html baseURL:[NSURL URLWithString:@"http://apps.skype.com"]];
> > }
> 
> 
> You are synchronously re-entering WebKit from inside a delegate callback.  Don't do that.  Put the loadHTMLString on a 0-delay.

Do we want to add some reachedTerminalState() checks in ResourceLoader to prevent this from crashing, or should we just close this bug now?

[Alexey Proskuryakov]

Since it's a regression, and there is nothing in API documentation saying that one must not re-enter, this still looks like a valid bug to me.

[Nate Chapin]

(In reply to comment #16)
> Since it's a regression, and there is nothing in API documentation saying that one must not re-enter, this still looks like a valid bug to me.

Ok, I think there actually 2 ways to resolve this:
1. Add early exits if m_reachedTerminalState if true in didFail() and didFinishLoading().
2. As you've suggested previously, change SubresourceLoader::releaseResources() to only clear variables, not heavy lifting like calling CachedResourceLoader::loadDone().

#2 is more extensive and would probably still allow cases like this to call releaseResources() twice (and therefore trigger assertions in debug), but would make calling releaseResources() multiple times benign.

[Nate Chapin]

Created attachment 149138 [details]
Do less in releaseResources()

This is an attempt at strategy 2 from comment #17

[WebKit Review Bot]

Comment on attachment 149138 [details]
Do less in releaseResources()

Attachment 149138 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13039521

[Nate Chapin]

Created attachment 149309 [details]
Fix chromium compile

[Brent Fulgham]

This seems like a reasonable change.  Any reason it's been dormant for 5 months?

[Antti Koivisto]

Comment on attachment 149309 [details]
Fix chromium compile

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

Seems like a sensible change. Naming should be improved though.

> Source/WebCore/loader/cache/CachedResource.h:205
> +    void cancelled();

Loader code has hots of bad legacy naming but we shouldn't add more. Please follow the usual naming rules. This should be something like cancelLoad().

> Source/WebCore/loader/ResourceLoader.h:160
> +        bool cancelled() const { return m_cancellationStatus >= Cancelled; }

Here is another cancelled() now with bool return. It should be along the lines of wasCanceled().

> Source/WebCore/loader/SubresourceLoader.h:78
> +    void finish();

These need more descriptive name as well. Finish what? Finish how?

[Yong Li]

Comment on attachment 149309 [details]
Fix chromium compile

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

> Source/WebCore/loader/SubresourceLoader.cpp:336
> +    m_requestCountTracker.clear();
> +    m_document->cachedResourceLoader()->loadDone();
> +    if (reachedTerminalState())
> +        return;
> +    m_documentLoader->removeSubresourceLoader(this);

Can we call m_documentLoader->removeSubresourceLoader(this); before calling m_document->cachedResourceLoader()->loadDone(); ?

This will fix https://bugs.webkit.org/show_bug.cgi?id=100791. loadDone() may trigger a start() on the same SubresourceLoader.

[Yong Li]

Comment on attachment 149309 [details]
Fix chromium compile

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

>> Source/WebCore/loader/SubresourceLoader.cpp:336
>> +    m_documentLoader->removeSubresourceLoader(this);
> 
> Can we call m_documentLoader->removeSubresourceLoader(this); before calling m_document->cachedResourceLoader()->loadDone(); ?
> 
> This will fix https://bugs.webkit.org/show_bug.cgi?id=100791. loadDone() may trigger a start() on the same SubresourceLoader.

hm... I thought removeSubresourceLoader was resourceScheduler->remove. never mind.

[Nate Chapin]

Created attachment 177790 [details]
Merged to trunk, functions renamed per antti's suggestions

[Build Bot]

Comment on attachment 177790 [details]
Merged to trunk, functions renamed per antti's suggestions

Attachment 177790 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15160357

New failing tests:
fast/workers/worker-gc2.html

[WebKit Review Bot]

Comment on attachment 177790 [details]
Merged to trunk, functions renamed per antti's suggestions

Attachment 177790 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15170267

New failing tests:
fast/workers/worker-gc2.html

[WebKit Review Bot]

Comment on attachment 177790 [details]
Merged to trunk, functions renamed per antti's suggestions

Attachment 177790 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15148719

New failing tests:
fast/workers/worker-gc2.html

[Nate Chapin]

Created attachment 189600 [details]
Merged to trunk again

[WebKit Commit Bot]

Comment on attachment 189600 [details]
Merged to trunk again

Rejecting attachment 189600 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 189600, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ource/WebCore/loader/chromium/ResourceLoaderChromium.cpp	(working copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
patching file Source/WebCore/loader/SubresourceLoader.cpp
Hunk #1 succeeded at 280 (offset 4 lines).
Hunk #2 succeeded at 305 (offset 4 lines).
Hunk #3 succeeded at 330 (offset 4 lines).

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/676334

[Alexey Proskuryakov]

Created attachment 203063 [details]
merged to trunk again

Let's check EWS again.

[WebKit Commit Bot]

Comment on attachment 203063 [details]
merged to trunk again

Clearing flags on attachment: 203063

Committed r150867: <http://trac.webkit.org/changeset/150867>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.