Bug 87743 - Crash in WebCore::SubresourceLoader::releaseResources when connection fails
Summary: Crash in WebCore::SubresourceLoader::releaseResources when connection fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.7
: P2 Major
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-05-29 07:04 PDT by Dimitris Apostolou
Modified: 2013-05-28 22:29 PDT (History)
12 users (show)

See Also:


Attachments
Test app. (286.50 KB, application/x-tar)
2012-06-14 16:00 PDT, Dimitris Apostolou
no flags Details
Do less in releaseResources() (11.97 KB, patch)
2012-06-22 16:37 PDT, Nate Chapin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fix chromium compile (12.66 KB, patch)
2012-06-25 09:38 PDT, Nate Chapin
koivisto: review-
koivisto: commit-queue-
Details | Formatted Diff | Diff
Merged to trunk, functions renamed per antti's suggestions (14.06 KB, patch)
2012-12-05 10:36 PST, Nate Chapin
buildbot: commit-queue-
Details | Formatted Diff | Diff
Merged to trunk again (12.71 KB, patch)
2013-02-21 14:17 PST, Nate Chapin
darin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
merged to trunk again (12.75 KB, patch)
2013-05-28 09:41 PDT, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitris Apostolou 2012-05-29 07:04:25 PDT
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.
Comment 1 Alexey Proskuryakov 2012-05-29 09:40:32 PDT
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.
Comment 2 Dimitris Apostolou 2012-06-11 18:06:25 PDT
Bug is still here with today's ML seed (12A239)
Comment 3 Alexey Proskuryakov 2012-06-11 18:12:34 PDT
<rdar://problem/11642828>
Comment 4 Alexey Proskuryakov 2012-06-12 16:36:48 PDT
Nate, did you have a chance to look into this?
Comment 5 Dimitris Apostolou 2012-06-12 16:42:12 PDT
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 :)
Comment 6 Brady Eidson 2012-06-12 16:48:34 PDT
(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.
Comment 7 Alexey Proskuryakov 2012-06-12 17:04:58 PDT
Finding a workaround for your code would be fair game though.
Comment 8 Nate Chapin 2012-06-13 09:32:05 PDT
(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.
Comment 9 Nate Chapin 2012-06-14 15:08:27 PDT
(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?
Comment 10 Dimitris Apostolou 2012-06-14 15:34:59 PDT
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.
Comment 11 Dimitris Apostolou 2012-06-14 16:00:08 PDT
Created attachment 147670 [details]
Test app.

Attaching test app.
Comment 12 Brady Eidson 2012-06-14 16:04:17 PDT
Actually in radar as <rdar://problem/11106873>
Comment 13 Brady Eidson 2012-06-14 16:11:33 PDT
> - (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.
Comment 14 Dimitris Apostolou 2012-06-14 18:59:19 PDT
Yes, we got the same suggestion from the Finnish guy at WWDC WebKit lab.

We applied and no longer crash.

Thanks :)
Comment 15 Nate Chapin 2012-06-22 13:21:57 PDT
(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?
Comment 16 Alexey Proskuryakov 2012-06-22 13:36:56 PDT
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.
Comment 17 Nate Chapin 2012-06-22 13:41:40 PDT
(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.
Comment 18 Nate Chapin 2012-06-22 16:37:37 PDT
Created attachment 149138 [details]
Do less in releaseResources()

This is an attempt at strategy 2 from comment #17
Comment 19 WebKit Review Bot 2012-06-22 17:55:56 PDT
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
Comment 20 Nate Chapin 2012-06-25 09:38:59 PDT
Created attachment 149309 [details]
Fix chromium compile
Comment 21 Brent Fulgham 2012-11-19 22:53:40 PST
This seems like a reasonable change.  Any reason it's been dormant for 5 months?
Comment 22 Antti Koivisto 2012-11-20 03:07:15 PST
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?
Comment 23 Yong Li 2012-12-04 12:48:09 PST
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.
Comment 24 Yong Li 2012-12-04 12:55:11 PST
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.
Comment 25 Nate Chapin 2012-12-05 10:36:40 PST
Created attachment 177790 [details]
Merged to trunk, functions renamed per antti's suggestions
Comment 26 Build Bot 2012-12-05 14:50:56 PST
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
Comment 27 WebKit Review Bot 2012-12-05 17:05:03 PST
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
Comment 28 WebKit Review Bot 2012-12-05 18:12:59 PST
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
Comment 29 Nate Chapin 2013-02-21 14:17:48 PST
Created attachment 189600 [details]
Merged to trunk again
Comment 30 WebKit Commit Bot 2013-05-26 18:07:29 PDT
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
Comment 31 Alexey Proskuryakov 2013-05-28 09:41:48 PDT
Created attachment 203063 [details]
merged to trunk again

Let's check EWS again.
Comment 32 WebKit Commit Bot 2013-05-28 22:29:44 PDT
Comment on attachment 203063 [details]
merged to trunk again

Clearing flags on attachment: 203063

Committed r150867: <http://trac.webkit.org/changeset/150867>
Comment 33 WebKit Commit Bot 2013-05-28 22:29:49 PDT
All reviewed patches have been landed.  Closing bug.