Bug 110075 – REGRESSION: Crash in MainResourceLoader::setDataBufferingPolicy when sharing an html5 video via email

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Jim Oase]

goto http://ia600208.us.archive.org/13/items/WiththeMarinesatTarawa/With_the_Marines_at_Tarawa_1944.mpeg

Select share via email

Webkit will crash

[Alexey Proskuryakov]

Result of MainDocumentLoader refactoring?

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010286c3e8 WebCore::MainResourceLoader::setDataBufferingPolicy(WebCore::DataBufferingPolicy) + 8
1   com.apple.WebCore             	0x00000001021bd8de WebCore::MediaDocumentParser::createDocumentStructure() + 894
2   com.apple.WebCore             	0x000000010287cd4f WebCore::MediaDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, unsigned long) + 31
3   com.apple.WebCore             	0x0000000102383f0c WebCore::DocumentLoader::commitData(char const*, unsigned long) + 508
4   com.apple.WebCore             	0x0000000102383cc7 WebCore::DocumentLoader::maybeCreateArchive() + 455
5   com.apple.WebCore             	0x0000000101dd65f1 WebCore::DocumentLoader::finishedLoading() + 81
6   com.apple.WebCore             	0x0000000101e47f28 WebCore::MainResourceLoader::didFinishLoading(double) + 312
7   com.apple.WebCore             	0x0000000101e42311 WebCore::MainResourceLoader::continueAfterContentPolicy(WebCore::PolicyAction, WebCore::ResourceResponse const&) + 1281
8   com.apple.WebCore             	0x000000010286bc50 WebCore::MainResourceLoader::responseReceived(WebCore::CachedResource*, WebCore::ResourceResponse const&) + 1696

[Alexey Proskuryakov]

<rdar://problem/13235726>

[Nate Chapin]

Created attachment 189151 [details]
patch

Tested manually, don't know enough about Safari's sharing feature to know whether it's possible to convert this to a layout test.

[Alexey Proskuryakov]

I suspect that this happens when loading a WebArchive with a <video> in it. Perhaps that would be possible to test?

[Nate Chapin]

