WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
63583
Web Inspector fails to display source for eval with syntax error
https://bugs.webkit.org/show_bug.cgi?id=63583
Summary
Web Inspector fails to display source for eval with syntax error
Juan C. Montemayor
Reported
2011-06-28 16:30:24 PDT
* 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.
Attachments
Proposed patch
(2.91 KB, patch)
2011-06-28 17:02 PDT
,
Juan C. Montemayor
no flags
Details
Formatted Diff
Diff
Updated patch
(4.63 KB, patch)
2011-06-29 15:39 PDT
,
Juan C. Montemayor
no flags
Details
Formatted Diff
Diff
updated patch
(4.78 KB, patch)
2011-06-29 16:22 PDT
,
Juan C. Montemayor
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-02
(1.38 MB, application/zip)
2011-06-29 16:40 PDT
,
WebKit Review Bot
no flags
Details
fixed patch
(5.62 KB, patch)
2011-06-29 19:31 PDT
,
Juan C. Montemayor
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-02
(1.54 MB, application/zip)
2011-06-29 19:52 PDT
,
WebKit Review Bot
no flags
Details
test patch
(5.62 KB, patch)
2011-06-30 10:10 PDT
,
Juan C. Montemayor
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Juan C. Montemayor
Comment 1
2011-06-28 17:02:24 PDT
Created
attachment 99004
[details]
Proposed patch
Geoffrey Garen
Comment 2
2011-06-28 20:55:02 PDT
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
Comment 3
2011-06-29 12:42:29 PDT
> 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
Comment 4
2011-06-29 12:51:21 PDT
> Is something like this ok?
Yup!
Juan C. Montemayor
Comment 5
2011-06-29 15:39:03 PDT
Created
attachment 99162
[details]
Updated patch Added a layout test and fixed the issues that Geoff mentioned.
Geoffrey Garen
Comment 6
2011-06-29 15:52:13 PDT
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
Comment 7
2011-06-29 16:22:14 PDT
Created
attachment 99174
[details]
updated patch Fixed the successfullyParsed issue mentioned previously
WebKit Review Bot
Comment 8
2011-06-29 16:40:48 PDT
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
Comment 9
2011-06-29 16:40:54 PDT
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
Comment 10
2011-06-29 16:47:23 PDT
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
Comment 11
2011-06-29 16:55:56 PDT
+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
Comment 12
2011-06-29 19:31:17 PDT
Created
attachment 99216
[details]
fixed patch Fixed tab and also added file that will hopefully not break cr-linux :)
WebKit Review Bot
Comment 13
2011-06-29 19:52:32 PDT
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
Comment 14
2011-06-29 19:52:38 PDT
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
Comment 15
2011-06-30 10:10:00 PDT
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
Comment 16
2011-06-30 10:12:06 PDT
> 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 17
2011-06-30 13:58:36 PDT
Comment on
attachment 99323
[details]
test patch r=me
WebKit Review Bot
Comment 18
2011-06-30 14:42:10 PDT
Comment on
attachment 99323
[details]
test patch Clearing flags on attachment: 99323 Committed
r90159
: <
http://trac.webkit.org/changeset/90159
>
WebKit Review Bot
Comment 19
2011-06-30 14:42:21 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug