[Webkit-unassigned] [Bug 66571] Keep track of topCallFrame for Stack traces

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66571


Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #104592|review?                     |review-
               Flag|                            |




--- Comment #11 from Geoffrey Garen <ggaren at apple.com>  2011-08-22 15:14:49 PST ---
(From update of attachment 104592)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.