[webkit-reviews] review denied: [Bug 118498] ASSERTION FAILED: callFrame == vm->topCallFrame || callFrame == callFrame->lexicalGlobalObject()->globalExec() || callFrame == callFrame->dynamicGlobalObject()->globalExec() in JSC::Interpreter::addStackTraceIfNecessary : [Attachment 206473] patch with functors

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Chris <chris_curtis at apple.com>'s
request for review:
Bug 118498: ASSERTION FAILED: callFrame == vm->topCallFrame || callFrame ==
callFrame->lexicalGlobalObject()->globalExec() || callFrame ==
callFrame->dynamicGlobalObject()->globalExec() in
JSC::Interpreter::addStackTraceIfNecessary
https://bugs.webkit.org/show_bug.cgi?id=118498

Attachment 206473: patch with functors
https://bugs.webkit.org/attachment.cgi?id=206473&action=review

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


Neat cleanup.

> Something to note, This does not address separate stack overflow issue which
creates the stack trace each time. The test cases right now will time out when
running correctly and crash when it fails.

Which test cases do you mean?

It's OK to land this patch, with this test, and address a related time-out
issue in a separate patch. But it's probably not OK if the test attached to
this patch times out, or if this patch causes an existing LayoutTest to start
timing out.

Which case is this?

A few more cleanups below...

> Source/JavaScriptCore/jit/JITStubs.cpp:1225
> +    ErrorWithExceptionFunctor functor=
ErrorWithExceptionFunctor(callFrame->vm().exception);

Missing a space here.

> Source/JavaScriptCore/jit/JITStubs.cpp:1226
> +    return throwExceptionFromOpCall <T> (jitStackFrame, newCallFrame,
returnAddressSlot, functor);

Should put spaces around <T> here.

> LayoutTests/fast/js/assert-fail-not-a-constructor-error-expected.txt:12
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x576
> +	 RenderBlock {P} at (0,0) size 784x18
> +	   RenderText {#text} at (0,0) size 660x18
> +	     text run at (0,0) width 660: "Must be run on a debug build to see
test correctly. Test for Assert crash on create not a Constructor error."

Please use testRunner.dumpAsText() to turn these results into text. You can
look at almost any other fast/js test to see how this works.

> LayoutTests/fast/js/assert-fail-not-a-constructor-error.html:27
> +console.log("DONE!");

Minor nit: We typically use the word "PASS" to indicate that a test completed
successfully.

> LayoutTests/fast/js/assert-fail-not-a-function-error-expected.txt:12
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x576
> +	 RenderBlock {P} at (0,0) size 784x18
> +	   RenderText {#text} at (0,0) size 638x18
> +	     text run at (0,0) width 638: "Must be run on a debug build to see
test correctly. Test for Assert crash on create not a function error."

Ditto.

> LayoutTests/fast/js/assert-fail-not-a-function-error.html:27
> +console.log("DONE!");

Minor nit: We typically use the word "PASS" to indicate that a test completed
successfully.