Bug 118498 – ASSERTION FAILED: callFrame == vm->topCallFrame || callFrame == callFrame->lexicalGlobalObject()->globalExec() || callFrame == callFrame->dynamicGlobalObject()->globalExec() in JSC::Interpreter::addStackTraceIfNecessary

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Renata Hodovan]

The following script fails the assertion above:

function test() {
    var myObj = {
        toString: function() {
            throw typeof new myObj();
        },
    };
    var help = [ 1, 2 ];
    var l2 = help[myObj];
  
}

test();


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000004352e0 in WTFCrash () at /home/reni/Data/REPOS/webkitnix/Source/WTF/wtf/Assertions.cpp:339
339	    *(int *)(uintptr_t)0xbbadbeef = 0;
(gdb) bt
#0  0x00000000004352e0 in WTFCrash () at /home/reni/Data/REPOS/webkitnix/Source/WTF/wtf/Assertions.cpp:339
#1  0x000000000049322d in JSC::Interpreter::addStackTraceIfNecessary (callFrame=0x7fffb44f3e08, error=<incomplete type>)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/interpreter/Interpreter.cpp:655
#2  0x00000000004ebb41 in JSC::throwError (exec=0x7fffb44f3e08, error=0x7ffff7e8ff08)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/runtime/Error.cpp:165
#3  0x00000000004ec92d in JSC::throwStackOverflowError (exec=0x7fffb44f3e08)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:141
#4  0x0000000000494e1a in JSC::Interpreter::executeCall (this=0xeec600, callFrame=0x7fffb44f3e08, function=0x7ffff7ececb0, callType=JSC::CallTypeJS, 
    callData=..., thisValue=<incomplete type>, args=...) at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/interpreter/Interpreter.cpp:961
#5  0x00000000004e2a87 in JSC::call (exec=0x7fffb44f3e08, functionObject=<incomplete type>, callType=JSC::CallTypeJS, callData=..., 
    thisValue=<incomplete type>, args=...) at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/runtime/CallData.cpp:40
#6  0x0000000000532444 in JSC::callDefaultValueFunction (exec=0x7fffb44f3e08, object=0x7ffff7e8ff20, propertyName=<incomplete type>)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/runtime/JSObject.cpp:1344
#7  0x00000000005325d9 in JSC::JSObject::defaultValue (object=0x7ffff7e8ff20, exec=0x7fffb44f3e08, hint=JSC::PreferString)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/runtime/JSObject.cpp:1365
#8  0x000000000050ab6b in JSC::JSObject::toPrimitive (this=0x7ffff7e8ff20, exec=0x7fffb44f3e08, preferredType=JSC::PreferString)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/runtime/JSObject.h:1402
#9  0x000000000050a645 in JSC::JSCell::toPrimitive (this=0x7ffff7e8ff20, exec=0x7fffb44f3e08, preferredType=JSC::PreferString)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/runtime/JSCell.cpp:145
#10 0x000000000055365c in JSC::JSValue::toStringSlowCase (this=0x7fffff81f290, exec=0x7fffb44f3e08)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/runtime/JSCJSValue.cpp:314
#11 0x0000000000424a47 in JSC::JSValue::toString (this=0x7fffff81f290, exec=0x7fffb44f3e08)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/runtime/JSString.h:530
#12 0x00000000004ec55d in JSC::createNotAConstructorError (exec=0x7fffb44f3e08, value=<incomplete type>)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:98
#13 0x00000000004ad708 in JSC::cti_op_construct_NotJSConstruct (args=0x7fffff81f370)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/jit/JITStubs.cpp:2467
#14 0x00000000004a8cb4 in JSC::tryCacheGetByID (callFrame=0xff81f300, codeBlock=0x4ad708 <JSC::cti_op_construct_NotJSConstruct(void**)+207>, 
    returnAddress=<incomplete type>, baseValue=<incomplete type>, propertyName=..., slot=..., stubInfo=0xeec610)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/jit/JITStubs.cpp:1068
