Bug 32442 – REGRESSION (r46972): Inspector console outputs the global value for a variable instead of the local value

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Kilzer (:ddkilzer)]

Created attachment 44701 [details]
Test case

* SUMMARY
When breaking inside a function with a local variable that masks a global variable, printing the variable shows the globally-scoped variable instead of the locally-scoped one.

* STEPS TO REPRODUCE
1. Launch Safari or WebKit nightly.
2. Enable JavaScript Debugging in the Web Inspector.
3. Load the test page.
4. Type 'x' in the console and hit Enter.

* EXPECTED RESULTS
The console should print:  "This is the local value"

* ACTUAL RESULTS
This console prints:  "This is the global value"

* REGRESSION
This is a regression from shipping Safari 4.0.4.

[David Kilzer (:ddkilzer)]

The bisect-builds script reports:

Works: r47686  Fails: r48034

Breakpoints don't seem to work in the Inspector between r47686-r48004, so I couldn't narrow it down in that range.

[David Kilzer (:ddkilzer)]

CC-ing some Web Inspector developers.

[Sam Weinig]

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

[David Kilzer (:ddkilzer)]

This appears to be a JavaScriptCore bug, not a Web Inspector bug, since it doesn't reproduce on Google Chrome for Mac 4.0.249.49.

Still occurs with WebKit nightly build r53845.

[Oliver Hunt]

No longer reproduces in the current nightly

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> No longer reproduces in the current nightly

I still see "This is the global value" (which is the broken behavior) with WebKit nightly r54122.

Which nightly are you using, Oliver?

[David Kilzer (:ddkilzer)]

<rdar://problem/7464737>

[Oliver Hunt]

(In reply to comment #6)
> (In reply to comment #5)
> > No longer reproduces in the current nightly
> 
> I still see "This is the global value" (which is the broken behavior) with
> WebKit nightly r54122.
> 
> Which nightly are you using, Oliver?

Wrong bug sorry :-(

[Martin Häcker]

Any progress here? I just spent another day hunting a bug in the wrong direction because of this. :/

Is there any help that you need? (Another testcase reduction perhpas?)

[Dmitry Gorbik]

Created attachment 51845 [details]
Proposed fix v1.0

[Pavel Feldman]

Comment on attachment 51845 [details]
Proposed fix v1.0

Looks great! One nit and we can land this!

> +    if (!dontUseCommandLineAPI) {
> +        expression = "with (window.console._inspectorCommandLineAPI) { with (window) {\n" + expression + "\n} }";
> +    }

Single-line conditions should not have {}.

[Dmitry Gorbik]

Created attachment 51846 [details]
Proposed fix v1.1

[WebKit Commit Bot]

Comment on attachment 51846 [details]
Proposed fix v1.1

Clearing flags on attachment: 51846

Committed r56676: <http://trac.webkit.org/changeset/56676>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

Comment on attachment 51846 [details]
Proposed fix v1.1

>+        Fix the regression caused by r28078: a global variable
>+        definition masks a local one in an inspector console
>+        https://bugs.webkit.org/show_bug.cgi?id=32442

r28078 doesn't seem to the be revision that caused the regression.
<http://trac.webkit.org/changeset/28078>

[David Kilzer (:ddkilzer)]

(In reply to comment #15)
> (From update of attachment 51846 [details])
> >+        Fix the regression caused by r28078: a global variable
> >+        definition masks a local one in an inspector console
> >+        https://bugs.webkit.org/show_bug.cgi?id=32442
> 
> r28078 doesn't seem to the be revision that caused the regression.
> <http://trac.webkit.org/changeset/28078>

Ah, I think you mean Bug 28078 which was fixed by r46972.
<http://trac.webkit.org/changeset/46972>

[David Kilzer (:ddkilzer)]

Nice job on the fix, Dmitry!

Is it possible to write a LayoutTests/inspector test for this so it doesn't happen again?

[Pavel Feldman]

(In reply to comment #17)
> Nice job on the fix, Dmitry!
> 
> Is it possible to write a LayoutTests/inspector test for this so it doesn't
> happen again?

Technically, it is possible. However, we don't yet have layout tests that stop in debugger, so various problems are likely to arise.

Scenario for the test would be:
- In the layout test page, create a function that has debugger; statement. That's pretty much all you do on the layout page side, rest is happening on the inspector front-end.
- On the front-end side, enable debugger (this is done synchronously) and execute the function you created in previous step using InjectedScriptAccess.evaluate.
- Just before you do this evaluate, add a sniffer that would give your code control upon WebInspector.pausedScript call from the backend.
- As soon as pausedScript is called, get a reference to the call frame.
- Either synchronously or in setTimeout(0) execute InjectedScriptAccess.evaluateInCallFrame against your call frame with "x" expression.

tests are located under LayoutTests/inspector