Bug 121972

Summary: testapi test crashes on Windows in WTF::Vector<wchar_t,64,WTF::UnsafeVectorOverflow>::size()
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bfulgham, cmarcelo, commit-queue, fpizlo, ggaren, mark.lam, mhahnenberg, msaboff, oliver, peavo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 123585    
Attachments:
Description Flags
Patch
none
Fixing OSAllocatorWin instead. bfulgham: review+

Description Mark Lam 2013-09-26 13:31:33 PDT
On the Windows build, the latest JSC (since before r156490) crashes when running testapi.

The crash stack trace looks like this:

	JavaScriptCore.dll!WTF::Vector<wchar_t,64,WTF::UnsafeVectorOverflow>::size()  Line 569 + 0x11 bytes	C++
 	JavaScriptCore.dll!JSC::MarkedAllocator::forEachBlock<JSC::Free>(JSC::Free & functor)  Line 141 + 0x8 bytes	C++
 	JavaScriptCore.dll!JSC::MarkedSpace::forEachBlock<JSC::Free>(JSC::Free & functor)  Line 230	C++
 	JavaScriptCore.dll!JSC::MarkedSpace::~MarkedSpace()  Line 106	C++
 	JavaScriptCore.dll!JSC::Heap::~Heap()  Line 282 + 0xee bytes	C++
 	JavaScriptCore.dll!JSC::VM::~VM()  Line 356 + 0x399 bytes	C++
 	JavaScriptCore.dll!JSC::VM::`scalar deleting destructor'()  + 0x16 bytes	C++
 	JavaScriptCore.dll!WTF::ThreadSafeRefCounted<JSC::VM>::deref()  Line 137 + 0x1c bytes	C++
 	JavaScriptCore.dll!WTF::derefIfNotNull<JSC::VM>(JSC::VM * ptr)  Line 45	C++
 	JavaScriptCore.dll!WTF::RefPtr<JSC::VM>::clear()  Line 102 + 0x9 bytes	C++
 	JavaScriptCore.dll!JSC::JSLockHolder::~JSLockHolder()  Line 84	C++
 	JavaScriptCore.dll!JSGlobalContextRelease(OpaqueJSContext * ctx)  Line 179	C++
 	testapi.exe!main(int argc, char * * argv)  Line 1176 + 0xf bytes	C++
 	testapi.exe!__tmainCRTStartup()  Line 555 + 0x17 bytes	C
 	kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes	
 	ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes	
 	ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes	

This crash is reproducible every time we run testapi.exe.
Comment 1 Mark Hahnenberg 2013-09-26 13:36:33 PDT
That backtrace makes no sense. We don't access a Vector in MarkedAllocator::forEachBlock. Is this a debug backtrace?
Comment 2 Mark Lam 2013-09-26 13:41:43 PDT
(In reply to comment #1)
> That backtrace makes no sense. We don't access a Vector in MarkedAllocator::forEachBlock. Is this a debug backtrace?

This was run on a debug build.  When it crashed, it gave me the opportunity to attach Visual Studio, and that's how I grabbed the stack trace.
Comment 3 Mark Hahnenberg 2013-09-26 13:47:19 PDT
Hmm, the only object I can find that uses a Vector<wchar_t,64,WTF::UnsafeVectorOverflow> is the JSStringBuilder. So maybe there's something weird going on there. Can you attach a full stack trace (e.g. with error code)?
Comment 4 Mark Lam 2013-09-26 13:49:51 PDT
(In reply to comment #3)
> Hmm, the only object I can find that uses a Vector<wchar_t,64,WTF::UnsafeVectorOverflow> is the JSStringBuilder. So maybe there's something weird going on there. Can you attach a full stack trace (e.g. with error code)?

That is the full stack trace.
Comment 5 Mark Hahnenberg 2013-09-26 13:50:47 PDT
> That is the full stack trace.

There's no crash log anywhere? Not even something to tell you what happened (e.g. segfault, etc.)?
Comment 6 Radar WebKit Bug Importer 2013-10-11 14:28:16 PDT
<rdar://problem/15211539>
Comment 7 Geoffrey Garen 2013-10-14 11:31:11 PDT
Seems like the JSC::Free must have called a bad function pointer.

My guess is that the top stack frame is just bogus -- a tools bug that happens when there's a PC in the backtrace that doesn't correspond to real code with debug info.
Comment 8 peavo 2013-12-03 07:44:06 PST
Created attachment 218294 [details]
Patch
Comment 9 peavo 2013-12-03 07:45:39 PST
The reason for the crash is that the wrong memory block is decommitted.
This can happen if no memory has been committed in the reserved block before the JSStack object is destroyed.
In the JSStack destructor, the pointer to decommit then points to the end of the block (or the start of the next), and the decommit size is zero.
If there is a block just after the block we are trying to decommit, this block will be decommitted, since Windows will decommit the whole block,
if the decommit size is zero (see VirtualFree). When somebody tries to read/write to this block later, we crash.
Comment 10 WebKit Commit Bot 2013-12-03 09:07:05 PST
Comment on attachment 218294 [details]
Patch

Clearing flags on attachment: 218294

Committed r160004: <http://trac.webkit.org/changeset/160004>
Comment 11 WebKit Commit Bot 2013-12-03 09:07:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Brent Fulgham 2013-12-03 09:07:34 PST
(In reply to comment #9)
> The reason for the crash is that the wrong memory block is decommitted.
> This can happen if no memory has been committed in the reserved block before the JSStack object is destroyed.
> In the JSStack destructor, the pointer to decommit then points to the end of the block (or the start of the next), and the decommit size is zero.
> If there is a block just after the block we are trying to decommit, this block will be decommitted, since Windows will decommit the whole block,
> if the decommit size is zero (see VirtualFree). When somebody tries to read/write to this block later, we crash.

Nice work!
Comment 13 Mark Lam 2013-12-03 13:43:34 PST
(In reply to comment #9)
> The reason for the crash is that the wrong memory block is decommitted.
> This can happen if no memory has been committed in the reserved block before the JSStack object is destroyed.
> In the JSStack destructor, the pointer to decommit then points to the end of the block (or the start of the next), and the decommit size is zero.
> If there is a block just after the block we are trying to decommit, this block will be decommitted, since Windows will decommit the whole block,
> if the decommit size is zero (see VirtualFree). When somebody tries to read/write to this block later, we crash.

Nice catch.  However, I think the better fix would be to make OSAllocator::decommit() handle a 0 size as one would expect i.e. do nothing.  The behavior of Window's VirtualFree() in terms of its interpretation of what a 0 size means is not intuitive.  I'll look into fixing this in  separate patch + bug.  Fixing the interpretation of size 0 in OSAllocator will also help avoid future surprises like this from manifesting.
Comment 14 Mark Lam 2013-12-03 17:38:56 PST
Reopening this bug to apply the better fix in OSAllocatorWin.
Comment 15 Mark Lam 2013-12-03 17:40:14 PST
Created attachment 218367 [details]
Fixing OSAllocatorWin instead.
Comment 16 Brent Fulgham 2013-12-03 17:41:09 PST
Comment on attachment 218367 [details]
Fixing OSAllocatorWin instead.

r=me
Comment 17 Mark Hahnenberg 2013-12-03 17:52:36 PST
Comment on attachment 218367 [details]
Fixing OSAllocatorWin instead.

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

> Source/WTF/wtf/OSAllocatorWin.cpp:69
> +        return; // Nothing to do.

This comment seems unnecessary.

> Source/WTF/wtf/OSAllocatorWin.cpp:78
> +        return; // Nothing to do.

Ditto.
Comment 18 Mark Hahnenberg 2013-12-03 17:53:32 PST
Comment on attachment 218367 [details]
Fixing OSAllocatorWin instead.

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

>> Source/WTF/wtf/OSAllocatorWin.cpp:69
>> +        return; // Nothing to do.
> 
> This comment seems unnecessary.

Instead, I would comment on how Windows does odd things when the size is 0 and refer them to this bug.
Comment 19 Mark Lam 2013-12-03 18:01:50 PST
Comment on attachment 218367 [details]
Fixing OSAllocatorWin instead.

I'll update the comments based on Mark H's suggestion and land the patch manually.
Comment 20 Mark Lam 2013-12-03 18:17:43 PST
Update patch landed in r160063: <http://trac.webkit.org/r160063>.