Bug 156831

Summary: Heap corruption is detected when destructing JSGlobalObject
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, bfulgham, cdumez, cmarcelo, commit-queue, ddkilzer, ggaren, joepeck, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Fujii Hironori 2016-04-20 20:03:22 PDT
[WinCairo] heap corruption is detected when destructing JSGlobalObject

trunk@199765
perl Tools/Scripts/build-webkit --debug --wincairo --64-bit

fast/dom/insertedIntoDocument-iframe.html

Log:

> Critical error detected c0000374

Callstack:

> ntdll.dll!00007fff7168e6db()	Unknown
> ntdll.dll!00007fff71690dc6()	Unknown
> ntdll.dll!00007fff71644b4a()	Unknown
> ntdll.dll!00007fff715c0f36()	Unknown
> ntdll.dll!00007fff715c09fd()	Unknown
> JavaScriptCore.dll!_free_base(void * block) Line 107	C++
> [External Code]	
> JavaScriptCore.dll!WTF::HashTable<OpaqueJSClass * __ptr64,WTF::KeyValuePair<OpaqueJSClass * __ptr64,std::unique_ptr<OpaqueJSClassContextData,std::default_delete<OpaqueJSClassContextData> > >,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<OpaqueJSClass * __ptr64,std::unique_ptr<OpaqueJSClassContextData,std::default_delete<OpaqueJSClassContextData> > > >,WTF::PtrHash<OpaqueJSClass * __ptr64>,WTF::HashMap<OpaqueJSClass * __ptr64,std::unique_ptr<OpaqueJSClassContextData,std::default_delete<OpaqueJSClassContextData> >,WTF::PtrHash<OpaqueJSClass * __ptr64>,WTF::HashTraits<OpaqueJSClass * __ptr64>,WTF::HashTraits<std::unique_ptr<OpaqueJSClassContextData,std::default_delete<OpaqueJSClassContextData> > > >::KeyValuePairTraits,WTF::HashTraits<OpaqueJSClass * __ptr64> >::~HashTable<OpaqueJSClass * __ptr64,WTF::KeyValuePair<OpaqueJSClass * __ptr64,std::unique_ptr<OpaqueJSClassContextData,std::default_delete<OpaqueJSClassContextData> > >,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<OpaqueJSClass * __ptr64,std::unique_ptr<OpaqueJSClassContextData,std::default_delete<OpaqueJSClassContextData> > > >,WTF::PtrHash<OpaqueJSClass * __ptr64>,WTF::HashMap<OpaqueJSClass * __ptr64,std::unique_ptr<OpaqueJSClassContextData,std::default_delete<OpaqueJSClassContextData> >,WTF::PtrHash<OpaqueJSClass * __ptr64>,WTF::HashTraits<OpaqueJSClass * __ptr64>,WTF::HashTraits<std::unique_ptr<OpaqueJSClassContextData,std::default_delete<OpaqueJSClassContextData> > > >::KeyValuePairTraits,WTF::HashTraits<OpaqueJSClass * __ptr64> >() Line 363	C++
> [External Code]	
> JavaScriptCore.dll!JSC::JSGlobalObject::~JSGlobalObject() Line 248	C++
> [External Code]	
> WebKit.dll!WebCore::JSDOMWindowBase::destroy(JSC::JSCell * cell) Line 100	C++
> JavaScriptCore.dll!JSC::Heap::FinalizerOwner::finalize(JSC::Handle<enum JSC::Unknown> handle, void * context) Line 1560	C++
> JavaScriptCore.dll!JSC::WeakBlock::finalize(JSC::WeakImpl * weakImpl) Line 53	C++
> JavaScriptCore.dll!JSC::WeakBlock::sweep() Line 85	C++
> JavaScriptCore.dll!JSC::WeakSet::sweep() Line 51	C++
> JavaScriptCore.dll!JSC::MarkedBlock::sweep(JSC::MarkedBlock::SweepMode sweepMode) Line 134	C++
> JavaScriptCore.dll!JSC::Sweep::operator()(JSC::MarkedBlock * block) Line 48	C++
> JavaScriptCore.dll!JSC::MarkedAllocator::forEachBlock<JSC::Sweep>(JSC::Sweep & functor) Line 159	C++
> JavaScriptCore.dll!JSC::MarkedSpace::forEachBlock<JSC::Sweep>(JSC::Sweep & functor) Line 228	C++
> JavaScriptCore.dll!JSC::MarkedSpace::forEachBlock<JSC::Sweep>() Line 244	C++
> JavaScriptCore.dll!JSC::MarkedSpace::sweep() Line 95	C++
> JavaScriptCore.dll!JSC::Heap::collectAndSweep(JSC::HeapOperation collectionType) Line 1102	C++
> WebKit.dll!JSC::Heap::collectAllGarbage() Line 168	C++
> WebKit.dll!WebCore::GCController::garbageCollectNow() Line 87	C++
> WebKit.dll!WebJavaScriptCollector::collect() Line 97	C++
> DumpRenderTreeLib.dll!GCController::collect() Line 43	C++
> DumpRenderTreeLib.dll!collectCallback(const OpaqueJSContext * context, OpaqueJSValue * function, OpaqueJSValue * thisObject, unsigned __int64 argumentCount, const OpaqueJSValue * const * arguments, const OpaqueJSValue * * exception) Line 49	C++
> JavaScriptCore.dll!JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState * exec) Line 61	C++
> JavaScriptCore.dll!JSC::LLInt::handleHostCall(JSC::ExecState * execCallee, JSC::Instruction * pc, JSC::JSValue callee, JSC::CodeSpecializationKind kind) Line 1132	C++
> JavaScriptCore.dll!JSC::LLInt::setUpCall(JSC::ExecState * execCallee, JSC::Instruction * pc, JSC::CodeSpecializationKind kind, JSC::JSValue calleeAsValue, JSC::LLIntCallLinkInfo * callLinkInfo) Line 1178	C++
> JavaScriptCore.dll!JSC::LLInt::genericCall(JSC::ExecState * exec, JSC::Instruction * pc, JSC::CodeSpecializationKind kind) Line 1262	C++
> JavaScriptCore.dll!llint_slow_path_call(JSC::ExecState * exec, JSC::Instruction * pc) Line 1268	C++
> JavaScriptCore.dll!llint_entry() Line 8582	Unknown
> [External Code]
Comment 1 Fujii Hironori 2016-04-20 20:08:44 PDT
This is caused by the heaps mismatch of allocating and deallocating.
Allocating in the heap of WebKit.dll, but Deallocating in JavaScriptCore.dll.

