Bug 157510 – Web Inspector: Eliminate the crazy code for evaluateOnCallFrame

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
Eliminate the crazy code for evaluateOnCallFrame.

While ingenious, it was very complex and difficult and confusing. Move to using a global scope extension and unify as much of the code for Debugger.evaluateOnCallFrame and Runtime.evaluate as possible.

[Radar WebKit Bug Importer]

<rdar://problem/26191332>

[Joseph Pecoraro]

Created attachment 278484 [details]
[PATCH] Proposed Fix

One very interesting bit is that in non-strict call frames I'm able to create local variables `var local = 1`, but in strict call frames I'm not. Probably an optimization difference? Or maybe based on the number of locals in the function? In any case, the test discovers this, but the behavior could go either way and I would be fine with it.

[Blaze Burg]

Comment on attachment 278484 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=278484&action=review

> LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI-expected.txt:40
> +PASS: Script evaluation. Should not be able to access arguments.callee.

Nit: 'strict'

> LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI-expected.txt:43
> +PASS: Local variable `keys` should be shadowed by CommandLineAPI `keys` function in call frame for `foo`.

This description does not match the test case name.

> LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html:82
> +            testEvaluateOnStrictCallFrame("a", (x) => { InspectorTest.expectThat(x === 6, "`a` should be 5 in `bar`."); });

Assertion message doesn't match the condition.

[Blaze Burg]

Comment on attachment 278484 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=278484&action=review

Looks like a big win to me. I would like Tim or Mark to look at this too.

> LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html:170
> +                InspectorTest.expectThat(resultValue === 123, "Local variable `keys` should be shadowed by CommandLineAPI `keys` function in call frame for `foo`.");

See above, this seems wrong.

> Source/JavaScriptCore/ChangeLog:19
> +        DebuggerCallFrame::evaluteWithScopeExtension. When evaluating globally

Typo: evaluateWithScopeExtension

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:-490
> -                "var __originalEval = global.eval; global.eval = __eval;" +

I had no idea this nasty stuff was being done. I wonder if it was hurting performance by doing weird things to globals/builtins.

> Source/JavaScriptCore/inspector/JSJavaScriptCallFramePrototype.cpp:-93
> -    ASSERT_GC_OBJECT_INHERITS(castedThis, JSJavaScriptCallFrame::info());

The changelog didn't say anything about this change. What's it for?

[Saam Barati]

(In reply to comment #2)
> Created attachment 278484 [details]
> [PATCH] Proposed Fix
> 
> One very interesting bit is that in non-strict call frames I'm able to
> create local variables `var local = 1`, but in strict call frames I'm not.
> Probably an optimization difference? Or maybe based on the number of locals
> in the function? In any case, the test discovers this, but the behavior
> could go either way and I would be fine with it.

This sounds like how strict mode eval works in general. In strict mode
eval you can not inject variables into your parent var environment. The
eval itself creates its own var environment. 
In sloppy mode eval, all vars declared in the eval are injected into
the parent var environment because the eval doesn't have a local var
environment.

[Timothy Hatcher]

Comment on attachment 278484 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=278484&action=review

So awesome!

>> Source/JavaScriptCore/inspector/InjectedScriptSource.js:-490
>> -                "var __originalEval = global.eval; global.eval = __eval;" +
> 
> I had no idea this nasty stuff was being done. I wonder if it was hurting performance by doing weird things to globals/builtins.

It was all for science…

[Joseph Pecoraro]

> > Source/JavaScriptCore/inspector/JSJavaScriptCallFramePrototype.cpp:-93
> > -    ASSERT_GC_OBJECT_INHERITS(castedThis, JSJavaScriptCallFrame::info());
> 
> The changelog didn't say anything about this change. What's it for?

This is just asserting the object inherits from JSJavaScriptCallFrame. However the code above it (in all these functions) does a dynamicCast and returns if it isn't that type. So the assert will never fail.

[Joseph Pecoraro]

Created attachment 278516 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 278516 [details]
[PATCH] For Landing

Rejecting attachment 278516 [details] from commit-queue.

New failing tests:
accessibility/meter-element.html
Full output: http://webkit-queues.webkit.org/results/1299711

[WebKit Commit Bot]

Created attachment 278518 [details]
Archive of layout-test-results from webkit-cq-02 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-02  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/200634>