Bug 62613

Created attachment 97047 [details] proposed patch

Comment on attachment 97047 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=97047&action=review The logic for this patch looks fine, however you need to have a slightly more detailed changelog (basically saying what the change you're making is). Normally you'd be expected to have a new testcase for this, but i suspect if you do run-webkit-tests you'll see a large number of failures due to the changed output. Depending on what the changes are we may consider the new test outputs to be sufficient on their own, otherwise you'll need to write a new test specifically. r- due to the test problem. > Source/JavaScriptCore/ChangeLog:7 > + You should have a more detailed description here.

Comment on attachment 97047 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=97047&action=review > Source/JavaScriptCore/parser/JSParser.cpp:156 > + m_errorMessage = UString(String::format("Unexpected token '%c'", source[tokenStart()]).impl()); Is one character of context enough? What does this print when the unexpected token is "return" or "break" or "continue" or "if" or something like that? I think you may need the full token as a string. e.g.: function f() { return if; } // Should be: "Unexpected token: 'if'". return; // Should be: "Unexpected token: 'return'". break; // Should be: "Unexpected token: 'break'". continue; // Should be: "Unexpected token: 'continue'". a[0]]; // I think you have this one right: "Unexpected token: ']'".

(In reply to comment #3) > (From update of attachment 97047 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97047&action=review > > > Source/JavaScriptCore/parser/JSParser.cpp:156 > > + m_errorMessage = UString(String::format("Unexpected token '%c'", source[tokenStart()]).impl()); > > Is one character of context enough? What does this print when the unexpected token is "return" or "break" or "continue" or "if" or something like that? I think you may need the full token as a string. > > e.g.: > > function f() { return if; } // Should be: "Unexpected token: 'if'". > > return; // Should be: "Unexpected token: 'return'". > > break; // Should be: "Unexpected token: 'break'". > > continue; // Should be: "Unexpected token: 'continue'". > > a[0]]; // I think you have this one right: "Unexpected token: ']'". We've discussed other bits in person; JC is going to work on improving the error messages a bit from what the current patch does.

Created attachment 97382 [details] proposed patch

Error messages now have more context, like "Unexpected token: ]" "Expected token: while" "Use of reserved word: super" "Unexpected number: 42" "Unexpected identifier: " "Unexpected string: "foobar"" "Invalid token character sequence: \u4023" "Unexpected EOF" The test 'fast/js/script-tests/parser-syntax-check.js' was fixed to accept the new parser messages. The expected results for the fast/js/ and sputnik tests were updated to reflect the new changes.

Comment on attachment 97382 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=97382&action=review The bulk of the patch is looking good, but still needs a bit of tidying in the logic and structure but not too much (see my comments below), but the error messages need a bit more work. We should just file a bug on the lexer errors and work on that once we've got the parser errors sorted out and landed. > Source/JavaScriptCore/parser/JSParser.cpp:135 > + ALWAYS_INLINE String getToken() { In JSC we use UString rather than String (yes i realise there both essentially RefPtr<StringImpl>, but while a foolish consistency may be the hobgoblin of a little mind, code consistency is awesome!) > Source/JavaScriptCore/parser/JSParser.cpp:184 > + break; Incorrect indenting > Source/JavaScriptCore/parser/JSParser.cpp:328 > + case LastUntaggedToken: > + ASSERT_NOT_REACHED(); > + break; I'd put this one at the end > Source/JavaScriptCore/parser/JSParser.cpp:346 > + case AUTOPLUSPLUS: > + name = "auto ++"; > + break; > + case AUTOMINUSMINUS: > + name = "auto --"; > + break; These are internal concepts, you should just make the above PLUSPLUS logic also handle AUTOPLUSPLUS, eg. case AUTOPLUSPLUS: case PLUSPLUS: ... (and the same for --) > Source/JavaScriptCore/parser/JSParser.cpp:426 > + return name; Given we're just returning name, it would probably be better to replace the name = ...; break; pattern with return ...; And then put ASSERT_NOT_REACHED(); return "internal error"; at the end. > Source/JavaScriptCore/parser/JSParser.cpp:450 > + errorMessage = "Invalid token character sequence: "; I think "Unrecognised token: " but i'm notoriously bad with such things so we may want to ask for input from geoff (in future the lexer should provide a meaningful message) > Source/JavaScriptCore/parser/JSParser.cpp:481 > + m_errorMessage = UString(String::format("Expected token: %s", name).impl()); We may want this message to be: Expected <token that we want> but got <token that we got> instead. > LayoutTests/fast/js/basic-strict-mode-expected.txt:31 > +PASS (function eval(){'use strict';}) threw exception SyntaxError: Unexpected token: }. looking at these i wonder if we want quotes around the symbol tokens? > LayoutTests/fast/js/basic-strict-mode-expected.txt:79 > +PASS 'use strict'; return threw exception SyntaxError: Unexpected token: return. Better error would be "return is only inside of a function" > LayoutTests/fast/js/basic-strict-mode-expected.txt:81 > +PASS 'use strict'; break threw exception SyntaxError: Unexpected token 'break'. > +PASS (function(){'use strict'; break}) threw exception SyntaxError: Unexpected token 'break'. Better error would be "break is only valid inside a switch or loop statement" > LayoutTests/fast/js/basic-strict-mode-expected.txt:83 > +PASS 'use strict'; continue threw exception SyntaxError: Unexpected token 'continue'. > +PASS (function(){'use strict'; continue}) threw exception SyntaxError: Unexpected token 'continue'. Better error would be "continue is only valid inside a loop statement" > LayoutTests/fast/js/basic-strict-mode-expected.txt:88 > +PASS (function(){'use strict'; for(;;)continue missingLabel}) threw exception SyntaxError: Unexpected identifier: missingLabel (in strict mode). Better error here is: "Label '<label name>' is not defined" > LayoutTests/fast/js/basic-strict-mode-expected.txt:94 > +PASS 'use strict'; 007; threw exception SyntaxError: Invalid token character sequence: 007 (in strict mode). > +PASS (function(){'use strict'; 007;}) threw exception SyntaxError: Invalid token character sequence: 007 (in strict mode). > +PASS 'use strict'; '\007'; threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode). > +PASS (function(){'use strict'; '\007';}) threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode). > +PASS '\007'; 'use strict'; threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode). > +PASS (function(){'\007'; 'use strict';}) threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode). These will be fixed when we make the lexer errors better > LayoutTests/fast/js/basic-strict-mode-expected.txt:100 > +PASS 'use strict'; delete aDeletableProperty; threw exception SyntaxError: Unexpected token: ;. > +PASS (function(){'use strict'; delete aDeletableProperty;}) threw exception SyntaxError: Unexpected token: ;. > +PASS 'use strict'; (function (){ delete someDeclaredGlobal;}) threw exception SyntaxError: Unexpected token: ;. > +PASS (function(){'use strict'; (function (){ delete someDeclaredGlobal;})}) threw exception SyntaxError: Unexpected token: ;. > +PASS (function (){ 'use strict'; delete someDeclaredGlobal;}) threw exception SyntaxError: Unexpected token: ;. > +PASS (function(){(function (){ 'use strict'; delete someDeclaredGlobal;})}) threw exception SyntaxError: Unexpected token: ;. These errors aren't very good -- we're saying that the semicolon is unexpected but it's actually strict mode disallowing deletion of an unqualified property > LayoutTests/fast/js/basic-strict-mode-expected.txt:114 > +PASS 'use strict'; ++eval threw exception SyntaxError: Unexpected EOF (in strict mode). > +PASS (function(){'use strict'; ++eval}) threw exception SyntaxError: Unexpected token: }. > +PASS 'use strict'; eval++ threw exception SyntaxError: Unexpected token: ++. > +PASS (function(){'use strict'; eval++}) threw exception SyntaxError: Unexpected token: ++. > +PASS 'use strict'; --eval threw exception SyntaxError: Unexpected EOF (in strict mode). > +PASS (function(){'use strict'; --eval}) threw exception SyntaxError: Unexpected token: }. > +PASS 'use strict'; eval-- threw exception SyntaxError: Unexpected token: --. These errors need to be improved: "Can't assign to eval in strict mode" and "Can't assign to arguments in strict mode"