#15 0x00007fffb44f3e08 in ?? ()
#16 0x0000000000eec610 in ?? ()
#17 0x00007fffff81f3d0 in ?? ()
#18 0x000000000049b609 in JSC::JSStack::installTrapsAfterFrame (this=0x588d8b4810244c89, frame=0x48ffffff688d8b48)
    at /home/reni/Data/REPOS/webkitnix/Source/JavaScriptCore/interpreter/JSStackInlines.h:212
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

[Renata Hodovan]

The test was checked on a Linux x86_64 PC.

[Mark Lam]

A quick gdb session says:

(gdb) p callFrame
$12 = (CallFrame *) 0x1102b8b68

(gdb) p vm->topCallFrame
$13 = (class JSC::ExecState *) 0x1102b8ba8

(gdb) p vm->topCallFrame->callerFrame()
$14 = (CallFrame *) 0x1102b8b68

Looks like this is a case where the topCallFrame has been popped already, but the topCallFrame pointer itself has not been updated yet.

[Chris Curtis]

Created attachment 206402 [details]
Modified throwExceptionFromOpCall to take in a function pointer.

ThrowExceptionFromOpCall maintains the topCallFrame for the error throw. In the cases where the Error needs to be created it was being passed exec->callerFrame(), but the topCallFrame was not being adjusted. When it was time to get the stack, the assert check saw that the topCallFrame was still pointed at exec not exec->callerFrame() and caused the crash. By waiting to create the error until after the maintenance of topCallFrame, the topCallFrames match.

[WebKit Commit Bot]

Attachment 206402 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/js/assert-fail-not-a-constructor-error-expected.txt', u'LayoutTests/fast/js/assert-fail-not-a-constructor-error.html', u'LayoutTests/fast/js/assert-fail-not-a-function-error-expected.txt', u'LayoutTests/fast/js/assert-fail-not-a-function-error.html', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/jit/JITStubs.cpp']" exit_code: 1
Source/JavaScriptCore/jit/JITStubs.cpp:1164:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/jit/JITStubs.cpp:1169:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Curtis]

Created attachment 206403 [details]
Fixed Styling Errors

[Chris Curtis]

Created attachment 206406 [details]
patch 3

[Filip Pizlo]

Comment on attachment 206406 [details]
patch 3

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

This is pretty cool, but it would be awesome to remove the code duplication.

> Source/JavaScriptCore/jit/JITStubs.cpp:1183
> -template<typename T> static T throwExceptionFromOpCall(JITStackFrame& jitStackFrame, CallFrame* newCallFrame, ReturnAddressPtr& returnAddressSlot, JSValue exception)
> +template<typename T> static T throwExceptionFromOpCall(JITStackFrame& jitStackFrame, CallFrame* newCallFrame, ReturnAddressPtr& returnAddressSlot, JSObject* (*createError)(ExecState*, JSValue))
>  {
> -    newCallFrame->callerFrame()->vm().exception = exception;
> -    return throwExceptionFromOpCall<T>(jitStackFrame, newCallFrame, returnAddressSlot);
> +    CallFrame* callFrame = newCallFrame->callerFrame();
> +    jitStackFrame.callFrame = callFrame;
> +    callFrame->vm().topCallFrame = callFrame;
> +    callFrame->vm().exception = (*createError)(callFrame, callFrame->callee());
> +    ASSERT(callFrame->vm().exception);
> +    returnToThrowTrampoline(&callFrame->vm(), ReturnAddressPtr(newCallFrame->returnPC()), returnAddressSlot);
> +    return T();
> +}
> +template<typename T> static T throwExceptionFromOpCall(JITStackFrame& jitStackFrame, CallFrame* newCallFrame, ReturnAddressPtr& returnAddressSlot, JSObject* (*createError)(ExecState*))
> +{
> +    CallFrame* callFrame = newCallFrame->callerFrame();
> +    jitStackFrame.callFrame = callFrame;
> +    callFrame->vm().topCallFrame = callFrame;
> +    callFrame->vm().exception = (*createError)(callFrame);
> +    ASSERT(callFrame->vm().exception);
> +    returnToThrowTrampoline(&callFrame->vm(), ReturnAddressPtr(newCallFrame->returnPC()), returnAddressSlot);
> +    return T();
>  }

This is cool.  But, I would recommend doing some more hacking to avoid this duplicated code.  I think you're duplicating it because of different createError signatures, right?

