48531 2010-10-28 09:56:08 -0700 Assertion failure in DocumentLoader::commitData when loading a media document in WebKit1 on Windows 2010-11-01 09:51:54 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) PC Windows XP RESOLVED FIXED http://www.nch.com.au/acm/sample.wav InRadar, PlatformOnly P2 Normal --- 1 aroben aroben abarth eric.carlson eric jer.noble oldest_to_newest 301125 0 aroben 2010-10-28 09:56:08 -0700 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() 301131 1 aroben 2010-10-28 10:02:25 -0700 Looks like we need to do something similar to r51104: http://trac.webkit.org/changeset/51104 301312 2 aroben 2010-10-28 13:46:53 -0700 <rdar://problem/8606635> 302712 3 72511 aroben 2010-11-01 08:43:52 -0700 Created attachment 72511 Cancel main resource loads after we hand them off to the media engine 302737 4 72511 mitz 2010-11-01 09:16:26 -0700 Comment on attachment 72511 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”. 302742 5 eric.carlson 2010-11-01 09:25:05 -0700 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? 302746 6 aroben 2010-11-01 09:35:33 -0700 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. 302751 7 aroben 2010-11-01 09:51:54 -0700 Committed r71032: <http://trac.webkit.org/changeset/71032> 72511 2010-11-01 08:43:52 -0700 2010-11-01 09:16:26 -0700 Cancel main resource loads after we hand them off to the media engine bug-48531-20101101114354.patch text/plain 3425 aroben ZGlmZiAtLWdpdCBhL1dlYktpdC93aW4vQ2hhbmdlTG9nIGIvV2ViS2l0L3dpbi9DaGFuZ2VMb2cK aW5kZXggY2U1OTQxOTMxNzA0NTdlNTZjNjhiMzVlNjQyNjE2MDc2NzMxNGI3OS4uNWYxYThkMDBi MDEzZjM0NmUwMDA2YTc0M2I2MzhiNWVkOTk3N2UzNiAxMDA2NDQKLS0tIGEvV2ViS2l0L3dpbi9D aGFuZ2VMb2cKKysrIGIvV2ViS2l0L3dpbi9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyNiBAQAorMjAx MC0xMS0wMSAgQWRhbSBSb2JlbiAgPGFyb2JlbkBhcHBsZS5jb20+CisKKyAgICAgICAgQ2FuY2Vs IG1haW4gcmVzb3VyY2UgbG9hZHMgYWZ0ZXIgd2UgaGFuZCB0aGVtIG9mZiB0byB0aGUgbWVkaWEg ZW5naW5lCisKKyAgICAgICAgVGhpcyBpcyB0aGUgV2luZG93cyBlcXVpdmFsZW50IG9mIHI1MTEw NC4gQ2xlYXJseSB0aGlzIGNvZGUgc2hvdWxkIGJlCisgICAgICAgIG1vdmVkIHRvIGEgY3Jvc3Mt cGxhdGZvcm0gbG9jYXRpb24gc29tZWRheS4KKworICAgICAgICBGaXhlcyA8aHR0cDovL3dlYmtp dC5vcmcvYi80ODUzMT4gPHJkYXI6Ly9wcm9ibGVtLzg2MDY2MzU+IEFzc2VydGlvbgorICAgICAg ICBmYWlsdXJlIGluIERvY3VtZW50TG9hZGVyOjpjb21taXREYXRhIHdoZW4gbG9hZGluZyBhIG1l ZGlhIGRvY3VtZW50IGluCisgICAgICAgIFdlYktpdDEgb24gV2luZG93cworCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogV2ViQ29yZVN1cHBvcnQvV2Vi RnJhbWVMb2FkZXJDbGllbnQuY3BwOgorICAgICAgICAoV2ViRnJhbWVMb2FkZXJDbGllbnQ6OmNv bW1pdHRlZExvYWQpOiBDYW5jZWwgdGhlIG1haW4gcmVzb3VyY2UgbG9hZAorICAgICAgICBhZnRl ciBoYW5kaW5nIG9mZiB0aGUgbG9hZCB0byB0aGUgbWVkaWEgZW5naW5lLiBUaGlzIGNvZGUgb3Jp Z2luYWxsCisgICAgICAgIGNhbWUgZnJvbSAtW1dlYkhUTUxSZXByZXNlbnRhdGlvbiByZWNlaXZl ZERhdGE6d2l0aERhdGFTb3VyY2U6XS4KKworICAgICAgICAqIFdlYkZyYW1lLmNwcDoKKyAgICAg ICAgKFdlYkZyYW1lOjpzaG91bGRGYWxsQmFjayk6IERvbid0IGZhbGwgYmFjayB3aGVuIGhhbmRp bmcgdGhlIHJlc291cmNlCisgICAgICAgIGxvYWQgb2ZmIHRvIHRoZSBtZWRpYSBlbmdpbmUgb3Ig YSBwbHVnaW4uIEFkZGVkIGVycm9yIGRvbWFpbiBjaGVja2luZworICAgICAgICBzbyB0aGF0IHdl IGRvbid0IHJlbHkgb24gZXJyb3IgY29kZXMgYmVpbmcgdW5pcXVlLgorCiAyMDEwLTEwLTI5ICBE YW5pZWwgQmF0ZXMgIDxkYmF0ZXNAcmltLmNvbT4KIAogICAgICAgICBObyByZXZpZXcsIHJvbGxp bmcgb3V0IDcwOTcxLgpkaWZmIC0tZ2l0IGEvV2ViS2l0L3dpbi9XZWJDb3JlU3VwcG9ydC9XZWJG cmFtZUxvYWRlckNsaWVudC5jcHAgYi9XZWJLaXQvd2luL1dlYkNvcmVTdXBwb3J0L1dlYkZyYW1l TG9hZGVyQ2xpZW50LmNwcAppbmRleCA2NTkxMzQ3ZDA3NDdmMDFjYTdlZjg1ZGJhZWIzZjMyZWQ4 ZDE0OTQyLi43ODBmOWFkOGFlYjE4YzNmY2U1OGI4NDEzOTE3OTIzODMzMDEzY2EwIDEwMDY0NAot LS0gYS9XZWJLaXQvd2luL1dlYkNvcmVTdXBwb3J0L1dlYkZyYW1lTG9hZGVyQ2xpZW50LmNwcAor KysgYi9XZWJLaXQvd2luL1dlYkNvcmVTdXBwb3J0L1dlYkZyYW1lTG9hZGVyQ2xpZW50LmNwcApA QCAtNDk0LDExICs0OTQsMTYgQEAgdm9pZCBXZWJGcmFtZUxvYWRlckNsaWVudDo6Y29tbWl0dGVk TG9hZChEb2N1bWVudExvYWRlciogbG9hZGVyLCBjb25zdCBjaGFyKiBkYXQKICAgICBpZiAoIW1f bWFudWFsTG9hZGVyKQogICAgICAgICBsb2FkZXItPmNvbW1pdERhdGEoZGF0YSwgbGVuZ3RoKTsK IAorICAgIC8vIElmIHRoZSBkb2N1bWVudCBpcyBhIHN0YW5kLWFsb25lIG1lZGlhIGRvY3VtZW50 LCBub3cgaXMgdGhlIHJpZ2h0IHRpbWUgdG8gY2FuY2VsIHRoZSBXZWJLaXQgbG9hZC4KKyAgICBG cmFtZSogY29yZUZyYW1lID0gY29yZShtX3dlYkZyYW1lKTsKKyAgICBpZiAoY29yZUZyYW1lLT5k b2N1bWVudCgpLT5pc01lZGlhRG9jdW1lbnQoKSkKKyAgICAgICAgbG9hZGVyLT5jYW5jZWxNYWlu UmVzb3VyY2VMb2FkKHBsdWdpbldpbGxIYW5kbGVMb2FkRXJyb3IobG9hZGVyLT5yZXNwb25zZSgp KSk7CisKICAgICBpZiAoIW1fbWFudWFsTG9hZGVyKQogICAgICAgICByZXR1cm47CiAKICAgICBp ZiAoIW1faGFzU2VudFJlc3BvbnNlVG9QbHVnaW4pIHsKLSAgICAgICAgbV9tYW51YWxMb2FkZXIt PmRpZFJlY2VpdmVSZXNwb25zZShjb3JlKG1fd2ViRnJhbWUpLT5sb2FkZXIoKS0+ZG9jdW1lbnRM b2FkZXIoKS0+cmVzcG9uc2UoKSk7CisgICAgICAgIG1fbWFudWFsTG9hZGVyLT5kaWRSZWNlaXZl UmVzcG9uc2UobG9hZGVyLT5yZXNwb25zZSgpKTsKICAgICAgICAgLy8gZGlkUmVjZWl2ZVJlc3Bv bnNlIHNldHMgdXAgYSBuZXcgc3RyZWFtIHRvIHRoZSBwbHVnLWluLiBvbiBhIGZ1bGwtcGFnZSBw bHVnLWluLCBhIGZhaWx1cmUgaW4KICAgICAgICAgLy8gc2V0dGluZyB1cCB0aGlzIHN0cmVhbSBj YW4gY2F1c2UgdGhlIG1haW4gZG9jdW1lbnQgbG9hZCB0byBiZSBjYW5jZWxsZWQsIHNldHRpbmcg bV9tYW51YWxMb2FkZXIKICAgICAgICAgLy8gdG8gbnVsbApkaWZmIC0tZ2l0IGEvV2ViS2l0L3dp bi9XZWJGcmFtZS5jcHAgYi9XZWJLaXQvd2luL1dlYkZyYW1lLmNwcAppbmRleCA5ZGI2YTJhMTI1 Y2JjOGY2MzM3NzNiY2JhOGY0NWU3OWNkM2RkYWI0Li5jMWQ5YzIxMzdlZTBhYjgxMjNmNDM2NDk3 MWNkMDgzM2U5NzZjMDYxIDEwMDY0NAotLS0gYS9XZWJLaXQvd2luL1dlYkZyYW1lLmNwcAorKysg Yi9XZWJLaXQvd2luL1dlYkZyYW1lLmNwcApAQCAtMTcxOCw3ICsxNzE4LDEzIEBAIFJlc291cmNl RXJyb3IgV2ViRnJhbWU6OnBsdWdpbldpbGxIYW5kbGVMb2FkRXJyb3IoY29uc3QgUmVzb3VyY2VS ZXNwb25zZSYgcmVzcG9uCiAKIGJvb2wgV2ViRnJhbWU6OnNob3VsZEZhbGxCYWNrKGNvbnN0IFJl c291cmNlRXJyb3ImIGVycm9yKQogewotICAgIHJldHVybiBlcnJvci5lcnJvckNvZGUoKSAhPSBX ZWJVUkxFcnJvckNhbmNlbGxlZDsKKyAgICBpZiAoZXJyb3IuZXJyb3JDb2RlKCkgPT0gV2ViVVJM RXJyb3JDYW5jZWxsZWQgJiYgZXJyb3IuZG9tYWluKCkgPT0gU3RyaW5nKFdlYlVSTEVycm9yRG9t YWluKSkKKyAgICAgICAgcmV0dXJuIGZhbHNlOworCisgICAgaWYgKGVycm9yLmVycm9yQ29kZSgp ID09IFdlYktpdEVycm9yUGx1Z0luV2lsbEhhbmRsZUxvYWQgJiYgZXJyb3IuZG9tYWluKCkgPT0g U3RyaW5nKFdlYktpdEVycm9yRG9tYWluKSkKKyAgICAgICAgcmV0dXJuIGZhbHNlOworCisgICAg cmV0dXJuIHRydWU7CiB9CiAKIENPTVB0cjxXZWJGcmFtZVBvbGljeUxpc3RlbmVyPiBXZWJGcmFt ZTo6c2V0VXBQb2xpY3lMaXN0ZW5lcihXZWJDb3JlOjpGcmFtZVBvbGljeUZ1bmN0aW9uIGZ1bmN0 aW9uKQo=