Created attachment 97477 [details] diff of the fast/js/ tests

Created attachment 97479 [details] fast/js tests diff

Created attachment 97487 [details] fast/js tests diff

Created attachment 97503 [details] fast/js tests diff

Created attachment 97513 [details] fast/js tests diff

Created attachment 97519 [details] fast/js tests diff

Created attachment 97522 [details] proposed patch

Comment on attachment 97522 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=97522&action=review Almost there, just a few style issues remain (i'm only complaining about the other whitespace issues because the style errors will require you to update your patch anyway). The only real problem is the additional branches added for eval/arguments in parseUnaryExpression (see my comments below) Make sure that all the tests that don't fail before this patch still pass after it (i'm surprised that there aren't any non-js/sputnik tests that ended up testing parse error messages). If you use "webkit-patch post" the style checker will run locally and catch any other commit-queue breaking style issues. > Source/JavaScriptCore/parser/JSParser.cpp:85 > + JSParser(Lexer*, JSGlobalData*, FunctionParameters*, bool isStrictContext, bool isFunction, const SourceCode*); Yay style error! 1 space not two (the commit queue barfs on all style errors) > Source/JavaScriptCore/parser/JSParser.cpp:349 > + ALWAYS_INLINE void updateErrorMessageSpecialCase(JSTokenType expectedToken) > + { This function should be doing early returns rather than assign->break->return > Source/JavaScriptCore/parser/JSParser.cpp:425 > + m_errorMessage = UString(msg); This UString conversion should happen automatically > Source/JavaScriptCore/parser/JSParser.cpp:512 > - > + Adding whitespace > Source/JavaScriptCore/parser/JSParser.cpp:933 > + Added whitespace (alas xcode will fight to insert this whitespace :-/ ) > Source/JavaScriptCore/parser/JSParser.cpp:1503 > + failIfFalseIfStrictWithMessage(m_statementDepth == 1, "Function declarations cannot be in a nested block in strict mode"); You should ask geoff if he can come up with a better worded sentence than this -- it's somewhat clunky as is. > Source/JavaScriptCore/parser/JSParser.cpp:1586 > + failIfTrueWithMessage(*name == m_globalData->propertyNames->underscoreProto, "Can't name a function __proto__"); Should use Cannot or Can't consistently -- i'd lean towards Cannot > Source/JavaScriptCore/parser/JSParser.cpp:2386 > - bool isEvalOrArguments = false; > + bool isEval = false; > + bool isArguments = false; > if (strictMode() && !m_syntaxAlreadyValidated) { > if (context.isResolve(expr)) { > - isEvalOrArguments = *m_lastIdentifier == m_globalData->propertyNames->eval || *m_lastIdentifier == m_globalData->propertyNames->arguments; > + isEval = *m_lastIdentifier == m_globalData->propertyNames->eval; > + isArguments = *m_lastIdentifier == m_globalData->propertyNames->arguments; > } > } > - failIfTrueIfStrict(isEvalOrArguments && modifiesExpr); > + failIfTrueIfStrictWithMessage(isEval && modifiesExpr, "'eval' cannot be modified in strict mode"); > + failIfTrueIfStrictWithMessage(isArguments && modifiesExpr, "'arguments' cannot be modified in strict mode"); These changes are unnecessary as you can pull the term 'eval' or 'arguments' out of m_lastIdentifier (we try not to add extra branches to valid code paths of the parser and lexer) > Source/JavaScriptCore/parser/JSParser.cpp:2394 > + failIfTrueIfStrictWithMessage(isEval, "'eval' cannot be modified in strict mode"); > + failIfTrueIfStrictWithMessage(isArguments, "'arguments' cannot be modified in strict mode"); as above > Source/JavaScriptCore/parser/JSParser.cpp:2404 > + failIfTrueIfStrictWithMessage(isEval, "'eval' cannot be modified in strict mode"); > + failIfTrueIfStrictWithMessage(isArguments, "'arguments' cannot be modified in strict mode"); And again > Source/JavaScriptCore/parser/JSParser.h:37 > +class SourceCode; Added whitespace > Source/JavaScriptCore/parser/JSParser.h:141 > + Moar whitespace

Created attachment 97535 [details] hopefully final patch fixed the whitespace issues and the style errors. fixed the branching problems

Comment on attachment 97535 [details] hopefully final patch View in context: https://bugs.webkit.org/attachment.cgi?id=97535&action=review revert the project file change, did you verify all the tests pass now? With that done i believe everything is finished > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:2668 > }; > buildConfigurationList = 149C277108902AFE008A9EFC /* Build configuration list for PBXProject "JavaScriptCore" */; > compatibilityVersion = "Xcode 3.1"; > - developmentRegion = English; > hasScannedForEncodings = 1; > knownRegions = ( > English, Revert your changes to the project file > Source/JavaScriptCore/parser/JSParser.cpp:387 > + errorMessage = "Unexpected EOF"; > + m_errorMessage = errorMessage.impl(); > + return; > + case RETURN: > + errorMessage = "Return statements are only valid inside functions"; > + m_errorMessage = errorMessage.impl(); No reason to do these in two steps

Created attachment 97606 [details] definitely final patch

Comment on attachment 97606 [details] definitely final patch r- as it doesn't look like you've updated all the test results :-( -- a quick scan implies that fast/dom fast/encoding fast/events fast/parser fast/regex html5lib, and the http tests all contain parse errors and i'd be surprised if we're doing anything to actual clobber what the reported error actually is.

(In reply to comment #19) > (From update of attachment 97606 [details]) > r- as it doesn't look like you've updated all the test results :-( oh right.. I was just thinking about fast/js and sputnik. I'll reset every test. > -- a quick scan implies that fast/dom fast/encoding fast/events fast/parser fast/regex html5lib, and the http tests all contain parse errors and i'd be surprised if we're doing anything to actual clobber what the reported error actually is.

Created attachment 97681 [details] updated patch added the new outputs for test cases. Some test case outputs were unchanged (like media-src-allowed) since that test fails regardless of the changes made by this patch.

Created attachment 97682 [details] updated patch fixed a rouge whitespace...

(In reply to comment #21) > Some test case outputs were unchanged (like media-src-allowed) since that test fails regardless of the changes made by this patch. Since these are regression tests, even a failure is checked against the expected output. So if the output changes, we do need to land new expectations. Unless the test is skipped because of an entry in the Skipped file. So I am concerned by what you say above about not updating some test expected results files. For the specific case of media-src-allowed, the expected results don’t seem to include any exceptions, so what you are reporting where that test fails may be a phenomenon limited to your computer.

Comment on attachment 97682 [details] updated patch The EWS is unable to process your patch because something is wrong with the JSParser parts of the patch: patching file Source/JavaScriptCore/parser/JSParser.cpp patch: **** malformed patch at line 48: @@ -123,11 +135,16 @@ private: patching file Source/JavaScriptCore/parser/JSParser.h patch: **** malformed patch at line 29: I’m not sure exactly why that’s failing, but until you deal with that we don’t have any early-warning results for your patch.

(In reply to comment #23) > (In reply to comment #21) > > Some test case outputs were unchanged (like media-src-allowed) since that test fails regardless of the changes made by this patch. > > Since these are regression tests, even a failure is checked against the expected output. So if the output changes, we do need to land new expectations. Unless the test is skipped because of an entry in the Skipped file. > > So I am concerned by what you say above about not updating some test expected results files. > > For the specific case of media-src-allowed, the expected results don’t seem to include any exceptions, so what you are reporting where that test fails may be a phenomenon limited to your computer. This is about tests that fail on his machine locally without his patch -- not about tests where the output has changed

Comment on attachment 97682 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=97682&action=review Noooooooooooooo -- r- due to those tests that have bogus changes (listed below) > LayoutTests/fast/js/cyclic-proto-expected.txt:1 > +posix_spawn fatal error: 9! This looks bogus > LayoutTests/http/tests/media/pdf-served-as-pdf-expected.txt:-5 > +posix_spawn fatal error: 9! > +FAIL: Timed out waiting for notifyDone to be called > EXPECTED (video.error == 'null') OK > EVENT(loadstart) > -EVENT(error) > -failed trying to load PDF file served as PDF OK > -END OF TEST bogus change > LayoutTests/http/tests/media/reload-after-dialog-expected.txt:3 > +posix_spawn fatal error: 9! > +FAIL: Timed out waiting for notifyDone to be called Bogus change

Created attachment 97684 [details] updated patch thought I reverted them all ;)

Created attachment 97685 [details] patch sorry for the spam -- keep finding whitespace... I'm pretty sure it's good now.

Comment on attachment 97685 [details] patch r=me!!!!!

Comment on attachment 97685 [details] patch Rejecting attachment 97685 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2 Last 500 characters of output: le LayoutTests/sputnik/Conformance/13_Function_Definition/S13_A7_T3-expected.txt patching file LayoutTests/sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/15.1.2/15.1.2.1_eval/S15.1.2.1_A2_T2-expected.txt patching file LayoutTests/sputnik/Conformance/15_Native_Objects/15.3_Function/15.3.4/15.3.4.2_Function.prototype.toString/S15.3.4.2_A1_T1-expected.txt Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 2 Full output: http://queues.webkit.org/results/8914343

(In reply to comment #30) > (From update of attachment 97685 [details]) > Rejecting attachment 97685 [details] from commit-queue. > > Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2 > > Last 500 characters of output: > le LayoutTests/sputnik/Conformance/13_Function_Definition/S13_A7_T3-expected.txt > patching file LayoutTests/sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/15.1.2/15.1.2.1_eval/S15.1.2.1_A2_T2-expected.txt > patching file LayoutTests/sputnik/Conformance/15_Native_Objects/15.3_Function/15.3.4/15.3.4.2_Function.prototype.toString/S15.3.4.2_A1_T1-expected.txt > > Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 2 > > Full output: http://queues.webkit.org/results/8914343 I'll have to go over this with you tomorrow as for whatever reason we're getting a bogus patch. Just as an exercise you could try creating a patch and seeing if that will produce a patch that works (eg. put it up in the bug with r?cq?)

CCing eric as webkit-patch should not be producing invalid patches.

Invalid patches, oh noes! dbates is our expert on svn-create-patch/svn-apply, etc.

(In reply to comment #33) > Invalid patches, oh noes! dbates is our expert on svn-create-patch/svn-apply, etc. This patch (attachment 97685 [details]) is messed up. In particular, some of the chunk range lines are wrong. See bug #62965 for more details.

Created attachment 97819 [details] Patch

(In reply to comment #35) > Created an attachment (id=97819) [details] > Patch Yay looks like that patch is applying properly.

Comment on attachment 97819 [details] Patch Clearing flags on attachment: 97819 Committed r89257: <http://trac.webkit.org/changeset/89257>

All reviewed patches have been landed. Closing bug.

It broke two javascriptcore tests on all bots: SL : http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/30548/steps/jscore-test GTK: http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/14871/steps/jscore-test Qt: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/34367/steps/jscore-test :(( Could you fix it, or should we roll it out?

I really don't understand why didn't you guys execute javascript-core-tests and run-webkit-tests and watch the bots after landing. As far as I know, committing regressions is unacceptable, but let me know if this policy is changed.

I won't rollout this regression, but if you would like to do it, just set cq+ here: https://bugs.webkit.org/show_bug.cgi?id=63052

(In reply to comment #40) > I really don't understand why didn't you guys execute javascript-core-tests and run-webkit-tests and watch the bots after landing. As far as I know, committing regressions is unacceptable, but let me know if this policy is changed. We do run tests, I just checked and this seems to be a release build specific crash, even the most rudimentary testing would have shown you this. Of course i recognise that if you're not working on the actual engine you don't encounter issues like this normally.

No need to reopen a bug unless the fix is rolled out

Seeing this posix_spawn fatal error: 9! message on the NRWT bot too: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(NRWT)/r90024%20(214)/fast/js/cyclic-prototypes-pretty-diff.html I wonder what is causing this to bleed into tests.

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

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

<rdar://problem/5907896>