[webkit-reviews] review denied: [Bug 118918] Eager stack trace for error objects. : [Attachment 207349] patch3

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Chris Curtis
<chris_curtis at apple.com>'s request for review:
Bug 118918: Eager stack trace for error objects.
https://bugs.webkit.org/show_bug.cgi?id=118918

Attachment 207349: patch3
https://bugs.webkit.org/attachment.cgi?id=207349&action=review

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


> Source/JavaScriptCore/ChangeLog:9
> +	   For better error information, we want to be able to have the error
objects have the 
> +	   .stack property at creation.

Let's shorten "we want to be able to have the error objects" to "we want error
objects to".

The goal here isn't really better information -- although you discovered that's
a nice side effect. The goal is to fill in an API that other browsers provide,
which allows you to get the current stack trace without having to throw an
exception. You should clarify this goal in your ChangeLog.

> Source/JavaScriptCore/ChangeLog:15
> +	   For all error objects that the user explicitly creates, the
topCallFrame is at a 
> +	   new frame created to handle the user's call. In this case though,
the error 
> +	   object needs the caller's frame to create the stack trace correctly.
An enum was 
> +	   created to notify the object that it needs to adjust its stackTrace.
This allows 
> +	   the stack trace to be correct without modifying the state of
topCallFrame.

I would move this comment next to constructWithErrorConstructor and
callErrorConstructor. It helps clarify that you're talking about those specific
functions.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:626
> +void Interpreter::getStackTrace(VM* vm, Vector<StackFrame>& results, size_t
maxStackSize, BacktraceMode backTraceType)

It's bad to give one thing two names. This should either be "BacktraceMode
backtraceMode" or "BacktraceType backtraceType" -- you can pick which name you
prefer.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:675
> +	   if (asObject(error)->hasProperty(callFrame,
vm->propertyNames->stack) && vm->exceptionStack().size())

Why the check for vm->exceptionStack().size() here? Are there cases where we
initially create an ErrorInstance with a .stack property that's the empty
string, even though an exception has been thrown? If so, I think this warrants
a comment explaining the case you're guarding against.

> Source/JavaScriptCore/runtime/ErrorInstance.h:72
> +	       vm.exceptionStack() = RefCountedArray<StackFrame>(stackTrace);

Why do we do this?  It seems wrong to set the exceptionStack value when an
exception hasn't been thrown. Won't this make it look like an exception has
been thrown?

> Source/JavaScriptCore/runtime/ErrorInstance.h:80
> +	       StringBuilder builder;
> +	       for (unsigned i = 0; i < stackTrace.size(); i++) {
> +		  
builder.append(String(stackTrace[i].toString(vm.dynamicGlobalObject->globalExec
())));
> +		   if (i != stackTrace.size() - 1)
> +		       builder.append('\n');
> +	       }
> +	       putDirect(vm, vm.propertyNames->stack, jsString(&vm,
builder.toString()), ReadOnly | DontDelete);

There's enough code here that this function should move to the .cpp file and
not be inlined anymore.

> LayoutTests/ChangeLog:15
> +	   The following test was modified to remove the error element. Prior
to this patch 

"element" here should be "object".

> LayoutTests/ChangeLog:16
> +	   the error object would evaluate null because there was no stack
property. The stack 

I'm not sure what "evaluate null" means here. Are you saying it would take the
value null and try to evaluate it as a script?

> LayoutTests/ChangeLog:17
> +	   trace included a file path specific to the machine that is running
the test, The 

"is" should be "was". "." should be ",".

> LayoutTests/fast/js/script-tests/stack-at-creation-for-error-objects.js:5
> +'This test checks that there is a stack element at creation of each error
object.'

"element" should be "property" here.

> LayoutTests/fast/js/script-tests/stack-at-creation-for-error-objects.js:10
> +	   testFailed(errorObject + " did not have a .stack element at creation
time.");

Ditto.

Also, can we test for something more than just being defined? You can use
typeof and .length to verify that we have a non-empty string. That's a little
more testing.