Bug 63583 – Web Inspector fails to display source for eval with syntax error

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Juan C. Montemayor]

* STEPS TO REPRODUCE
1. eval("a[0]]");

(Try attached scratch.html in Safari.)

--> You see "Syntax Error: Parse error." in the Web Inspector console, but no source code is displayed.

[Juan C. Montemayor]

Created attachment 99004 [details]
Proposed patch

[Geoffrey Garen]

Comment on attachment 99004 [details]
Proposed patch

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

r- because I think there's a better way to write this test.

> Source/JavaScriptCore/API/tests/testapi.c:1352
> +    exception = NULL;
> +    functionBody = JSStringCreateWithUTF8CString("\neval(a[0]]);");
> +    line = JSStringCreateWithUTF8CString("line");
> +    ASSERT(!JSObjectMakeFunction(context, NULL, 0, NULL, functionBody, NULL, 1, &exception));
> +    ASSERT(JSValueIsObject(context, exception));
> +    v = JSObjectGetProperty(context, JSValueToObject(context, exception, NULL), line, NULL);
> +    assertEqualsAsNumber(v, 2);
> +    JSStringRelease(functionBody);
> +    JSStringRelease(line);

It's better to write this kind of test as a layout test. Layout tests have better reporting. Is there something preventing you from using a layout test in this case?

> Source/JavaScriptCore/parser/Parser.h:68
> +        bool isEvalNode(EvalNode*) { return true; }
> +        bool isEvalNode(ScopeNode*) { return false; }

I know you didn't write the original version of this code, but for future reference, a template function is a clearer way to achieve this goal:

// Declare:
template <typename T> inline bool isEvalNode() { return false; }
template <> inline bool isEvalNode<EvalNode>() { return true; }

// Call:
if (isEvalNode<ParsedNode>()) { ... }

[Juan C. Montemayor]

> It's better to write this kind of test as a layout test. Layout tests have better reporting. Is there something preventing you from using a layout test in this case?

Is something like this ok? 

description(
"This file tests whether a syntax error inside an eval() has the correct line number. That line number should not be the line offset of the error within an eval, but rather the eval itself. "
);

var string = "\n\n\neval(a[0]]);";
try {
	eval(string);
} catch (e) {
	if (e.line == 4)
		successfullyParsed = true;
	else
		successfullyParsed = false;
}

[Geoffrey Garen]

> Is something like this ok? 

Yup!

[Juan C. Montemayor]

Created attachment 99162 [details]
Updated patch

Added a layout test and fixed the issues that Geoff mentioned.

[Geoffrey Garen]

Comment on attachment 99162 [details]
Updated patch

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

This patch is ready to go, but I think it's worth fixing this one last layout test issue.

> LayoutTests/fast/js/script-tests/eval-contained-syntax-error.js:11
> +	if (e.line == 6)
> +		successfullyParsed = true;
> +	else
> +		successfullyParsed = false;

This is kind of an abuse of how "successfullyParsed" is usually used in tests. "successfullyParsed" is only used to indicate whether you made it to the end of the test. To indicate success or failure of the thing you're testing, you want something like:

debug("PASS: e.line should be 6 and is.");

AND

debug("FAIL: e.line should be 6 but instead is " + e.line + ".");

[Juan C. Montemayor]

Created attachment 99174 [details]
updated patch

Fixed the successfullyParsed issue mentioned previously

[WebKit Review Bot]

Comment on attachment 99174 [details]
updated patch

Attachment 99174 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8957499

New failing tests:
fast/js/eval-contained-syntax-error.html

[WebKit Review Bot]

Created attachment 99181 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Juan C. Montemayor]

This is the diff from the test failure in cr-linux:

--- /tmp/layout-test-results/fast/js/eval-contained-syntax-error-expected.txt 
+++ /tmp/layout-test-results/fast/js/eval-contained-syntax-error-actual.txt 
@@ -3,7 +3,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS: e.line should be 6 and is.
+FAIL: e.line should be 6 but instead is undefined.
 PASS successfullyParsed is true
 
 TEST COMPLETE

[Geoffrey Garen]

+try {
+	eval("a[0]]"); //line 6: error should come from here

I think you have tabs instead of spaces here.

\ No newline at end of file

Need a newline.

Unclear why the linux bot is failing. You might want to check whether it has expected results for any line number exceptions on any tests.

[Juan C. Montemayor]

Created attachment 99216 [details]
fixed patch

Fixed tab and also added file that will hopefully not break cr-linux :)

[WebKit Review Bot]

Comment on attachment 99216 [details]
fixed patch

Attachment 99216 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8966273

New failing tests:
fast/js/eval-contained-syntax-error.html

[WebKit Review Bot]

Created attachment 99220 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Juan C. Montemayor]

Created attachment 99323 [details]
test patch

Don't know why cr-linux wants another newline at the end of that file... hopefully this one will go through

[Juan C. Montemayor]

> Don't know why cr-linux wants another newline at the end of that file... hopefully this one will go through

I know why! I checked the diffs and there was a newline missing. My bad.

[Geoffrey Garen]

Comment on attachment 99323 [details]
test patch

r=me

[WebKit Review Bot]

Comment on attachment 99323 [details]
test patch

Clearing flags on attachment: 99323

Committed r90159: <http://trac.webkit.org/changeset/90159>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.