It seems like there are two equally decent ways of getting around this.  Both of them will result in a net increase of code, but IMHO more code in this case is better than duplicated code, particularly given the gnarly nature of this code.

1) Create a base class called ErrorCreator like so: class ErrorCreator { public: virtual JSObject* createError(ExecState*, JSValue); }.  Then have two subclasses, one that wraps a "JSObject* (*createError)(ExecState*, JSValue)" and another than wraps a "JSObject* (*createError)(ExecState*)".

2) Do the same but using a functor.  A functor just means that you omit the base class, and have throwExceptionFromOpCall() be templatized on both T and the type of the ErrorCreator.  This saves a virtual call and means you don't have to write the code for the base class, but OTOH it's somewhat more confusing (both to write and maintain) and leads to code duplication (each instance of the template will mean the same code splatted again by the compiler).

> Source/JavaScriptCore/jit/JITStubs.cpp:1478
> +        return throwExceptionFromOpCall<void*>(stackFrame, callFrame, STUB_RETURN_ADDRESS, &createStackOverflowError);

It ought not be necessary to say "&" here.  In C and C++, if you refer to a function by saying "createStackOverflowError" and use it in a function pointer context, you get the & for free.

Weird, I know - but we regularly omit the "&" when making function pointers.

> Source/JavaScriptCore/jit/JITStubs.cpp:2177
> +        return throwExceptionFromOpCall<void*>(stackFrame, callFrame, STUB_RETURN_ADDRESS, &createStackOverflowError);

Ditto.

> Source/JavaScriptCore/jit/JITStubs.cpp:2190
> +        return throwExceptionFromOpCall<void*>(stackFrame, callFrame, STUB_RETURN_ADDRESS, &createStackOverflowError);

Ditto.

> Source/JavaScriptCore/jit/JITStubs.cpp:2355
> +        return throwExceptionFromOpCall<EncodedJSValue>(stackFrame, callFrame, STUB_RETURN_ADDRESS, &createNotAFunctionError);

Ditto.

> Source/JavaScriptCore/jit/JITStubs.cpp:2481
> +        return throwExceptionFromOpCall<EncodedJSValue>(stackFrame, callFrame, STUB_RETURN_ADDRESS, &createNotAConstructorError);

Ditto.

[Chris Curtis]

Created attachment 206473 [details]
patch with functors

The patch with functors. Something to note, This does not address separate stack overflow issue which creates the stack trace each time. The test cases right now will time out when running correctly and crash when it fails. I have not looked into this too much yet, but Oliver mentioned it possibly had something to do with toStringSlowCase. Should that be addressed in this patch as well or in a new bug?

[Chris Curtis]

For added information the assert was crashing because the topCallFrame was not matching this patch fixed that part. but now it is inside of an infinite loop of trying to create the myObj and each time it is fining that it is not an object and creating a stack trace for it. This is why it is timing out.

[Geoffrey Garen]

Comment on attachment 206473 [details]
patch with functors

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

Neat cleanup.

> Something to note, This does not address separate stack overflow issue which creates the stack trace each time. The test cases right now will time out when running correctly and crash when it fails.

Which test cases do you mean?

It's OK to land this patch, with this test, and address a related time-out issue in a separate patch. But it's probably not OK if the test attached to this patch times out, or if this patch causes an existing LayoutTest to start timing out.

Which case is this?

A few more cleanups below...

> Source/JavaScriptCore/jit/JITStubs.cpp:1225
> +    ErrorWithExceptionFunctor functor= ErrorWithExceptionFunctor(callFrame->vm().exception);

Missing a space here.

> Source/JavaScriptCore/jit/JITStubs.cpp:1226
> +    return throwExceptionFromOpCall <T> (jitStackFrame, newCallFrame, returnAddressSlot, functor);

Should put spaces around <T> here.

