Bug 52075 - Don't assert when trying to recompile JS while executing JS
Summary: Don't assert when trying to recompile JS while executing JS
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-07 13:01 PST by Benjamin Meyer
Modified: 2011-01-14 17:23 PST (History)
5 users (show)

See Also:


Attachments
proposed solution (2.62 KB, patch)
2011-01-07 13:04 PST, Benjamin Meyer
barraclough: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Meyer 2011-01-07 13:01:24 PST
We can get into this situation when page loading is deferred.

It happened when there are two PageGroups, one with deferredLoading turned on (for example, JS in one of its pages is showing an alert) and one without.  A timer scheduled by the other group, which is not deferred, can cause ALL scripts, in all page groups, to be recompiled because ScriptDebugServer is global.
Comment 1 Benjamin Meyer 2011-01-07 13:04:04 PST
Created attachment 78259 [details]
proposed solution
Comment 2 Joe Mason 2011-01-07 13:05:46 PST
Trying to make a test for this now.  I think I'll need to expose the page group name through DumpRenderTree.
Comment 3 Gavin Barraclough 2011-01-07 21:59:55 PST
Comment on attachment 78259 [details]
proposed solution

This won't work.  If you do this, then you'll end up throwing away the code for functions that are live one the stack.  The code buffer may be reused and overwritten, and then we may may a return back into that address range expecting the old code to be there.

Right now we can only handle throwing away code if the machine stack is empty, WebCore does need to ensure that it only calls this method whilst no JavaScript execution is taking place.
Comment 4 Joe Mason 2011-01-10 13:49:51 PST
(In reply to comment #3)
> (From update of attachment 78259 [details])
> This won't work.  If you do this, then you'll end up throwing away the code for functions that are live one the stack.  The code buffer may be reused and overwritten, and then we may may a return back into that address range expecting the old code to be there.
> 
> Right now we can only handle throwing away code if the machine stack is empty, WebCore does need to ensure that it only calls this method whilst no JavaScript execution is taking place.

The call stack is:

webkitCrash(const char * file=0x3c7dbc5c, int line=0x0000003c, const char * function=0x3c7dbc34)  Line 25    C++
JSC::Debugger::recompileAllJSFunctions(JSC::JSGlobalData * globalData=0x3caf0058)  Line 60 + 0x36 bytes    C++
WebCore::ScriptDebugServer::recompileAllJSFunctions(WebCore::Timer<WebCore::ScriptDebugServer> * __formal=0x3cad2240)  Line 574    C++
WebCore::Timer<WebCore::ScriptDebugServer>::fired()  Line 98 + 0x1f bytes    C++
WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 115    C++
WebCore::ThreadTimers::sharedTimerFired()  Line 91    C++

Is the problem that once recompileAllJSFunctions is called, it must complete?  In that case it might be safe to delay firing the timer as long as loading is deferred.

Or is the problem that once recompileAllJSFunctionsSoon gets called (which sets the timer), they must actually get recompiled next time through the event loop?
Comment 5 Gavin Barraclough 2011-01-10 14:31:41 PST
The problem probably lies further up the call stack.

globalData->dynamicGlobalObject should be being set upon the topmost entry into JavaScriptCore and cleared when this entry returns.  I.e. this value being non-null should correspond to their being any JavaScriptCore call frames higher up the call stack.  If this value is non-null, and there are no call frames inside JavaScript execution higher up the stack, then this is a bug – we should work out how this got set, or why it hasn't been cleared.

If there are call frames in JavaScript higher up the stack then we cannot call recompileAllJSFunctions().  There may be call frames on the stack associated with JIT code translations, and if we throw away the references to the JIT code, then the code may be trashed before we return to it.

An exemple of a sequence of events that would cause incorrect behavior would be:

(1) WebCore calls to JSC
(2) JSC JIT compiles a JavaScript function, and calls into the JIT generated code.
(3) the JIT generated code calls out to a host function (a function or property getter implemented in native code).
(4) the host callback calls from JSC back into WebCore (hence we are in WebCore, with dynamicGlobalObject set to a non-null value)
(5) WebCore calls recompileAllJSFunctions, throwing away all JIT code.  Memory previously used by JIT code is deallocated, any may be overwritten.
(6) WebCore returns back to JSC.
(7) JSC returns back from the host callback to the JIT code for the JS function it has previously been executing.
(8) The JIT code has been released, and possibly overwritten, so you may (or may not) now crash, depending on whether the JIT code happens to have been overwritten yet, the allocator in use, etc, etc.

This is why we don't currently allow code for functions to be thrown away whilst JIT call frames are on the machine stack.
Comment 6 Joe Mason 2011-01-10 15:11:21 PST
Ah, now that I look closer the call stack is actually:

JSC::Interpreter::execute
...
WebCore::Chrome::runJavaScriptAlert
...
platform code to run a nested event loop
...
WebCore::Timer<WebCore::ScriptDebugServer>::fired()
WebCore::ScriptDebugServer::recompileAllJSFunctions
JSC::Debugger::recompileAllJSFunctions

So the question is why dynamicGlobalObject is not set here.

ScriptDebugServer::recompileAllJSFunctions is:


    JSLock lock(SilenceAssertionsOnly);
    // If JavaScript stack is not empty postpone recompilation.
    if (JSDOMWindow::commonJSGlobalData()->dynamicGlobalObject)
        recompileAllJSFunctionsSoon();
    else
        Debugger::recompileAllJSFunctions(JSDOMWindow::commonJSGlobalData());

And then JSC::Debugger::recompileAllJSFunctions is:

    void Debugger::recompileAllJSFunctions(JSGlobalData* globalData)
    {
        ASSERT(!globalData->dynamicGlobalObject);

So unless JSDOMWindow::commonJSGlobalData() returns a different object every time its called, it's just repeating the same test twice, and passing the first time but failing the second time.
Comment 7 Gavin Barraclough 2011-01-10 15:38:46 PST
commonJSGlobalData() should always be returning the same value, so yes, this looks odd.
Comment 8 Joe Mason 2011-01-14 14:17:44 PST
Turns out I was only getting this error with old code that didn't have the check in recompileAllJSFunctionsSoon.
Comment 9 Joe Mason 2011-01-14 17:03:54 PST
(In reply to comment #8)
> Turns out I was only getting this error with old code that didn't have the check in recompileAllJSFunctionsSoon.

I meant the check in ScriptDebugServer::recompileAllJSFunctions
Comment 10 Daniel Bates 2011-01-14 17:23:40 PST
From talking with Joe Mason offline, this issue is invalid since we made the proposed change to Debugger.cpp with respect to an earlier revision of the file ScriptDebugServer. If this issue is still valid then please re-open this bug.