Bug 65829 – JavaScript console can't evaluate expression in strict context

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

[Mark S. Miller]

In WebKit Nightly Version 5.0.6 (5533.22.3, r92569), open the attachment and follow the instructions.

The next attachment will show what I see when I do so.

[Mark S. Miller]

Created attachment 103184 [details]
Load and follow instructions to see JavaScript console bug

This attachment didn't take the first time, so adding it now.

[Mark S. Miller]

Created attachment 103185 [details]
What I see when I follow the instructions on previous attachment

This screenshot seems to demonstrate that, when the JavaScript console is in a strict context and tries to evaluate an expression, it first surrounds the expression with a "with" statement and then tries to evaluate the resulting string in a strict context. This of course cannot work, since "with" is illegal in strict mode.

[Mark S. Miller]

Created attachment 103186 [details]
Demonstrates that the bug also occurs on the released Safari 5.0.6

[David Baumgold]

Confirmed that this bug is still occuring in the latest version of Safari (Version 5.1.3 (7534.53.10)) and in the latest Webkit Nightly build (r108593).

[David Baumgold]

Created attachment 128509 [details]
the latest error message and traceback from the attached test case (Webkit r108593)

[Øyvind Smestad]

This issue seems to be duplicated in: https://bugs.webkit.org/show_bug.cgi?id=83267 and still occurs in Safari 6.0.2 (8536.26.17)

[Timothy Hatcher]

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

[Timothy Hatcher]

Created attachment 194361 [details]
Proposed Change

[Geoffrey Garen]

Comment on attachment 194361 [details]
Proposed Change

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

r=me

> Source/WebCore/ChangeLog:14
> +        console to stay in strict mode. This also allows users to opt-into strict mode by prefixing

s/stay in/stay out of/ -- we're evaluating inside a strict mode function, but then injecting a non-strict function.

[Geoffrey Garen]

Comment on attachment 194361 [details]
Proposed Change

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

> Source/WebCore/inspector/InjectedScriptSource.js:584
>              var result = evalFunction.call(object, expression);

I think we should surround this case in an empty function expression. That way, the behavior of things like "var" won't change.

[Build Bot]

Comment on attachment 194361 [details]
Proposed Change

Attachment 194361 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17290116

New failing tests:
inspector/debugger/debugger-pause-in-eval-script.html
inspector/debugger/mutation-observer-suspend-while-paused.html
inspector/console/console-eval-syntax-error.html
inspector/console/console-eval-fake.html
svg/custom/marker-orient-auto.html
inspector/extensions/extensions-sidebar.html
inspector/extensions/extensions-eval.html
inspector/console/command-line-api.html

[Build Bot]

Created attachment 194368 [details]
Archive of layout-test-results from webkit-ews-11

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: <class 'webkitpy.common.config.ports.MacWK2Port'>  Platform: Mac OS X 10.8.2

[WebKit Review Bot]

Comment on attachment 194361 [details]
Proposed Change

Attachment 194361 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17207558

New failing tests:
inspector/debugger/debugger-pause-in-eval-script.html
http/tests/inspector/console-cd-completions.html
inspector/debugger/mutation-observer-suspend-while-paused.html
inspector/console/console-eval-syntax-error.html
inspector/console/console-eval-fake.html
inspector/extensions/extensions-sidebar.html
inspector/extensions/extensions-eval.html
inspector/console/command-line-api.html

[WebKit Review Bot]

Created attachment 194373 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Build Bot]

Comment on attachment 194361 [details]
Proposed Change

Attachment 194361 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17112655

New failing tests:
inspector/debugger/debugger-pause-in-eval-script.html
inspector/debugger/mutation-observer-suspend-while-paused.html
inspector/console/console-eval-syntax-error.html
inspector/console/console-eval-fake.html
inspector/extensions/extensions-sidebar.html
inspector/extensions/extensions-eval.html
inspector/console/command-line-api.html

[Build Bot]

Created attachment 194454 [details]
Archive of layout-test-results from webkit-ews-04

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: <class 'webkitpy.common.config.ports.MacPort'>  Platform: Mac OS X 10.8.2

[Timothy Hatcher]

I have a better patch coming soon.

[Timothy Hatcher]

Created attachment 194718 [details]
Proposed Change (Take 2)

