Bug 136192 – After r172867 another crash in in js/dom/line-column-numbers.html

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

[Michael Saboff]

After r172867: <http://trac.webkit.org/changeset/172867>, there is a new crash in js/dom/line-column-numbers.html:

Process:         com.apple.WebKit.WebContent.Development [38894]
Path:            /Volumes/VOLUME/*/WebKit.framework/Versions/A/XPCServices/com.apple.WebKit.WebContent.Development.xpc/Contents/MacOS/com.apple.WebKit.WebContent.Development
Identifier:      com.apple.WebKit.WebContent.Development
Version:         538+ (538.45+)
Code Type:       X86-64 (Native)
Parent Process:  ??? [1]
Responsible:     com.apple.WebKit.WebContent.Development [38894]
User ID:         501

Date/Time:       2014-08-22 16:50:17.556 -0700
OS Version:      Mac OS X 10.9.4 (13E28)
Report Version:  11
Anonymous UUID:  615A0368-B225-16FD-FF14-A202D44A3EC4


Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: EXC_I386_GPFLT

Application Specific Information:
CRASHING TEST:js/dom/line-column-numbers.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore          0x0000000112cfbea7 JSC::CodeBlock::handlerForBytecodeOffset(unsigned int) + 39 (Vector.h:610)
1   com.apple.JavaScriptCore          0x0000000112f01f5f JSC::UnwindFunctor::operator()(JSC::StackVisitor&) + 111 (Interpreter.cpp:665)
2   com.apple.JavaScriptCore          0x0000000112efef5b JSC::Interpreter::unwind(void*&, JSC::ExecState*&, JSC::JSValue&) + 555 (StackVisitor.h:129)
3   com.apple.JavaScriptCore          0x0000000112f1940b JSC::genericUnwind(JSC::VM*, JSC::ExecState*, JSC::JSValue) + 91 (JITExceptions.cpp:67)
4   com.apple.JavaScriptCore          0x0000000112f38e4c lookupExceptionHandlerFromCallerFrame + 60 (JITOperations.cpp:1854)
5   ???                               0x000046399ca02d83 0 + 77213254823299
6   com.apple.JavaScriptCore          0x00000001130015e9 vmEntryToJavaScript + 326
7   com.apple.JavaScriptCore          0x0000000112f17b73 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 35 (VM.h:373)
8   com.apple.JavaScriptCore          0x0000000112f01096 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 438 (Interpreter.cpp:989)
9   com.apple.JavaScriptCore          0x0000000112ceb9be JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 62 (CallData.cpp:39)
10  com.apple.JavaScriptCore          0x0000000112f90295 JSC::JSObject::defaultValue(JSC::JSObject const*, JSC::ExecState*, JSC::PreferredPrimitiveType) + 1189 (Register.h:116)
11  ???                               0x000046399ca02c8c 0 + 77213254823052
12  com.apple.JavaScriptCore          0x00000001130015e9 vmEntryToJavaScript + 326
13  com.apple.JavaScriptCore          0x0000000112f17b73 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 35 (VM.h:373)
14  com.apple.JavaScriptCore          0x0000000112f01096 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 438 (Interpreter.cpp:989)
15  com.apple.JavaScriptCore          0x0000000112ceb9be JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 62 (CallData.cpp:39)
16  com.apple.JavaScriptCore          0x0000000112f90295 JSC::JSObject::defaultValue(JSC::JSObject const*, JSC::ExecState*, JSC::PreferredPrimitiveType) + 1189 (Register.h:116)
17  ???                               0x000046399ca02c8c 0 + 77213254823052
18  com.apple.JavaScriptCore          0x00000001130015e9 vmEntryToJavaScript + 326
19  com.apple.JavaScriptCore          0x0000000112f17b73 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 35 (VM.h:373)
20  com.apple.JavaScriptCore          0x0000000112f01096 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 438 (Interpreter.cpp:989)
21  com.apple.JavaScriptCore          0x0000000112ceb9be JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 62 (CallData.cpp:39)

[Michael Saboff]

Created attachment 237026 [details]
Patch

[Mark Lam]

Comment on attachment 237026 [details]
Patch

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

> Source/JavaScriptCore/jit/JITOperations.cpp:1847
> -    NativeCallFrameTracer tracer(vm, callerFrame);
> +    NativeCallFrameTracerWithRestore tracer(vm, vmEntryFrame, callerFrame);

Why the need for the restoration of vm->topVMEntryFrame and vm->topCallFrame?

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 237026 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237026&action=review
> 
> > Source/JavaScriptCore/jit/JITOperations.cpp:1847
> > -    NativeCallFrameTracer tracer(vm, callerFrame);
> > +    NativeCallFrameTracerWithRestore tracer(vm, vmEntryFrame, callerFrame);
> 
> Why the need for the restoration of vm->topVMEntryFrame and vm->topCallFrame?

We need to use the caller's CallFrame and VMEntryFrame when calling genericUnwind(). NativeCallFrameTracerWithRestore() does that for us.

In general, NativeCallFrameTracerWithRestore(), restores the values because we may do more processing that requires the current callFrame and vmEntryFrame before we get to the catch handler where we change these to the catch values.  In this particular case, that restoration isn't currently needed, but we add complexity and possible future confusion if we create another NativeCallFrameTracerXXX() version that doesn't restore the values.

[Geoffrey Garen]

Comment on attachment 237026 [details]
Patch

r=me

The sentinel value option is looking better every day. VM::topVMEntryFrame is turning out to be very subtle, and easy to use wrong.

[Mark Lam]

Comment on attachment 237026 [details]
Patch

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

>>> Source/JavaScriptCore/jit/JITOperations.cpp:1847
>>> +    NativeCallFrameTracerWithRestore tracer(vm, vmEntryFrame, callerFrame);
>> 
>> Why the need for the restoration of vm->topVMEntryFrame and vm->topCallFrame?
> 
> We need to use the caller's CallFrame and VMEntryFrame when calling genericUnwind(). NativeCallFrameTracerWithRestore() does that for us.
> 
> In general, NativeCallFrameTracerWithRestore(), restores the values because we may do more processing that requires the current callFrame and vmEntryFrame before we get to the catch handler where we change these to the catch values.  In this particular case, that restoration isn't currently needed, but we add complexity and possible future confusion if we create another NativeCallFrameTracerXXX() version that doesn't restore the values.

Would you mind adding this comment to the ChangeLog?  I think it’s more informative than the existing one that says "NativeCallFrameTracerWithRestore() so that VM::topVMEntryFrame will be updated before calling genericUnwind()."

[Michael Saboff]

Committed r172932: <http://trac.webkit.org/changeset/172932>

[Michael Saboff]

(In reply to comment #5)
> (From update of attachment 237026 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237026&action=review
> 
> >>> Source/JavaScriptCore/jit/JITOperations.cpp:1847
> >>> +    NativeCallFrameTracerWithRestore tracer(vm, vmEntryFrame, callerFrame);
> >> 
> >> Why the need for the restoration of vm->topVMEntryFrame and vm->topCallFrame?
> > 
> > We need to use the caller's CallFrame and VMEntryFrame when calling genericUnwind(). NativeCallFrameTracerWithRestore() does that for us.
> > 
> > In general, NativeCallFrameTracerWithRestore(), restores the values because we may do more processing that requires the current callFrame and vmEntryFrame before we get to the catch handler where we change these to the catch values.  In this particular case, that restoration isn't currently needed, but we add complexity and possible future confusion if we create another NativeCallFrameTracerXXX() version that doesn't restore the values.
> 
> Would you mind adding this comment to the ChangeLog?  I think it’s more informative than the existing one that says "NativeCallFrameTracerWithRestore() so that VM::topVMEntryFrame will be updated before calling genericUnwind()."

Added to ChangeLog.