Bug 158224 – [Win][IndexedDB] Crash when running blob test.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

The test storage/indexeddb/modern/blob-simple.html makes DumpRenderTree crash on Windows.

[Per Arne Vollan]

Created attachment 280138 [details]
Patch

[Brady Eidson]

What's the crash?

Why does it crash on Windows but not any other platform?

[Per Arne Vollan]

(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

[Alex Christensen]

Comment on attachment 280138 [details]
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?

[Darin Adler]

(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.

[Brady Eidson]

Comment on attachment 280138 [details]
Patch

No r+!  This is actually wrong.

Alex will come explain in a followup.

[Brady Eidson]

(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.

[Brady Eidson]

Comment on attachment 280138 [details]
Patch

Consider this restoring Darin's r+

[Per Arne Vollan]

Thanks for reviewing, guys!

[Per Arne Vollan]

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?

[Brady Eidson]

(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.

[Per Arne Vollan]

(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) {

[Per Arne Vollan]

Committed r201547: <https://trac.webkit.org/changeset/201547>

[Carlos Alberto Lopez Perez]

(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

[Brady Eidson]

(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"

[Zan Dobersek]

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.