[WebKit Review Bot]

Comment on attachment 194718 [details]
Proposed Change (Take 2)

Attachment 194718 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17235424

New failing tests:
inspector/console/command-line-api.html

[WebKit Review Bot]

Created attachment 194725 [details]
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Timothy Hatcher]

Created attachment 194726 [details]
Proposed Change (Take 3)

[Build Bot]

Comment on attachment 194726 [details]
Proposed Change (Take 3)

Attachment 194726 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17311055

New failing tests:
http/tests/inspector/console-resource-errors.html

[Build Bot]

Created attachment 194733 [details]
Archive of layout-test-results from webkit-ews-05 for mac-future

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

[Build Bot]

Comment on attachment 194726 [details]
Proposed Change (Take 3)

Attachment 194726 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17304056

New failing tests:
http/tests/inspector/console-resource-errors.html

[Build Bot]

Created attachment 194734 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

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

[Timothy Hatcher]

Created attachment 194752 [details]
Proposed Change (Take 4)

[Pavel Feldman]

Comment on attachment 194752 [details]
Proposed Change (Take 4)

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

> Source/WebCore/inspector/InjectedScriptHost.h:109
> +    bool evaluateReturnsEvalFunction() { return true; }

So far we were hiding all the differences like this behind Script* boundary. Also, could we instead introduce evaluateFunction getter that v8 will fail to return instead of this boolean flag? That way InjectedScriptSource will be compiler friendly.

[Timothy Hatcher]

(In reply to comment #27)
> (From update of attachment 194752 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194752&action=review
> 
> > Source/WebCore/inspector/InjectedScriptHost.h:109
> > +    bool evaluateReturnsEvalFunction() { return true; }
> 
> So far we were hiding all the differences like this behind Script* boundary. Also, could we instead introduce evaluateFunction getter that v8 will fail to return instead of this boolean flag? That way InjectedScriptSource will be compiler friendly.

How is this not patch not compiler friendly?

I considered exposing evaluateFunction. But I didn't want both evaluate and evaluateFunction in the JSC path. We want to have only one way to evaluate.

[WebKit Review Bot]

Comment on attachment 194752 [details]
Proposed Change (Take 4)

Clearing flags on attachment: 194752

Committed r146840: <http://trac.webkit.org/changeset/146840>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark S. Miller]

Created attachment 195019 [details]
The bug still happens on the latest Nightly (part 1)

I see that this bug is closed. But the attached screenshot shows that the latest Nightly still suffers from this bug.

[Mark S. Miller]

Created attachment 195020 [details]
The bug still happens on the latest Nightly (part 2)

[Timothy Hatcher]

WebKit r146818 was built on 25 March 2013 and is a 47.5 MB download.

This landed in r146840. Next build will have it.

[Timothy Hatcher]

WebKit r146855 was built on 26 March 2013 and is a 47.4 MB download.

Give it a try now.

[Mark S. Miller]

Created attachment 195138 [details]
Screenshot showing that the bug isn't fixed -- Part 1

(In reply to comment #34)
> WebKit r146855 was built on 26 March 2013 and is a 47.4 MB download.
> 
> Give it a try now.

I just updated to the latest Nightly, which self-identifies as r146901  and  6.0.3 (8536.28.10, 537+). Attached is the first screenshot demonstrating that the bug still isn't fixed, but manifests differently. Second screenshot in next attachment

[Mark S. Miller]

Created attachment 195139 [details]
Screenshot showing that the bug isn't fixed -- Part 2

[Timothy Hatcher]

Thanks Mark. Apparently my strict mode test stopped triggering strict mode due to a bug — and I totally missed this.

I have a fix coming right up.

[Timothy Hatcher]

Reopening.

[Timothy Hatcher]

Created attachment 195156 [details]
Proposed Additional Change

[Joseph Pecoraro]

Comment on attachment 195156 [details]
Proposed Additional Change

r=me

[Timothy Hatcher]

Comment on attachment 195156 [details]
Proposed Additional Change

https://trac.webkit.org/r146937

[Blaze Burg]

Migrating to new component. The patch has undoubtedly bitrotted.

[Timothy Hatcher]

This has been fixed in TOT and Safari 7 with a different approach.