[webkit-reviews] review denied: [Bug 66571] Keep track of topCallFrame for Stack traces : [Attachment 104592] updated patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Juan C. Montemayor
<jmont at apple.com>'s request for review:
Bug 66571: Keep track of topCallFrame for Stack traces
https://bugs.webkit.org/show_bug.cgi?id=66571

Attachment 104592: updated patch
https://bugs.webkit.org/attachment.cgi?id=104592&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=104592&action=review


Some of the setting and resetting of topCallFrame seems haphazard in the
Interpreter.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:711
>      CodeBlock* codeBlock = callFrame->codeBlock();
>      bool isInterrupt = false;
> +    callFrame->globalData().topCallFrame = callFrame;

Why does the first act of throwing an exception need to be to set topCallFrame?
In theory, topCallFrame was already set when we entered the function that is
now throwing an exception.

On the other hand, I think you're missing an update of topCallFrame after
you've thrown an exception and decided to return to a handler. You can put the
update into unwindCallFrame().

> Source/JavaScriptCore/interpreter/Interpreter.cpp:934
>  JSValue Interpreter::executeCall(CallFrame* callFrame, JSObject* function,
CallType callType, const CallData& callData, JSValue thisValue, const ArgList&
args)
>  {
> +    callFrame->globalData().topCallFrame = callFrame;

Why here? Seems unnecessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:977
>  JSValue Interpreter::executeCall(CallFra
>	   }
>  
>	   newCallFrame->init(newCodeBlock, 0, callDataScopeChain,
callFrame->addHostCallFrameFlag(), argCount, function);
> +	   TopCallFrameSetter topCallFrameSetter(callFrame->globalData(),
newCallFrame);

Here, you caught the CallTypeJS case, but missed the non-JS case.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1030
>  JSObject* Interpreter::executeConstruct(CallFrame* callFrame, JSObject*
constructor, ConstructType constructType, const ConstructData& constructData,
const ArgList& args)
>  {
> +    callFrame->globalData().topCallFrame = callFrame;

Why here? Seems unnecessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1055
> +    TopCallFrameSetter topCallFrameSetter(callFrame->globalData(),
newCallFrame);
> +

This is too soon, since newCallFrame is not yet fully initialized, and can
change later in the function.

A good rule of thumb is to update topCallFrame right after calling
CallFrame::init, since that means you've just created a new CallFrame and fully
initialized it.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1217
>  JSValue Interpreter::execute(EvalExecutable* eval, CallFrame* callFrame,
JSValue thisValue, ScopeChainNode* scopeChain)
>  {
> +    callFrame->globalData().topCallFrame = callFrame;

Why here? Seems unnecessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1227
>  JSValue Interpreter::execute(EvalExecutable* eval, CallFrame* callFrame,
JSValue thisValue, int globalRegisterOffset, ScopeChainNode* scopeChain)
>  {
>      ASSERT(isValidThisObject(thisValue, callFrame));
> +    callFrame->globalData().topCallFrame = callFrame;

Why here? Seems unnecessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1658
>  #endif
>  
> +    *topCallFrameSlot = callFrame;
>  #if ENABLE(COMPUTED_GOTO_INTERPRETER)

Why here? Seems unnecessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4188
>		   goto vm_throw;
>	       functionReturnValue = result;
>  
> +	       *topCallFrameSlot = callFrame;

Why here? callEval() should restore topCallFrame for you.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4257
> +	       *topCallFrameSlot = newCallFrame;
>	       newCallFrame->init(0, vPC + OPCODE_LENGTH(op_call), scopeChain,
callFrame, argCount, asObject(v));

Slightly better to switch these two lines of code, so topCallFrame doesn't
point at newCallFrame until newCallFrame is in a valid state.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4427
> +	       *topCallFrameSlot = newCallFrame;
>	       newCallFrame->init(0, vPC + OPCODE_LENGTH(op_call_varargs),
scopeChain, callFrame, argCount, asObject(v));

Same comment here.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4438
> +	       *topCallFrameSlot = callFrame;

This needs to happen before CHECK_FOR_EXCEPTION(), to ensure that you put
things back into a valid state before jumping somewhere else.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4518
> +	   *topCallFrameSlot = callFrame;
>	   if (callFrame->hasHostCallFrameFlag())
>	       return returnValue;

Slightly better to change around the order of these two statements, since you
don't need to update topCallFrame if you're returning back to C++. (The C++
code should update for you.)

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4761
>	       CHECK_FOR_EXCEPTION();
>	       functionReturnValue = returnValue;
>  
> +	       *topCallFrameSlot = callFrame;

Update needs to happen before CHECK_FOR_EXCEPTION.

You missed the initial setting of topCallFrame in the ConstructTypeHost case
here.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4940
> +	   *topCallFrameSlot = callFrame;

Why here? Seems unnecessary, if exception unwinding does its job correctly.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4964
>	   codeBlock = callFrame->codeBlock();
> +	   *topCallFrameSlot = callFrame;
>	   vPC = codeBlock->instructions().begin() + handler->target;

If you fix throwException to update topCallFrame correctly, this line shouldn't
be necessary.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:5124
> +	   *topCallFrameSlot = callFrame;
>	   vPC = codeBlock->instructions().begin() + handler->target;

Same comment here.