158224 2016-05-31 04:05:10 -0700 [Win][IndexedDB] Crash when running blob test. 2016-06-01 23:22:03 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 pvollan pvollan achristensen alecflett beidson bfulgham clopez commit-queue darin jsbell zan oldest_to_newest 1197760 0 pvollan 2016-05-31 04:05:10 -0700 The test storage/indexeddb/modern/blob-simple.html makes DumpRenderTree crash on Windows. 1197761 1 280138 pvollan 2016-05-31 04:19:07 -0700 Created attachment 280138 Patch 1197777 2 beidson 2016-05-31 06:52:46 -0700 What's the crash? Why does it crash on Windows but not any other platform? 1197780 3 pvollan 2016-05-31 07:14:39 -0700 (In reply to comment #2) > What's the crash? > > Why does it crash on Windows but not any other platform? My first guess on why it is only crashing on Windows was that other compilers might be optimizing away the WTFMove(value) call, since the parameter is not used in the lambda as far as I can tell. Or perhaps this points to a bug in MSVC? Crash details follows: EXCEPTION_RECORD: (.exr -1) .exr -1 ExceptionAddress: 00000000581ec843 (WebKit!WebCore::BlobRegistryImpl::writeBlobsToTemporaryFiles+0x0000000000000013) ExceptionCode: c0000005 (Access violation) ExceptionFlags: 00000000 NumberParameters: 2 Parameter[0]: 0000000000000000 Parameter[1]: 0000000000000014 Attempt to read from address 0000000000000014 00 0018e9c8 580dfd03 WebKit!WebCore::BlobRegistryImpl::writeBlobsToTemporaryFiles(class WTF::Vector<WTF::String,0,WTF::CrashOnOverflow,16> * blobURLs = 0x00000014, class std::function<void __cdecl(WTF::Vector<WTF::String,0,WTF::CrashOnOverflow,16> const &)> completionHandler = {...})+0x13 [c:\projects\webkit2\opensource\webkit\source\webcore\platform\network\blobregistryimpl.cpp @ 262] 01 0018ea0c 584af1c3 WebKit!WebCore::SerializedScriptValue::writeBlobsToDiskForIndexedDB(class std::function<void __cdecl(WebCore::IDBValue const &)> completionHandler = empty)+0x83 [c:\projects\webkit2\opensource\webkit\source\webcore\bindings\js\serializedscriptvalue.cpp @ 2819] 02 0018eaac 584b39e8 WebKit!WebCore::IDBTransaction::putOrAddOnServer(class WebCore::IDBClient::TransactionOperation * operation = 0x0741f950, class WTF::RefPtr<WebCore::IDBKey> key = class WTF::RefPtr<WebCore::IDBKey>, class WTF::RefPtr<WebCore::SerializedScriptValue> value = class WTF::RefPtr<WebCore::SerializedScriptValue>, WebCore::IndexedDB::ObjectStoreOverwriteMode * overwriteMode = 0x086ab748)+0x1d3 [c:\projects\webkit2\opensource\webkit\source\webcore\modules\indexeddb\idbtransaction.cpp @ 986] 03 (Inline) -------- WebKit!WebCore::IDBClient::TransactionOperationImpl<WTF::RefPtr<WebCore::IDBKey>,WTF::RefPtr<WebCore::SerializedScriptValue>,enum WebCore::IndexedDB::ObjectStoreOverwriteMode const &>::{ctor}::__l2::<lambda_a7f81b14eb3fdd35b04794f4701533e2>::operator()+0x36 [c:\projects\webkit2\opensource\webkit\source\webcore\modules\indexeddb\client\transactionoperation.h @ 139] 04 (Inline) -------- WebKit!std::_Invoker_functor::_Call+0x36 [c:\program files (x86)\microsoft visual studio 14.0\vc\include\type_traits @ 1398] 05 (Inline) -------- WebKit!std::invoke+0x36 [c:\program files (x86)\microsoft visual studio 14.0\vc\include\type_traits @ 1466] 06 (Inline) -------- WebKit!std::_Invoke_ret+0x36 [c:\program files (x86)\microsoft visual studio 14.0\vc\include\type_traits @ 1484] 07 0018eac0 584ae8cb WebKit!std::_Func_impl<<lambda_a7f81b14eb3fdd35b04794f4701533e2>,std::allocator<int>,void>::_Do_call(void)+0x38 [c:\program files (x86)\microsoft visual studio 14.0\vc\include\functional @ 214] 08 (Inline) -------- WebKit!std::_Func_class<void>::operator()+0xf [c:\program files (x86)\microsoft visual studio 14.0\vc\include\functional @ 279] 09 (Inline) -------- WebKit!WebCore::IDBClient::TransactionOperation::perform+0x12 [c:\projects\webkit2\opensource\webkit\source\webcore\modules\indexeddb\client\transactionoperation.h @ 59] 0a 0018ead4 5820ac05 WebKit!WebCore::IDBTransaction::operationTimerFired(void)+0x3b [c:\projects\webkit2\opensource\webkit\source\webcore\modules\indexeddb\idbtransaction.cpp @ 407] 0b 0018eafc 585266b3 WebKit!WebCore::ThreadTimers::sharedTimerFiredInternal(void)+0x95 [c:\projects\webkit2\opensource\webkit\source\webcore\platform\threadtimers.cpp @ 124] 0c 0018eb04 75e784f3 WebKit!WebCore::TimerWindowWndProc(struct HWND__ * hWnd = 0x00d20776, unsigned int message = 0xc41e, unsigned int wParam = 0, long lParam = 0n0)+0x83 [c:\projects\webkit2\opensource\webkit\source\webcore\platform\win\mainthreadsharedtimerwin.cpp @ 91] 0d 0018eb30 75e56c40 USER32!_InternalCallWinProc+0x2b 0e 0018ebd8 75e56541 USER32!UserCallWinProcCheckWow+0x1f0 0f 0018ec44 75e56300 USER32!DispatchMessageWorker+0x231 10 0018ec50 58b2782e USER32!DispatchMessageW+0x10 11 0018ed3c 58b23e9a DumpRenderTreeLib!runTest(class std::basic_string<char,std::char_traits<char>,std::allocator<char> > * inputLine = 0x0018ed7c "C:\Projects\WebKit2\OpenSource\WebKit\LayoutTests\storage\indexeddb\modern\blob-simple.html")+0x53e [c:\projects\webkit2\opensource\webkit\tools\dumprendertree\win\dumprendertree.cpp @ 1133] 12 0018f598 58b23fee DumpRenderTreeLib!main(int argc = 0n3, char ** argv = 0x006d2490)+0x2da [c:\projects\webkit2\opensource\webkit\tools\dumprendertree\win\dumprendertree.cpp @ 1481] 13 0018f5a8 00a816c9 DumpRenderTreeLib!dllLauncherEntryPoint(int argc = 0n3, char ** argv = 0x006d2490)+0xe [c:\projects\webkit2\opensource\webkit\tools\dumprendertree\win\dumprendertree.cpp @ 1516] 14 0018f870 00a832ba DumpRenderTree!main(int argc = 0n3, char ** argv = 0x006d2490)+0x469 [c:\projects\webkit2\opensource\webkit\tools\win\dlllauncher\dlllaunchermain.cpp @ 247] 15 (Inline) -------- DumpRenderTree!invoke_main+0x1d [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 64] 16 0018f8bc 76f638f4 DumpRenderTree!__scrt_common_main_seh(void)+0xff [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 255] 17 0018f8d0 77095de3 KERNEL32!BaseThreadInitThunk+0x24 18 0018f918 77095dae ntdll_77030000!__RtlUserThreadStart+0x2f 19 0018f928 00000000 ntdll_77030000!_RtlUserThreadStart+0x1b 1197805 4 280138 achristensen 2016-05-31 09:50:59 -0700 Comment on attachment 280138 Patch The order of these operations probably isn't defined. Brady would have to confirm that value isn't needed in the lambda. It's probably needed to keep value alive during the life of the lambda. Could we save the SerializedScriptValue* to use for the function call before possibly WTFMoving it away? 1197821 5 darin 2016-05-31 10:17:53 -0700 (In reply to comment #4) > It's probably needed to keep value alive during the life of the lambda. I am pretty sure that is not true, but if it was true, then writeBlobsToTemporaryFiles should take care of it, not the lambda itself. 1197822 6 280138 beidson 2016-05-31 10:19:31 -0700 Comment on attachment 280138 Patch No r+! This is actually wrong. Alex will come explain in a followup. 1197837 7 beidson 2016-05-31 11:16:18 -0700 (In reply to comment #6) > Comment on attachment 280138 [details] > Patch > > No r+! This is actually wrong. > > Alex will come explain in a followup. He did not explain. So I just got back from my appointment and was going to explain. But then looked closer and saw darin was right - Inside writeBlobsToDiskForIndexedDB, the SSV keeps itself alive. The captured value is, in fact, unnecessary. 1197838 8 280138 beidson 2016-05-31 11:16:32 -0700 Comment on attachment 280138 Patch Consider this restoring Darin's r+ 1197850 9 pvollan 2016-05-31 11:27:48 -0700 Thanks for reviewing, guys! 1197860 10 pvollan 2016-05-31 11:43:03 -0700 By the way, it seems that the lambda is meant to be executing on the main thread. Does the SerializedScriptValue need to kept alive until the lambda has finished executing on the main thread? 1197872 11 beidson 2016-05-31 12:13:59 -0700 (In reply to comment #10) > By the way, it seems that the lambda is meant to be executing on the main > thread. Does the SerializedScriptValue need to kept alive until the lambda > has finished executing on the main thread? It is. See SerializedScriptValue::writeBlobsToDiskForIndexedDB. 1197910 12 pvollan 2016-05-31 13:33:12 -0700 (In reply to comment #11) > (In reply to comment #10) > > By the way, it seems that the lambda is meant to be executing on the main > > thread. Does the SerializedScriptValue need to kept alive until the lambda > > has finished executing on the main thread? > > It is. > > See SerializedScriptValue::writeBlobsToDiskForIndexedDB. Ok, thanks :) I can see now that this is done by 'protectedThis = WTFMove(protectedThis)' in the code below. void SerializedScriptValue::writeBlobsToDiskForIndexedDB(std::function<void (const IDBValue&)> completionHandler) { ASSERT(isMainThread()); ASSERT(hasBlobURLs()); RefPtr<SerializedScriptValue> protectedThis(this); blobRegistry().writeBlobsToTemporaryFiles(m_blobURLs, [completionHandler = WTFMove(completionHandler), this, protectedThis = WTFMove(protectedThis)](auto& blobFilePaths) { 1198106 13 pvollan 2016-06-01 02:12:53 -0700 Committed r201547: <https://trac.webkit.org/changeset/201547> 1198144 14 clopez 2016-06-01 07:38:43 -0700 (In reply to comment #2) > What's the crash? > > Why does it crash on Windows but not any other platform? For the record: We have found that WebKitGTK+ was crashing when loading http://html5test.com when we built WebKit with GCC but not with Clang. We identified that the crash started to happen with http://trac.webkit.org/changeset/201302 Zan had a fix for it http://sprunge.us/DQMf but then we found that r201547 already fixed the crash. So this crash was triggered both with GCC and MSVC. Only with Clang it was fine. Not sure what this means 1198168 15 beidson 2016-06-01 09:18:37 -0700 (In reply to comment #14) > (In reply to comment #2) > > What's the crash? > > > > Why does it crash on Windows but not any other platform? > > For the record: > > We have found that WebKitGTK+ was crashing when loading http://html5test.com > when we built WebKit with GCC but not with Clang. > > We identified that the crash started to happen with > http://trac.webkit.org/changeset/201302 > > Zan had a fix for it http://sprunge.us/DQMf but then we found that r201547 > already fixed the crash. > > So this crash was triggered both with GCC and MSVC. Only with Clang it was > fine. > > Not sure what this means Since the code was wrong from a sanity standpoint, not sure it's worth digging much deeper. I'm willing to accept "it means the behavior being triggered was probably unspecified, and Clang was lenient" 1198467 16 zan 2016-06-01 23:22:03 -0700 This happens due to different calling convention being used on GCC and apparently MSVC as well -- there the lambda argument is constructed first, moving from the RefPtr which is later dereferenced to provide `this` for the call to SerializedScriptValue::writeBlobsToDiskForIndexedDB(), which is null after the move is done. Clang uses a calling convention that does this in the reverse order, caching the value for `this` first and only afterwards constructing the lambda and performing the move. 280138 2016-05-31 04:19:07 -0700 2016-05-31 11:16:32 -0700 Patch bug-158224-20160531042000.patch text/plain 1716 pvollan SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwMTUwOSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE2LTA1LTMxICBQZXIgQXJu ZSBWb2xsYW4gIDxwdm9sbGFuQGFwcGxlLmNvbT4KKworICAgICAgICBbV2luXVtJbmRleGVkREJd IENyYXNoIHdoZW4gcnVubmluZyBibG9iIHRlc3QuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTgyMjQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBBdm9pZCBjYWxsaW5nIFdURk1vdmUoeCkgYmVmb3JlIGNh bGxpbmcgeC0+bWV0aG9kKCkuCisKKyAgICAgICAgKiBNb2R1bGVzL2luZGV4ZWRkYi9JREJUcmFu c2FjdGlvbi5jcHA6CisgICAgICAgIChXZWJDb3JlOjpJREJUcmFuc2FjdGlvbjo6cHV0T3JBZGRP blNlcnZlcik6CisKIDIwMTYtMDUtMzAgIEJyYWR5IEVpZHNvbiAgPGJlaWRzb25AYXBwbGUuY29t PgogCiAgICAgICAgIE1vdmUgQ3Jvc3NUaHJlYWRDb3BpZXIvQ3Jvc3NUaHJlYWRUYXNrIHRvIFdU Ri4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL01vZHVsZXMvaW5kZXhlZGRiL0lEQlRyYW5zYWN0aW9u LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL2luZGV4ZWRkYi9JREJU cmFuc2FjdGlvbi5jcHAJKHJldmlzaW9uIDIwMTUwNSkKKysrIFNvdXJjZS9XZWJDb3JlL01vZHVs ZXMvaW5kZXhlZGRiL0lEQlRyYW5zYWN0aW9uLmNwcAkod29ya2luZyBjb3B5KQpAQCAtOTY4LDcg Kzk2OCw3IEBAIHZvaWQgSURCVHJhbnNhY3Rpb246OnB1dE9yQWRkT25TZXJ2ZXIoSUQKIAogICAg IFJlZlB0cjxJREJUcmFuc2FjdGlvbj4gcHJvdGVjdGVkVGhpcyh0aGlzKTsKICAgICBSZWZQdHI8 SURCQ2xpZW50OjpUcmFuc2FjdGlvbk9wZXJhdGlvbj4gcHJvdGVjdGVkT3BlcmF0aW9uKCZvcGVy YXRpb24pOwotICAgIHZhbHVlLT53cml0ZUJsb2JzVG9EaXNrRm9ySW5kZXhlZERCKFtwcm90ZWN0 ZWRUaGlzID0gV1RGTW92ZShwcm90ZWN0ZWRUaGlzKSwgdGhpcywgcHJvdGVjdGVkT3BlcmF0aW9u ID0gV1RGTW92ZShwcm90ZWN0ZWRPcGVyYXRpb24pLCBrZXkgPSBXVEZNb3ZlKGtleSksIHZhbHVl ID0gV1RGTW92ZSh2YWx1ZSksIG92ZXJ3cml0ZU1vZGVdKGNvbnN0IElEQlZhbHVlJiBpZGJWYWx1 ZSkgbXV0YWJsZSB7CisgICAgdmFsdWUtPndyaXRlQmxvYnNUb0Rpc2tGb3JJbmRleGVkREIoW3By b3RlY3RlZFRoaXMgPSBXVEZNb3ZlKHByb3RlY3RlZFRoaXMpLCB0aGlzLCBwcm90ZWN0ZWRPcGVy YXRpb24gPSBXVEZNb3ZlKHByb3RlY3RlZE9wZXJhdGlvbiksIGtleSA9IFdURk1vdmUoa2V5KSwg b3ZlcndyaXRlTW9kZV0oY29uc3QgSURCVmFsdWUmIGlkYlZhbHVlKSBtdXRhYmxlIHsKICAgICAg ICAgQVNTRVJUKGN1cnJlbnRUaHJlYWQoKSA9PSBvcmlnaW5UaHJlYWRJRCgpKTsKICAgICAgICAg QVNTRVJUKGlzTWFpblRocmVhZCgpKTsKICAgICAgICAgaWYgKGlkYlZhbHVlLmRhdGEoKS5kYXRh KCkpIHsK