Bug 121001

Summary: [Win] Javascript crash with DFG JIT enabled.
Product: WebKit Reporter: peavo
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: bfulgham, commit-queue, fpizlo, ggaren, mhahnenberg, szkarlen
Priority: P2 Keywords: PlatformOnly
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Unspecified   
Bug Depends on: 120998    
Bug Blocks:    
Description Flags
Patch none

Description peavo 2013-09-08 02:00:59 PDT
When I run with DFG JIT enabled on Windows, I'm frequently getting a NULL pointer access violation crash (writing).

The offending assembler instruction is:

07273082  movsd       mmword ptr ds:[0],xmm0  

I have traced this down to the code generation in the function osrExitGenerationThunkGenerator in DFGThunks.cpp.
The problem seems to be that using register GPRInfo::regT0 as parameter (e.g. JIT::storeDouble(..., GPRInfo::regT0)),
results in a call to JIT::storeDouble(FPRegisterID src, const void* address) on Windows, where the address parameter
gets the value of GPRInfo::regT0, which is 0 (eax on Windows). This causes the register to be written to address 0, hence the crash.
I assume the intention here is to write the register to the address in regT0. 

This is the stacktrace of the crash:

 	JavaScriptCore.dll!JSC::JITCode::execute(JSC::JSStack * stack, JSC::ExecState * callFrame, JSC::VM * vm)  Line 46 + 0x1e bytes	C++
 	JavaScriptCore.dll!JSC::Interpreter::execute(JSC::ProgramExecutable * program, JSC::ExecState * callFrame, JSC::JSObject * thisObj)  Line 844 + 0x36 bytes	C++
 	JavaScriptCore.dll!JSC::evaluate(JSC::ExecState * exec, const JSC::SourceCode & source, JSC::JSValue thisValue, JSC::JSValue * returnedException)  Line 85	C++
 	WebKit.dll!WebCore::JSMainThreadExecState::evaluate(JSC::ExecState * exec, const JSC::SourceCode & source, JSC::JSValue thisValue, JSC::JSValue * exception)  Line 61 + 0x20 bytes	C++
 	WebKit.dll!WebCore::ScriptController::evaluateInWorld(const WebCore::ScriptSourceCode & sourceCode, WebCore::DOMWrapperWorld * world)  Line 142 + 0x23 bytes	C++
 	WebKit.dll!WebCore::ScriptController::evaluate(const WebCore::ScriptSourceCode & sourceCode)  Line 158 + 0x16 bytes	C++
 	WebKit.dll!WebCore::ScriptElement::executeScript(const WebCore::ScriptSourceCode & sourceCode)  Line 315 + 0x17 bytes	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::executePendingScriptAndDispatchEvent(WebCore::PendingScript & pendingScript)  Line 151	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::executeParsingBlockingScript()  Line 123	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::executeParsingBlockingScripts()  Line 201 + 0x8 bytes	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element> scriptElement, const WTF::TextPosition & scriptStartPosition)  Line 191	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()  Line 273	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode mode, WebCore::PumpSession & session)  Line 292	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode mode)  Line 536 + 0x10 bytes	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode mode)  Line 237	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution()  Line 899	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource * cachedResource)  Line 939	C++
 	WebKit.dll!WebCore::CachedResource::checkNotify()  Line 369 + 0x13 bytes	C++
 	WebKit.dll!WebCore::CachedResource::finishLoading(WebCore::ResourceBuffer * __formal)  Line 385 + 0xf bytes	C++
 	WebKit.dll!WebCore::CachedScript::finishLoading(WebCore::ResourceBuffer * data)  Line 90	C++
 	WebKit.dll!WebCore::SubresourceLoader::didFinishLoading(double finishTime)  Line 283 + 0x26 bytes	C++
 	WebKit.dll!WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle * __formal, double finishTime)  Line 489 + 0x18 bytes	C++
 	WebKit.dll!WebCore::ResourceHandleManager::downloadTimerCallback(WebCore::Timer<WebCore::ResourceHandleManager> * __formal)  Line 570 + 0x35 bytes	C++
 	WebKit.dll!WebCore::Timer<WebCore::ResourceLoadScheduler>::fired()  Line 114 + 0x23 bytes	C++
 	WebKit.dll!WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 129 + 0xf bytes	C++
 	WebKit.dll!WebCore::ThreadTimers::sharedTimerFired()  Line 106	C++
 	WebKit.dll!WebCore::TimerWindowWndProc(HWND__ * hWnd, unsigned int message, unsigned int wParam, long lParam)  Line 110 + 0x8 bytes	C++
Comment 1 peavo 2013-09-08 02:06:19 PDT
Created attachment 210972 [details]
Comment 2 Filip Pizlo 2013-09-08 08:34:33 PDT
Comment on attachment 210972 [details]

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

> Source/JavaScriptCore/dfg/DFGThunks.cpp:56
> +        jit.storeDouble(FPRInfo::toRegister(i), MacroAssembler::ImplicitAddress(GPRInfo::regT0));

Use Address not ImplicitAddres.
Comment 3 peavo 2013-09-08 23:55:25 PDT
Created attachment 211012 [details]
Comment 4 Karlen Simonyan 2013-09-21 01:56:37 PDT
(In reply to comment #3)
> Created an attachment (id=211012) [details]
> Patch

Hi! Provided patch resolves some issues, but there still crash behavior - using loops (with iteration > 100) JSC just crashes.
Comment 5 peavo 2013-10-19 05:18:47 PDT
Created attachment 214651 [details]
Comment 6 peavo 2013-10-19 05:21:10 PDT
Upgraded the patch with changes in trunk.
The same fix was also needed in dfg/DFGOSRExitCompiler32_64.cpp.
I also added an assert if we try to generate code which writes to a null pointer.
Comment 7 Geoffrey Garen 2013-10-25 14:35:18 PDT
Comment on attachment 214651 [details]


Would be nice to make the GPRInfo::regT0 type incompatible with void*, so this became a compile error.
Comment 8 WebKit Commit Bot 2013-10-25 14:59:56 PDT
Comment on attachment 214651 [details]

Clearing flags on attachment: 214651

Committed r158057: <http://trac.webkit.org/changeset/158057>
Comment 9 WebKit Commit Bot 2013-10-25 14:59:59 PDT
All reviewed patches have been landed.  Closing bug.