Full Text Bug Listing

Mark Lam

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

Mark Hahnenberg

Comment 1 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?

Mark Lam

Comment 2 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.

Mark Hahnenberg

Comment 3 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)?

Mark Lam

Comment 4 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.

Mark Hahnenberg

Comment 5 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.)?

Radar WebKit Bug Importer

Comment 6 2013-10-11 14:28:16 PDT

<rdar://problem/15211539>

Geoffrey Garen

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

peavo

Comment 8 2013-12-03 07:44:06 PST

Created attachment 218294 [details] Patch

peavo

Comment 9 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.

WebKit Commit Bot

Comment 10 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>

WebKit Commit Bot

Comment 11 2013-12-03 09:07:09 PST

All reviewed patches have been landed. Closing bug.

Brent Fulgham

Comment 12 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!

Mark Lam

Comment 13 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.

Mark Lam

Comment 14 2013-12-03 17:38:56 PST

Reopening this bug to apply the better fix in OSAllocatorWin.

Mark Lam

Comment 15 2013-12-03 17:40:14 PST

Created attachment 218367 [details] Fixing OSAllocatorWin instead.

Brent Fulgham

Comment 16 2013-12-03 17:41:09 PST

Comment on attachment 218367 [details] Fixing OSAllocatorWin instead. r=me

Mark Hahnenberg

Comment 17 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.

Mark Hahnenberg

Comment 18 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.

Mark Lam

Comment 19 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.

Mark Lam

Comment 20 2013-12-03 18:17:43 PST

Update patch landed in r160063: <http://trac.webkit.org/r160063>.

Attachments
Patch (2.42 KB, patch) 2013-12-03 07:44 PST, peavo	no flags	Details Formatted Diff Diff
Fixing OSAllocatorWin instead. (3.67 KB, patch) 2013-12-03 17:40 PST, Mark Lam	bfulgham: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Bug 121972