Bug 150752 – Web Inspector: Console: Variables defined with let/const aren't accessible outside of console's scope

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

> let x = 1
< undefined
> x
< ReferenceError: Can't find variable: x

> const y = 2
< undefined
> y
< ReferenceError: Can't find variable: y

This is particularly inconvenient when copy/pasting code snippets

Same problem persists in Chrome DevTools, but it works well in Firefox DevTools.

[Radar WebKit Bug Importer]

<rdar://problem/23343385>

[Timothy Hatcher]

A developer on Twitter hit this and was wonder why it was failing.

https://twitter.com/immatthamlin/status/717129260964659200

He also says Chrome 51 works. https://twitter.com/immatthamlin/status/717129950025875456

[Joseph Pecoraro]

> He also says Chrome 51 works.
> https://twitter.com/immatthamlin/status/717129950025875456

It was addressed in Blink by eliminating the `with` scope in console evaluations.
<https://chromium.googlesource.com/chromium/src/+/38b555367eaa0f03931b75ea9134148659e45bd7>

[Joseph Pecoraro]

*** Bug 144034 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

Created attachment 278235 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 278235 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.h:460:  The parameter name "scope" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

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

Attachment 278235 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1275235

New failing tests:
http/tests/inspector/console/cross-domain-inspected-node-access.html

[Build Bot]

Created attachment 278238 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

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

Attachment 278235 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1275239

New failing tests:
http/tests/inspector/console/cross-domain-inspected-node-access.html

[Build Bot]

Created attachment 278239 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

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

Attachment 278235 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1275399

New failing tests:
http/tests/inspector/console/cross-domain-inspected-node-access.html

[Build Bot]

Created attachment 278244 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Timothy Hatcher]

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

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

I'll let someone on the JSC team do the final review.

> LayoutTests/inspector/debugger/js-stacktrace-expected.txt:22
> -        "functionName": "Eval Code",
> +        "functionName": "Global Code",

Why is this one title case…?

> LayoutTests/inspector/debugger/js-stacktrace-expected.txt:43
> -        "functionName": "eval code",
> +        "functionName": "global code",

… and this one isn't?

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:528
> +        // When not evaluating on a call frame, we evaluate as a program
> +        // with the Command Line API as a scope extension object.
> +        var result = InjectedScriptHost.evaluateWithScopeExtension(expression, commandLineAPI);
> +        if (saveResult)
> +            this._saveResult(result);
> +        return result;

Nice! Too bad the while paused code is still so nasty.

[Mark Lam]

Are the EWS sadness relevant?

[Joseph Pecoraro]

(In reply to comment #13)
> 
> > LayoutTests/inspector/debugger/js-stacktrace-expected.txt:22
> > -        "functionName": "Eval Code",
> > +        "functionName": "Global Code",
> 
> Why is this one title case…?
> 
> > LayoutTests/inspector/debugger/js-stacktrace-expected.txt:43
> > -        "functionName": "eval code",
> > +        "functionName": "global code",
> 
> … and this one isn't?

The title case is our localized strings constructed from the stack trace (WebInspector.StackTrace / WebInspector.CallFrame). The lowercase is what JavaScriptCore actually produces in error callstacks. I'll take a look to see if we are missing a place where we can be providing the better strings.


(In reply to comment #14)
> Are the EWS sadness relevant?

Yes, but that is just a single http/inspector test that needs to be rebaselined.

[Mark Lam]

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

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

> Source/JavaScriptCore/ChangeLog:11
> +        in Scopes fails to resolve anything, consult the Scope Extension.

lower case "scopes" here?

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:101
> +    if (!scriptValue.isString())
> +        return scriptValue;

Is this correct  ... that we want to return the bad argument if it's not a string?  Should we be throwing an error instead?  Looks like we're already getting this treatment below if toString() fails.

> Source/JavaScriptCore/inspector/JSInjectedScriptHostPrototype.cpp:173
> +    ASSERT_GC_OBJECT_INHERITS(castedThis, JSInjectedScriptHost::info());

I think this assertion can never fail given the jsDynamicCast above.  Please remove since it does unnecessary work.

> Source/JavaScriptCore/runtime/Completion.cpp:131
> +        globalObject->setGlobalScopeExtension(JSWithScope::create(exec->vm(), exec->lexicalGlobalObject(), scopeExtensionObject, ignoredPreviousScope));

This seems weird that we create the WithScope with the lexicalGlobalObject but we set the extension on the VM entry globalObject.  Is this correct?

> Source/JavaScriptCore/runtime/JSScope.cpp:212
> +            if (JSScope* globalScopeExtension = scope->globalObject()->globalScopeExtension()) {
> +                JSObject* object = JSScope::objectAtScope(globalScopeExtension);
> +                if (object->hasProperty(exec, ident))
> +                    return object;
> +            }

This seems contrary to what you told me offline.  I thought you said wanted the global to take precedence over the Inspector's scopeExtension.  Here, you're checking for the property in the scope extension first.  Is this correct?

[Joseph Pecoraro]

Created attachment 278256 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

> > Source/JavaScriptCore/runtime/JSScope.cpp:212
> > +            if (JSScope* globalScopeExtension = scope->globalObject()->globalScopeExtension()) {
> > +                JSObject* object = JSScope::objectAtScope(globalScopeExtension);
> > +                if (object->hasProperty(exec, ident))
> > +                    return object;
> > +            }
> 
> This seems contrary to what you told me offline.  I thought you said wanted
> the global to take precedence over the Inspector's scopeExtension.  Here,
> you're checking for the property in the scope extension first.  Is this
> correct?

Debugging with Mark we found that my impl was mistakenly between the GlobalLexicalEnvironment and the GlobalObject itself. I wasn't aware of global properties on the global object differed from global variables. Examples of that are named properties on the DOMWindowGlobalObject (or static properties on JSGlobalObject, but none conflict with Command Line APIs).

So with this test case:

    <div id="values"></div>

There is a "global property" `values` === `window.values`, that should not be shadowed by the Command Line API function named `values`. I added a test for this and corrected the resolve.

[Joseph Pecoraro]

Created attachment 278282 [details]
[PATCH] Addressed Comments

I'm going to run full performance benchmarks, but in a quick kracken/octane/sunspider I saw negligible impact.

[Mark Lam]

Comment on attachment 278282 [details]
[PATCH] Addressed Comments

r=me if perf is unaffected.

[Joseph Pecoraro]

Created attachment 278287 [details]
[TXT] run-jsc-benchmarks results

Results look negligible to me on all the major benchmarks.

[WebKit Commit Bot]

Comment on attachment 278282 [details]
[PATCH] Addressed Comments

Clearing flags on attachment: 278282

Committed r200533: <http://trac.webkit.org/changeset/200533>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 278282 [details]
[PATCH] Addressed Comments

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

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1404
> +    for (var i = 0; i < BasicCommandLineAPI.methods.length; ++i)
> +        this[method.name] = method;

Oops, this should be `for (var method of BasicCommandLineAPI.methods)`. I probably forgot to save before git adding. I'll fix as a follow-up.

[Joseph Pecoraro]

> Oops, this should be `for (var method of BasicCommandLineAPI.methods)`. I
> probably forgot to save before git adding. I'll fix as a follow-up.

Bug 157450. Fix: <http://trac.webkit.org/changeset/200538>