Bug 48531 – Assertion failure in DocumentLoader::commitData when loading a media document in WebKit1 on Windows

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

[Adam Roben (:aroben)]

To reproduce:

1. Go to http://www.nch.com.au/acm/sample.wav in a debug build

You'll hit an assertion in DocumentLoader::commitData:

    ASSERT(m_frame->document()->parsing());

The wav file seems to load fine in Release builds.

Here's the backtrace:


>	WebKit.dll!WebCore::DocumentLoader::commitData(const char * bytes=0x0a9fa008, int length=69300)  Line 306 + 0x35 bytes	C++
 	WebKit.dll!WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader * loader=0x0a888068, const char * data=0x0a9fa008, int length=69300)  Line 497	C++
 	WebKit.dll!WebCore::DocumentLoader::commitLoad(const char * data=0x0a9fa008, int length=69300)  Line 292 + 0x29 bytes	C++
 	WebKit.dll!WebCore::DocumentLoader::receivedData(const char * data=0x0a9fa008, int length=69300)  Line 320	C++
 	WebKit.dll!WebCore::MainResourceLoader::addData(const char * data=0x0a9fa008, int length=69300, bool allAtOnce=false)  Line 157	C++
 	WebKit.dll!WebCore::ResourceLoader::didReceiveData(const char * data=0x0a9fa008, int length=69300, __int64 lengthReceived=69300, bool allAtOnce=false)  Line 262 + 0x1b bytes	C++
 	WebKit.dll!WebCore::MainResourceLoader::didReceiveData(const char * data=0x0a9fa008, int length=69300, __int64 lengthReceived=69300, bool allAtOnce=false)  Line 437	C++
 	WebKit.dll!WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle * __formal=0x09825830, const char * data=0x0a9fa008, int length=69300, int lengthReceived=69300)  Line 415 + 0x1f bytes	C++
 	WebKit.dll!WebCore::didReceiveData(_CFURLConnection * conn=0x097d05a0, const __CFData * data=0x097cf750, long originalLength=69300, const void * clientInfo=0x09825830)  Line 214 + 0x2a bytes	C++
 	CFNetwork.dll!URLConnectionClient::_clientDidReceiveData()

[Adam Roben (:aroben)]

Looks like we need to do something similar to r51104: http://trac.webkit.org/changeset/51104

[Adam Roben (:aroben)]

<rdar://problem/8606635>

[Adam Roben (:aroben)]

Created attachment 72511 [details]
Cancel main resource loads after we hand them off to the media engine

Comment on attachment 72511 [details]
Cancel main resource loads after we hand them off to the media engine

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

> WebKit/win/ChangeLog:16
> +        after handing off the load to the media engine. This code originall

Missing “y”.

[Eric Carlson]

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

Dan beat me to the review, but here are a few more comments:

> WebKit/win/ChangeLog:6
> +        This is the Windows equivalent of r51104. Clearly this code should be
> +        moved to a cross-platform location someday.

Probably worth a bug if we don't already have one.


> WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:500
> +    if (coreFrame->document()->isMediaDocument())
> +        loader->cancelMainResourceLoad(pluginWillHandleLoadError(loader->response()));

I know that we use the same error in other ports, but canceling the download with "pluginWillHandleLoadError" when we are actually handing it off the the media element seems wrong.


> WebKit/win/WebFrame.cpp:1726
> +    if (error.errorCode() == WebURLErrorCancelled && error.domain() == String(WebURLErrorDomain))
> +        return false;
> +
> +    if (error.errorCode() == WebKitErrorPlugInWillHandleLoad && error.domain() == String(WebKitErrorDomain))
> +        return false;
> +

Looking at http://trac.webkit.org/changeset/51104 I see that GTK also returns false if the error is interruptForPolicyChangeError, should we do that as well?

[Adam Roben (:aroben)]

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

> > WebKit/win/ChangeLog:6
> > +        This is the Windows equivalent of r51104. Clearly this code should be
> > +        moved to a cross-platform location someday.
> 
> Probably worth a bug if we don't already have one.

Filed bug 48762.

> > WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:500
> > +    if (coreFrame->document()->isMediaDocument())
> > +        loader->cancelMainResourceLoad(pluginWillHandleLoadError(loader->response()));
> 
> I know that we use the same error in other ports, but canceling the download with "pluginWillHandleLoadError" when we are actually handing it off the the media element seems wrong.

Agreed. File bug 48763.

> > WebKit/win/WebFrame.cpp:1726
> > +    if (error.errorCode() == WebURLErrorCancelled && error.domain() == String(WebURLErrorDomain))
> > +        return false;
> > +
> > +    if (error.errorCode() == WebKitErrorPlugInWillHandleLoad && error.domain() == String(WebKitErrorDomain))
> > +        return false;
> > +
> 
> Looking at http://trac.webkit.org/changeset/51104 I see that GTK also returns false if the error is interruptForPolicyChangeError, should we do that as well?

I was trying to match Mac here, so I think I'll keep this as-is for now.

[Adam Roben (:aroben)]

Committed r71032: <http://trac.webkit.org/changeset/71032>