Bug 136192 - After r172867 another crash in in js/dom/line-column-numbers.html
Summary: After r172867 another crash in in js/dom/line-column-numbers.html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-22 21:15 PDT by Michael Saboff
Modified: 2014-08-25 12:33 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.42 KB, patch)
2014-08-22 21:44 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2014-08-22 21:15:52 PDT
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)
Comment 1 Michael Saboff 2014-08-22 21:44:16 PDT
Created attachment 237026 [details]
Patch
Comment 2 Mark Lam 2014-08-25 10:46:24 PDT
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?
Comment 3 Michael Saboff 2014-08-25 10:57:43 PDT
(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.
Comment 4 Geoffrey Garen 2014-08-25 11:04:11 PDT
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.
Comment 5 Mark Lam 2014-08-25 11:06:33 PDT
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()."
Comment 6 Michael Saboff 2014-08-25 12:32:58 PDT
Committed r172932: <http://trac.webkit.org/changeset/172932>
Comment 7 Michael Saboff 2014-08-25 12:33:29 PDT
(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.