| Summary: | After r172867 another crash in in js/dom/line-column-numbers.html | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||
| Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | fpizlo, ggaren, mark.lam | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
Michael Saboff
2014-08-22 21:15:52 PDT
Created attachment 237026 [details]
Patch
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? (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 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 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()." Committed r172932: <http://trac.webkit.org/changeset/172932> (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. |