Bug 65829

Summary: JavaScript console can't evaluate expression in strict context
Product: WebKit Reporter: Mark S. Miller <erights>
Component: Web InspectorAssignee: Timothy Hatcher <timothy>
Status: RESOLVED FIXED    
Severity: Major CC: barraclough, bburg, buildbot, ddkilzer, dglazkov, erights, ggaren, graouts, joepeck, mark.lam, matt.cg, mdeschamps, o.smestad, rniwa, timothy, tomi.kyostila, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Macintosh Intel   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 113291    
Attachments:
Description Flags
Load and follow instructions to see JavaScript console bug
none
What I see when I follow the instructions on previous attachment
none
Demonstrates that the bug also occurs on the released Safari 5.0.6
none
the latest error message and traceback from the attached test case (Webkit r108593)
none
Proposed Change
ggaren: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-11
none
Archive of layout-test-results from gce-cr-linux-06
none
Archive of layout-test-results from webkit-ews-04
none
Proposed Change (Take 2)
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64
none
Proposed Change (Take 3)
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-05 for mac-future
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Proposed Change (Take 4)
none
The bug still happens on the latest Nightly (part 1)
none
The bug still happens on the latest Nightly (part 2)
none
Screenshot showing that the bug isn't fixed -- Part 1
none
Screenshot showing that the bug isn't fixed -- Part 2
none
Proposed Additional Change joepeck: review+, timothy: commit-queue-

Description Mark S. Miller 2011-08-07 13:28:47 PDT
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.
Comment 1 Mark S. Miller 2011-08-07 13:31:45 PDT
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.
Comment 2 Mark S. Miller 2011-08-07 13:34:28 PDT
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.
Comment 3 Mark S. Miller 2011-08-07 13:42:14 PDT
Created attachment 103186 [details]
Demonstrates that the bug also occurs on the released Safari 5.0.6
Comment 4 David Baumgold 2012-02-23 11:13:28 PST
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).
Comment 5 David Baumgold 2012-02-23 11:16:52 PST
Created attachment 128509 [details]
the latest error message and traceback from the attached test case (Webkit r108593)
Comment 6 Øyvind Smestad 2012-12-10 04:22:49 PST
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)
Comment 7 Timothy Hatcher 2013-03-21 15:03:53 PDT
*** Bug 83267 has been marked as a duplicate of this bug. ***
Comment 8 Timothy Hatcher 2013-03-21 15:17:54 PDT
Created attachment 194361 [details]
Proposed Change
Comment 9 Geoffrey Garen 2013-03-21 15:21:51 PDT
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.
Comment 10 Geoffrey Garen 2013-03-21 15:36:46 PDT
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.
Comment 11 Build Bot 2013-03-21 15:57:45 PDT
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
Comment 12 Build Bot 2013-03-21 15:57:47 PDT
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
Comment 13 WebKit Review Bot 2013-03-21 16:11:22 PDT
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
Comment 14 WebKit Review Bot 2013-03-21 16:11:26 PDT
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
Comment 15 Build Bot 2013-03-21 23:39:11 PDT
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
Comment 16 Build Bot 2013-03-21 23:39:14 PDT
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
Comment 17 Timothy Hatcher 2013-03-23 07:58:42 PDT
I have a better patch coming soon.
Comment 18 Timothy Hatcher 2013-03-23 14:32:03 PDT
Created attachment 194718 [details]
Proposed Change (Take 2)
Comment 19 WebKit Review Bot 2013-03-23 18:16:02 PDT
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
Comment 20 WebKit Review Bot 2013-03-23 18:16:07 PDT
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
Comment 21 Timothy Hatcher 2013-03-23 18:26:27 PDT
Created attachment 194726 [details]
Proposed Change (Take 3)
Comment 22 Build Bot 2013-03-23 21:11:24 PDT
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
Comment 23 Build Bot 2013-03-23 21:11:27 PDT
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
Comment 24 Build Bot 2013-03-23 21:21:25 PDT
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
Comment 25 Build Bot 2013-03-23 21:21:30 PDT
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
Comment 26 Timothy Hatcher 2013-03-24 08:00:30 PDT
Created attachment 194752 [details]
Proposed Change (Take 4)
Comment 27 Pavel Feldman 2013-03-25 06:48:54 PDT
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.
Comment 28 Timothy Hatcher 2013-03-25 08:49:18 PDT
(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.
Comment 29 WebKit Review Bot 2013-03-25 19:38:24 PDT
Comment on attachment 194752 [details]
Proposed Change (Take 4)

Clearing flags on attachment: 194752

Committed r146840: <http://trac.webkit.org/changeset/146840>
Comment 30 WebKit Review Bot 2013-03-25 19:38:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Mark S. Miller 2013-03-26 00:09:01 PDT
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.
Comment 32 Mark S. Miller 2013-03-26 00:10:41 PDT
Created attachment 195020 [details]
The bug still happens on the latest Nightly (part 2)
Comment 33 Timothy Hatcher 2013-03-26 00:13:04 PDT
WebKit r146818 was built on 25 March 2013 and is a 47.5 MB download.

This landed in r146840. Next build will have it.
Comment 34 Timothy Hatcher 2013-03-26 05:50:35 PDT
WebKit r146855 was built on 26 March 2013 and is a 47.4 MB download.

Give it a try now.
Comment 35 Mark S. Miller 2013-03-26 12:15:58 PDT
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
Comment 36 Mark S. Miller 2013-03-26 12:16:37 PDT
Created attachment 195139 [details]
Screenshot showing that the bug isn't fixed -- Part 2
Comment 37 Timothy Hatcher 2013-03-26 13:04:01 PDT
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.
Comment 38 Timothy Hatcher 2013-03-26 13:05:21 PDT
Reopening.
Comment 39 Timothy Hatcher 2013-03-26 13:48:41 PDT
Created attachment 195156 [details]
Proposed Additional Change
Comment 40 Joseph Pecoraro 2013-03-26 13:57:29 PDT
Comment on attachment 195156 [details]
Proposed Additional Change

r=me
Comment 41 Timothy Hatcher 2013-03-26 15:10:06 PDT
Comment on attachment 195156 [details]
Proposed Additional Change

https://trac.webkit.org/r146937
Comment 42 Brian Burg 2014-01-25 15:43:55 PST
Migrating to new component. The patch has undoubtedly bitrotted.
Comment 43 Timothy Hatcher 2014-01-25 16:54:13 PST
This has been fixed in TOT and Safari 7 with a different approach.