Bug 156831 – Heap corruption is detected when destructing JSGlobalObject

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Fujii Hironori]

[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]

[Fujii Hironori]

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]

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 276896 [details]
Patch

[Mark Lam]

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

[Fujii Hironori]

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.

[Mark Lam]

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.

[Fujii Hironori]

Created attachment 277215 [details]
Patch

[Darin Adler]

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.

[Fujii Hironori]

Thank you for reviewing my patch.

[Fujii Hironori]

Created attachment 277226 [details]
Patch

[Mark Lam]

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.

[Fujii Hironori]

Created attachment 277296 [details]
Patch

Thank you for r+. I have no permission to commit, created a new patch by fixing the typos.

[Geoffrey Garen]

Comment on attachment 277296 [details]
Patch

r=me

👍

[WebKit Commit Bot]

Comment on attachment 277296 [details]
Patch

Clearing flags on attachment: 277296

Committed r200068: <http://trac.webkit.org/changeset/200068>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/25926852>