JSGlobalObject::createRareDataIfNeeded is inlined,
but JSGlobalObject::~JSGlobalObject is not inlined.

Callstack of allocation:

> WebKit.dll!WTF::Lock::Lock() Line 119	C++
> [External Code]	
> WebKit.dll!WTF::HashTable<WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::IdentityExtractor,WTF::PtrHash<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> > >::HashTable<WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::IdentityExtractor,WTF::PtrHash<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> > >() Line 557	C++
> WebKit.dll!WTF::HashSet<WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::PtrHash<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> > >::HashSet<WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::PtrHash<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> > >() Line 56	C++
> WebKit.dll!JSC::JSGlobalObject::JSGlobalObjectRareData::JSGlobalObjectRareData() Line 188	C++
> [External Code]	
> WebKit.dll!JSC::JSGlobalObject::createRareDataIfNeeded() Line 354	C++
> WebKit.dll!JSC::JSGlobalObject::setProfileGroup(unsigned int value) Line 626	C++
> WebKit.dll!WebCore::ScriptController::initScript(WebCore::DOMWrapperWorld & world) Line 262	C++
> WebKit.dll!WebCore::ScriptController::windowShell(WebCore::DOMWrapperWorld & world) Line 90	C++
> WebKit.dll!WebCore::ScriptController::evaluateInWorld(const WebCore::ScriptSourceCode & sourceCode, WebCore::DOMWrapperWorld & world, WebCore::ExceptionDetails * exceptionDetails) Line 154	C++
> WebKit.dll!WebCore::ScriptController::evaluate(const WebCore::ScriptSourceCode & sourceCode, WebCore::ExceptionDetails * exceptionDetails) Line 180	C++
> WebKit.dll!WebCore::ScriptElement::executeScript(const WebCore::ScriptSourceCode & sourceCode) Line 321	C++
> WebKit.dll!WebCore::ScriptElement::prepareScript(const WTF::TextPosition & scriptStartPosition, WebCore::ScriptElement::LegacyTypeSupport supportLegacyTypes) Line 245	C++
> WebKit.dll!WebCore::HTMLScriptRunner::runScript(WebCore::Element * script, const WTF::TextPosition & scriptStartPosition) Line 304	C++
> WebKit.dll!WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element> scriptElement, const WTF::TextPosition & scriptStartPosition) Line 177	C++
> WebKit.dll!WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() Line 215	C++
> WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode mode, bool parsingFragment, WebCore::PumpSession & session) Line 234	C++
> WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode mode) Line 282	C++
> WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode mode) Line 169	C++
> WebKit.dll!WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl> && inputSource) Line 396	C++
> WebKit.dll!WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter & writer) Line 60	C++
> WebKit.dll!WebCore::DocumentWriter::end() Line 255	C++
> WebKit.dll!WebCore::DocumentLoader::finishedLoading(double finishTime) Line 437	C++
> WebKit.dll!WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource * resource) Line 384	C++
> WebKit.dll!WebCore::CachedResource::checkNotify() Line 299	C++
> WebKit.dll!WebCore::CachedResource::finishLoading(WebCore::SharedBuffer * __formal) Line 316	C++
> WebKit.dll!WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer * data) Line 104	C++
> WebKit.dll!WebCore::SubresourceLoader::didFinishLoading(double finishTime) Line 431	C++
> WebKit.dll!WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle * __formal, double finishTime) Line 643	C++
> WebKit.dll!WebCore::ResourceHandleManager::downloadTimerCallback() Line 700	C++
> [External Code]	
> WebKit.dll!WebCore::Timer::fired() Line 142	C++
> WebKit.dll!WebCore::ThreadTimers::sharedTimerFiredInternal() Line 124	C++
> WebKit.dll!WebCore::ThreadTimers::setSharedTimer::__l8::<lambda>() Line 73	C++
> [External Code]	
> WebKit.dll!WebCore::MainThreadSharedTimer::fired() Line 53	C++
> WebKit.dll!WebCore::TimerWindowWndProc(HWND__ * hWnd, unsigned int message, unsigned __int64 wParam, __int64 lParam) Line 91	C++
> [External Code]	
> DumpRenderTreeLib.dll!runTest(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & inputLine) Line 1145	C++
> DumpRenderTreeLib.dll!main(int argc, const char * * argv) Line 1486	C++
> DumpRenderTreeLib.dll!dllLauncherEntryPoint(int argc, const char * * argv) Line 1517	C++
> DumpRenderTree.exe!main(int argc, const char * * argv) Line 260	C++
> [External Code]
Comment 2 Fujii Hironori 2016-04-20 20:57:44 PDT
There are two ways to solve this problem:

1) Not inlined JSGlobalObject::createRareDataIfNeeded or part of it
2) Apply WTF_MAKE_FAST_ALLOCATED the classes allocated from inline functions

I don't know why AppleWin port does not have this problem, but WinCairo port does.
Comment 3 Fujii Hironori 2016-04-20 21:17:37 PDT
Created attachment 276896 [details]
Patch
Comment 4 Mark Lam 2016-04-21 11:30:14 PDT
(In reply to comment #2)
> This is caused by the heaps mismatch of allocating and deallocating.
> Allocating in the heap of WebKit.dll, but Deallocating in JavaScriptCore.dll.
> 
> JSGlobalObject::createRareDataIfNeeded is inlined,
> but JSGlobalObject::~JSGlobalObject is not inlined.

Why is this an issue?  Shouldn't both WebKit.dll and JavaScripCore.dll be allocating/deallocating from the same heap of the process that loaded them?

Can you please elaborate?
Comment 5 Fujii Hironori 2016-04-21 21:08:05 PDT
Thank you for reviewing my patch.

(In reply to comment #4)
> Why is this an issue?  Shouldn't both WebKit.dll and JavaScripCore.dll be
> allocating/deallocating from the same heap of the process that loaded them?

WebKit uses CRT static libarary.
In Source/cmake/OptionsWin.cmake:

> # Use the multithreaded static runtime library instead of the default DLL runtime.
> string(REGEX REPLACE "/MD" "/MT" ${flag_var} "${${flag_var}}")

Then,

Potential Errors Passing CRT Objects Across DLL Boundaries
https://msdn.microsoft.com/en-US/library/ms235460(v=vs.110).aspx

> Also, because each copy of the CRT library has its own heap
> manager, allocating memory in one CRT library and passing the
> pointer across a DLL boundary to be freed by a different copy of
> the CRT library is a potential cause for heap corruption.
Comment 6 Mark Lam 2016-04-22 10:57:28 PDT
Comment on attachment 276896 [details]
Patch

(In reply to comment #5)
> Thank you for reviewing my patch.
> 
> (In reply to comment #4)
> > Why is this an issue?  Shouldn't both WebKit.dll and JavaScripCore.dll be
> > allocating/deallocating from the same heap of the process that loaded them?
> 
> WebKit uses CRT static libarary.
> In Source/cmake/OptionsWin.cmake:
> 
> > # Use the multithreaded static runtime library instead of the default DLL runtime.
> > string(REGEX REPLACE "/MD" "/MT" ${flag_var} "${${flag_var}}")
> 
> Then,
> 
> Potential Errors Passing CRT Objects Across DLL Boundaries
> https://msdn.microsoft.com/en-US/library/ms235460(v=vs.110).aspx
> 
> > Also, because each copy of the CRT library has its own heap
> > manager, allocating memory in one CRT library and passing the
> > pointer across a DLL boundary to be freed by a different copy of
> > the CRT library is a potential cause for heap corruption.

This is the kind of good information that we should have in the ChangeLog to justify the change.  Please add it.  The fix to add WTF_MAKE_FAST_ALLOCATED looks good to me.  I'll re-review after you've updated the patch.

r- for now.
Comment 7 Fujii Hironori 2016-04-24 20:02:53 PDT
Created attachment 277215 [details]
Patch
Comment 8 Darin Adler 2016-04-24 22:16:52 PDT
Comment on attachment 277215 [details]
Patch

Looks like a good change.

I have these concerns:

1) A code change to shared code like this should not use the [WinCairo] prefix on the bug title.

2) The explanation covers why JSGlobalObjectRareData needs to be changed, but does not explain the Lock change at all.

3) Nothing has been done to prevent this kind of problem from happening in the future in the WinCairo port; other ports don’t depend so sensitively on the the use of WTF_MAKE_FAST_ALLOCATED. Someone needs to come up with a technique to prevent this kind of error from being reintroduced in the future.

Otherwise the patch looks OK to me.
Comment 9 Fujii Hironori 2016-04-24 22:57:48 PDT
Thank you for reviewing my patch.
Comment 10 Fujii Hironori 2016-04-24 23:32:39 PDT
Created attachment 277226 [details]
Patch
Comment 11 Mark Lam 2016-04-25 09:42:36 PDT
Comment on attachment 277226 [details]
Patch

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

Please fix the typos.

> Source/JavaScriptCore/ChangeLog:8
> +        WebKit uses CRT static libarary on Windows.  Each copy of the CRT

typo: /libarary/library/.

> Source/JavaScriptCore/ChangeLog:23
> +        the inlined consturctor of JSGlobalObjectRareData.

typo: /consturctor/constructor/.

> Source/WTF/ChangeLog:8
> +        WebKit uses CRT static libarary on Windows.  Each copy of the CRT

Ditto.  Please fix typo.

> Source/WTF/ChangeLog:23
> +        the inlined consturctor of JSGlobalObjectRareData.

Please fix typo.
Comment 12 Fujii Hironori 2016-04-25 17:51:24 PDT
Created attachment 277296 [details]
Patch

Thank you for r+. I have no permission to commit, created a new patch by fixing the typos.
Comment 13 Geoffrey Garen 2016-04-25 19:32:00 PDT
Comment on attachment 277296 [details]
Patch

r=me

👍
Comment 14 WebKit Commit Bot 2016-04-25 20:21:42 PDT
Comment on attachment 277296 [details]
Patch

Clearing flags on attachment: 277296

Committed r200068: <http://trac.webkit.org/changeset/200068>
Comment 15 WebKit Commit Bot 2016-04-25 20:21:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2016-04-26 01:22:25 PDT
<rdar://problem/25926852>