> LayoutTests/fast/js/assert-fail-not-a-constructor-error-expected.txt:12
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x576
> +      RenderBlock {P} at (0,0) size 784x18
> +        RenderText {#text} at (0,0) size 660x18
> +          text run at (0,0) width 660: "Must be run on a debug build to see test correctly. Test for Assert crash on create not a Constructor error."

Please use testRunner.dumpAsText() to turn these results into text. You can look at almost any other fast/js test to see how this works.

> LayoutTests/fast/js/assert-fail-not-a-constructor-error.html:27
> +console.log("DONE!");

Minor nit: We typically use the word "PASS" to indicate that a test completed successfully.

> LayoutTests/fast/js/assert-fail-not-a-function-error-expected.txt:12
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x576
> +      RenderBlock {P} at (0,0) size 784x18
> +        RenderText {#text} at (0,0) size 638x18
> +          text run at (0,0) width 638: "Must be run on a debug build to see test correctly. Test for Assert crash on create not a function error."

Ditto.

> LayoutTests/fast/js/assert-fail-not-a-function-error.html:27
> +console.log("DONE!");

Minor nit: We typically use the word "PASS" to indicate that a test completed successfully.

[Geoffrey Garen]

> > +    return throwExceptionFromOpCall <T> (jitStackFrame, newCallFrame, returnAddressSlot, functor);

> Should put spaces around <T> here.

Correction: Should *not* put spaces around <T> here.

[Chris Curtis]

Created attachment 206481 [details]
patch 4

To answer your question about the tests. It is the case where these tests(the two tests included in this patch) time out because of the stack overflow error, but without the patch in a Debug build these tests will crash immediately.

[Build Bot]

Comment on attachment 206481 [details]
patch 4

Attachment 206481 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/985428

New failing tests:
fast/js/assert-fail-not-a-constructor-error.html
fast/js/assert-fail-not-a-function-error.html

[Build Bot]

Created attachment 206491 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 206481 [details]
patch 4

Attachment 206481 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1043314

New failing tests:
fullscreen/full-screen-iframe-with-max-width-height.html
fast/js/assert-fail-not-a-function-error.html
fast/js/assert-fail-not-a-constructor-error.html

[Build Bot]

Created attachment 206493 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Chris Curtis]

Created attachment 206910 [details]
Patch 5

Fixes the assertion failure on the debug build and prevents error exceptions from creating additional exceptions.

[Geoffrey Garen]

Comment on attachment 206910 [details]
Patch 5

One quick minor comment:

Please remove the '' around the type in the error message. '' was appropriate when we were quoting source content. Now, we're just printing a type.

[Chris Curtis]

(In reply to comment #18)
> (From update of attachment 206910 [details])
> One quick minor comment:
> 
> Please remove the '' around the type in the error message. '' was appropriate when we were quoting source content. Now, we're just printing a type.

For objects, do you want "object" or <nameOfObject>? Right now it does <nameOfObject>.

[Geoffrey Garen]

I think <nameOfObject> is cool.

[Build Bot]

Comment on attachment 206910 [details]
Patch 5

Attachment 206910 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1096348

New failing tests:
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
plugins/npruntime/object-from-destroyed-plugin.html

[Build Bot]

Created attachment 206943 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Chris Curtis]

Created attachment 206999 [details]
patch6

removed the ' ' and fixed the expected.txt files.

[Geoffrey Garen]

Comment on attachment 206999 [details]
patch6

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

r=me

> Source/JavaScriptCore/ChangeLog:16
> +        Created new throwExceptionFromOpCall that takes in a functor that contains
> +        a function pointer (to create the errorObject) instead of a JSValue. Inside
> +        of throwExceptionFromOpCall the topCallFrame is being rolled back in order
> +        to handle the error throw. By passing the function pointer in, we can defer
> +        the creation of the error object until after topCallFrame has been rolled 
> +        back. This allows the error object to be created with the appropriate top 
> +        frame.

This explanation is much improved from the original!

[Build Bot]

Comment on attachment 206999 [details]
patch6

Attachment 206999 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1101562

New failing tests:
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
plugins/npruntime/object-from-destroyed-plugin.html
editing/spelling/markers.html

[Build Bot]

Created attachment 207001 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Chris Curtis]

Created attachment 207020 [details]
patch7

Fixed the remaining tests that existed on wk2.

[Geoffrey Garen]

Comment on attachment 207020 [details]
patch7

r=me

[WebKit Commit Bot]

Comment on attachment 207020 [details]
patch7

Clearing flags on attachment: 207020

Committed r152871: <http://trac.webkit.org/changeset/152871>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.