(In reply to comment #4)
> I suspect that this happens when loading a WebArchive with a <video> in it. Perhaps that would be possible to test?

I can't find an example of a case where the webarchive is the test, rather than the output. Am I missing something obvious?

[Alexey Proskuryakov]

I think that there is a bunch of tests in webarchive/loading, e.g. webarchive/loading/test-loading-archive.html. It does take two steps, as DRT doesn't look for tests with webarchive extension.

[Nate Chapin]

(In reply to comment #6)
> I think that there is a bunch of tests in webarchive/loading, e.g. webarchive/loading/test-loading-archive.html. It does take two steps, as DRT doesn't look for tests with webarchive extension.

Thanks for the pointer. Looks like I can make that style of test work, though it'll take a little bit of messing around to get it just right.

[Nate Chapin]

Created attachment 189411 [details]
+ test

[Jim Oase]

Nightly build r143504 did not display the menu for sharing the email of 
http://ia600208.us.archive.org/13/items/WiththeMarinesatTarawa/With_the_Marines_at_Tarawa_1944.mpeg

Opening the current released version of Safari to the same site would not display the video but did display the share menu.

Returning to WebKit r143504  the video was visible, the share email menu was not… then suddenly Webkit crashed

[Alexey Proskuryakov]

Comment on attachment 189411 [details]
+ test

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

> Source/WebCore/ChangeLog:28
> [phone]  Nate Chapin  <[e-mail]>
> +
> +       
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * loader/MainResourceLoader.cpp:
> +        (WebCore::MainResourceLoader::setDataBufferingPolicy): m_resource can be null
> +           in a reasonable case, handle it rather than asserting that it can't be null.

Parts of a previous ChangeLog here.

> LayoutTests/webarchive/loading/video-in-webarchive-expected.txt:13
> +frame "<!--framePath //<!--frame0-->-->" - didFailLoadWithError

What fails here? Is that expected?

[Jim Oase]

Created attachment 189420 [details]
Additional information from latest build r143504

Sorry.. put this in without the attachment.

Nightly build r143504 did not display the menu for sharing the email of 
http://ia600208.us.archive.org/13/items/WiththeMarinesatTarawa/With_the_Marines_at_Tarawa_1944.mpeg

Opening the current released version of Safari to the same site would not display the video but did display the share menu.

Returning to WebKit r143504  the video was visible, the share email menu was not… then suddenly Webkit crashed

[Nate Chapin]

Comment on attachment 189411 [details]
+ test

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

>> LayoutTests/webarchive/loading/video-in-webarchive-expected.txt:13
>> +frame "<!--framePath //<!--frame0-->-->" - didFailLoadWithError
> 
> What fails here? Is that expected?

The iframe in the webarchive points to a 0-byte mpeg. I'd guess that's the cause, but I'm not 100% sure.

[Nate Chapin]

Created attachment 189426 [details]
Fix ChangeLog

[Alexey Proskuryakov]

Comment on attachment 189426 [details]
Fix ChangeLog

> The iframe in the webarchive points to a 0-byte mpeg. I'd guess that's the cause, but I'm not 100% sure.

This looks more like a bug than like correct behavior. This happens because we cancel main resource load when switching to a media document, and that somehow translates into a frame loader failure. But it's a resource load cancellation, not a frame load failure.


    // If the document is a stand-alone media document, now is the right time to cancel the WebKit load.
    // FIXME: This code should be shared across all ports. <http://webkit.org/b/48762>.
    if (m_frame->coreFrame()->document()->isMediaDocument())
        loader->cancelMainResourceLoad(pluginWillHandleLoadError(loader->response()));


#0	0x000000010128afcc in WebKit::WebFrameLoaderClient::dispatchDidFailLoading(WebCore::DocumentLoader*, unsigned long, WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:249
#1	0x00000001046df8af in WebCore::MainResourceLoader::receivedError(WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:99
#2	0x00000001046dfb27 in WebCore::MainResourceLoader::cancel(WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:132
#3	0x000000010399a11f in WebCore::DocumentLoader::cancelMainResourceLoad(WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/DocumentLoader.cpp:913
#4	0x000000010128db82 in WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) at /Users/ap/Safari/OpenSource/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:852
#5	0x0000000103997b20 in WebCore::DocumentLoader::commitLoad(char const*, int) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/DocumentLoader.cpp:318
#6	0x00000001039980cb in WebCore::DocumentLoader::receivedData(char const*, int) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/DocumentLoader.cpp:397
#7	0x00000001046e1f4a in WebCore::MainResourceLoader::dataReceived(WebCore::CachedResource*, char const*, int) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:522
#8	0x00000001046e0e20 in WebCore::MainResourceLoader::continueAfterContentPolicy(WebCore::PolicyAction, WebCore::ResourceResponse const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:365
#9	0x00000001046e1266 in WebCore::MainResourceLoader::continueAfterContentPolicy(WebCore::PolicyAction) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:381
#10	0x00000001046e11ab in WebCore::MainResourceLoader::callContinueAfterContentPolicy(void*, WebCore::PolicyAction) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:373
#11	0x00000001046e18c6 in WebCore::MainResourceLoader::responseReceived(WebCore::CachedResource*, WebCore::ResourceResponse const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:450
#12	0x00000001046df594 in WebCore::MainResourceLoader::handleSubstituteDataLoadNow(WebCore::RunLoopTimer<WebCore::MainResourceLoader>*) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:628

I don't know if that's a long-standing behavior, or something that got broken with MainResourceLoader refactoring recently. Would you be willing to take a look?

r+ for this patch.

[Alexey Proskuryakov]

Thank you for taking the effort to make a test - it's great that it helped you discover and fix an additional issue.

[Nate Chapin]

(In reply to comment #14)
> (From update of attachment 189426 [details])
> > The iframe in the webarchive points to a 0-byte mpeg. I'd guess that's the cause, but I'm not 100% sure.
> 
> This looks more like a bug than like correct behavior. This happens because we cancel main resource load when switching to a media document, and that somehow translates into a frame loader failure. But it's a resource load cancellation, not a frame load failure.
> 
> 
>     // If the document is a stand-alone media document, now is the right time to cancel the WebKit load.
>     // FIXME: This code should be shared across all ports. <http://webkit.org/b/48762>.
>     if (m_frame->coreFrame()->document()->isMediaDocument())
>         loader->cancelMainResourceLoad(pluginWillHandleLoadError(loader->response()));
> 
> 
> #0    0x000000010128afcc in WebKit::WebFrameLoaderClient::dispatchDidFailLoading(WebCore::DocumentLoader*, unsigned long, WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:249
> #1    0x00000001046df8af in WebCore::MainResourceLoader::receivedError(WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:99
> #2    0x00000001046dfb27 in WebCore::MainResourceLoader::cancel(WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:132
> #3    0x000000010399a11f in WebCore::DocumentLoader::cancelMainResourceLoad(WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/DocumentLoader.cpp:913
> #4    0x000000010128db82 in WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) at /Users/ap/Safari/OpenSource/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:852
> #5    0x0000000103997b20 in WebCore::DocumentLoader::commitLoad(char const*, int) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/DocumentLoader.cpp:318
> #6    0x00000001039980cb in WebCore::DocumentLoader::receivedData(char const*, int) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/DocumentLoader.cpp:397
> #7    0x00000001046e1f4a in WebCore::MainResourceLoader::dataReceived(WebCore::CachedResource*, char const*, int) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:522
> #8    0x00000001046e0e20 in WebCore::MainResourceLoader::continueAfterContentPolicy(WebCore::PolicyAction, WebCore::ResourceResponse const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:365
> #9    0x00000001046e1266 in WebCore::MainResourceLoader::continueAfterContentPolicy(WebCore::PolicyAction) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:381
> #10    0x00000001046e11ab in WebCore::MainResourceLoader::callContinueAfterContentPolicy(void*, WebCore::PolicyAction) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:373
> #11    0x00000001046e18c6 in WebCore::MainResourceLoader::responseReceived(WebCore::CachedResource*, WebCore::ResourceResponse const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:450
> #12    0x00000001046df594 in WebCore::MainResourceLoader::handleSubstituteDataLoadNow(WebCore::RunLoopTimer<WebCore::MainResourceLoader>*) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:628
> 
> I don't know if that's a long-standing behavior, or something that got broken with MainResourceLoader refactoring recently. Would you be willing to take a look?
> 
> r+ for this patch.

Sure, I'll take a look. My best guess is that this is a long standing issue, because cancelMainResourceLoad() should trigger a full cancel with all the relevant callbacks both before and after the recent refactorings.

[WebKit Review Bot]

Comment on attachment 189426 [details]
Fix ChangeLog

Clearing flags on attachment: 189426

Committed r143630: <http://trac.webkit.org/changeset/143630>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jim Oase]

Created attachment 190074 [details]
Shared menu is greyed out.

Decided to check on the progress of the repair efforts for the crash problem when trying to share via email. 

The menu is now greyed out.

So took a Command Key 4 screen shot.

That deep ended the display... perpetual beach ball.

Switch to another tab results in an all white window.

Resetting Webkit ... no help

Reloading a page ... no help

Still have beach ball on the Tarawa page and white pages in all other tabs.

Cure.... quit Webkit        Using build r143902

[Alexey Proskuryakov]

> The menu is now greyed out.

Can you please reboot and try to reproduce this again? If it's reproducible, please file a new bug.

[Nate Chapin]

(In reply to comment #16)
> (In reply to comment #14)
> > (From update of attachment 189426 [details] [details])
> > > The iframe in the webarchive points to a 0-byte mpeg. I'd guess that's the cause, but I'm not 100% sure.
> > 
> > This looks more like a bug than like correct behavior. This happens because we cancel main resource load when switching to a media document, and that somehow translates into a frame loader failure. But it's a resource load cancellation, not a frame load failure.
> > 
> > 
> >     // If the document is a stand-alone media document, now is the right time to cancel the WebKit load.
> >     // FIXME: This code should be shared across all ports. <http://webkit.org/b/48762>.
> >     if (m_frame->coreFrame()->document()->isMediaDocument())
> >         loader->cancelMainResourceLoad(pluginWillHandleLoadError(loader->response()));
> > 
> > 
> > #0    0x000000010128afcc in WebKit::WebFrameLoaderClient::dispatchDidFailLoading(WebCore::DocumentLoader*, unsigned long, WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:249
> > #1    0x00000001046df8af in WebCore::MainResourceLoader::receivedError(WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:99
> > #2    0x00000001046dfb27 in WebCore::MainResourceLoader::cancel(WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:132
> > #3    0x000000010399a11f in WebCore::DocumentLoader::cancelMainResourceLoad(WebCore::ResourceError const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/DocumentLoader.cpp:913
> > #4    0x000000010128db82 in WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) at /Users/ap/Safari/OpenSource/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:852
> > #5    0x0000000103997b20 in WebCore::DocumentLoader::commitLoad(char const*, int) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/DocumentLoader.cpp:318
> > #6    0x00000001039980cb in WebCore::DocumentLoader::receivedData(char const*, int) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/DocumentLoader.cpp:397
> > #7    0x00000001046e1f4a in WebCore::MainResourceLoader::dataReceived(WebCore::CachedResource*, char const*, int) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:522
> > #8    0x00000001046e0e20 in WebCore::MainResourceLoader::continueAfterContentPolicy(WebCore::PolicyAction, WebCore::ResourceResponse const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:365
> > #9    0x00000001046e1266 in WebCore::MainResourceLoader::continueAfterContentPolicy(WebCore::PolicyAction) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:381
> > #10    0x00000001046e11ab in WebCore::MainResourceLoader::callContinueAfterContentPolicy(void*, WebCore::PolicyAction) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:373
> > #11    0x00000001046e18c6 in WebCore::MainResourceLoader::responseReceived(WebCore::CachedResource*, WebCore::ResourceResponse const&) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:450
> > #12    0x00000001046df594 in WebCore::MainResourceLoader::handleSubstituteDataLoadNow(WebCore::RunLoopTimer<WebCore::MainResourceLoader>*) at /Users/ap/Safari/OpenSource/Source/WebCore/loader/MainResourceLoader.cpp:628
> > 
> > I don't know if that's a long-standing behavior, or something that got broken with MainResourceLoader refactoring recently. Would you be willing to take a look?
> > 
> > r+ for this patch.
> 
> Sure, I'll take a look. My best guess is that this is a long standing issue, because cancelMainResourceLoad() should trigger a full cancel with all the relevant callbacks both before and after the recent refactorings.

The more I think about and look at this, the less convinced I am that there's a good way to fix this. I don't think that the loader has a concept of a main resource load cancellation that isn't also a frame load failure. And if I understand how media document load works, from WebCore's perspective, this is really a failure: the frame load is getting ignored and processed externally.

Is that reasonable?

[Alexey Proskuryakov]

I'm fine with not pouring more effort into this now. Thanks for looking into it!