Bug 150752 - Web Inspector: Console: Variables defined with let/const aren't accessible outside of console's scope
Summary: Web Inspector: Console: Variables defined with let/const aren't accessible ou...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 144034 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-10-30 19:59 PDT by Nikita Vasilyev
Modified: 2016-05-06 22:45 PDT (History)
17 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (37.47 KB, patch)
2016-05-05 22:41 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (802.04 KB, application/zip)
2016-05-05 23:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (1.38 MB, application/zip)
2016-05-05 23:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-yosemite (1.44 MB, application/zip)
2016-05-06 00:24 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (38.85 KB, patch)
2016-05-06 11:59 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Addressed Comments (42.83 KB, patch)
2016-05-06 15:45 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[TXT] run-jsc-benchmarks results (73.91 KB, text/plain)
2016-05-06 16:10 PDT, Joseph Pecoraro
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2015-10-30 19:59:30 PDT
> 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.
Comment 1 Radar WebKit Bug Importer 2015-10-30 19:59:43 PDT
<rdar://problem/23343385>
Comment 2 Timothy Hatcher 2016-04-04 16:33:12 PDT
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
Comment 3 Joseph Pecoraro 2016-04-04 21:04:34 PDT
> 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>
Comment 4 Joseph Pecoraro 2016-05-05 22:29:39 PDT
*** Bug 144034 has been marked as a duplicate of this bug. ***
Comment 5 Joseph Pecoraro 2016-05-05 22:41:22 PDT
Created attachment 278235 [details]
[PATCH] Proposed Fix
Comment 6 WebKit Commit Bot 2016-05-05 22:42:16 PDT
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.
Comment 7 Build Bot 2016-05-05 23:41:39 PDT
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
Comment 8 Build Bot 2016-05-05 23:41:43 PDT
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
Comment 9 Build Bot 2016-05-05 23:53:46 PDT
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
Comment 10 Build Bot 2016-05-05 23:53:51 PDT
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
Comment 11 Build Bot 2016-05-06 00:23:55 PDT
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
Comment 12 Build Bot 2016-05-06 00:24:01 PDT
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
Comment 13 Timothy Hatcher 2016-05-06 09:22:22 PDT
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.
Comment 14 Mark Lam 2016-05-06 10:38:28 PDT
Are the EWS sadness relevant?
Comment 15 Joseph Pecoraro 2016-05-06 11:56:49 PDT
(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.
Comment 16 Mark Lam 2016-05-06 11:59:31 PDT
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?
Comment 17 Joseph Pecoraro 2016-05-06 11:59:52 PDT
Created attachment 278256 [details]
[PATCH] Proposed Fix
Comment 18 Joseph Pecoraro 2016-05-06 15:43:55 PDT
> > 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.
Comment 19 Joseph Pecoraro 2016-05-06 15:45:39 PDT
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.
Comment 20 Mark Lam 2016-05-06 16:03:19 PDT
Comment on attachment 278282 [details]
[PATCH] Addressed Comments

r=me if perf is unaffected.
Comment 21 Joseph Pecoraro 2016-05-06 16:10:04 PDT
Created attachment 278287 [details]
[TXT] run-jsc-benchmarks results

Results look negligible to me on all the major benchmarks.
Comment 22 WebKit Commit Bot 2016-05-06 16:31:47 PDT
Comment on attachment 278282 [details]
[PATCH] Addressed Comments

Clearing flags on attachment: 278282

Committed r200533: <http://trac.webkit.org/changeset/200533>
Comment 23 WebKit Commit Bot 2016-05-06 16:31:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Joseph Pecoraro 2016-05-06 22:13:24 PDT
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.
Comment 25 Joseph Pecoraro 2016-05-06 22:45:13 PDT
> 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>