Bug 62613 - No context for javascript parse errors.
: No context for javascript parse errors.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
: InRadar
Depends on: 63052
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-13 17:20 PDT by Juan C. Montemayor
Modified: 2012-06-19 13:51 PDT (History)
9 users (show)

See Also:


Attachments
proposed patch (7.11 KB, patch)
2011-06-13 17:48 PDT, Juan C. Montemayor
oliver: review-
oliver: commit‑queue-
Details | Formatted Diff | Diff
proposed patch (323.95 KB, patch)
2011-06-15 18:09 PDT, Juan C. Montemayor
oliver: review-
oliver: commit‑queue-
Details | Formatted Diff | Diff
diff of the fast/js/ tests (262.80 KB, patch)
2011-06-16 12:37 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
fast/js tests diff (264.15 KB, patch)
2011-06-16 12:41 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
fast/js tests diff (266.71 KB, patch)
2011-06-16 13:50 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
fast/js tests diff (267.15 KB, patch)
2011-06-16 14:40 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
fast/js tests diff (268.09 KB, patch)
2011-06-16 15:39 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
fast/js tests diff (268.79 KB, patch)
2011-06-16 16:47 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
proposed patch (339.63 KB, patch)
2011-06-16 17:08 PDT, Juan C. Montemayor
oliver: review-
oliver: commit‑queue-
Details | Formatted Diff | Diff
hopefully final patch (339.48 KB, patch)
2011-06-16 18:36 PDT, Juan C. Montemayor
oliver: review-
oliver: commit‑queue-
Details | Formatted Diff | Diff
definitely final patch (338.75 KB, patch)
2011-06-17 09:19 PDT, Juan C. Montemayor
oliver: review-
oliver: commit‑queue-
Details | Formatted Diff | Diff
updated patch (353.29 KB, patch)
2011-06-17 18:59 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
updated patch (353.29 KB, patch)
2011-06-17 19:01 PDT, Juan C. Montemayor
oliver: review-
oliver: commit‑queue-
Details | Formatted Diff | Diff
updated patch (351.28 KB, patch)
2011-06-17 19:28 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
patch (351.26 KB, patch)
2011-06-17 19:36 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
Patch (351.04 KB, patch)
2011-06-20 10:14 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Juan C. Montemayor 2011-06-13 17:20:06 PDT
For parse errors in javascript files, the Web Inspector prints out the message: "SyntaxError: Parse Error" which is not very useful.

What we want here is some more context for parse errors, of this form:
    SyntaxError: Unexpected token: 'TOKEN'
Comment 1 Juan C. Montemayor 2011-06-13 17:48:09 PDT
Created attachment 97047 [details]
proposed patch
Comment 2 Oliver Hunt 2011-06-13 18:49:59 PDT
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 3 Geoffrey Garen 2011-06-14 11:12:19 PDT
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: ']'".
Comment 4 Oliver Hunt 2011-06-14 11:23:01 PDT
(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.
Comment 5 Juan C. Montemayor 2011-06-15 18:09:46 PDT
Created attachment 97382 [details]
proposed patch
Comment 6 Juan C. Montemayor 2011-06-15 18:15:07 PDT
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 7 Oliver Hunt 2011-06-15 22:39:48 PDT
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"
Comment 8 Juan C. Montemayor 2011-06-16 12:37:08 PDT
Created attachment 97477 [details]
diff of the fast/js/ tests
Comment 9 Juan C. Montemayor 2011-06-16 12:41:01 PDT
Created attachment 97479 [details]
fast/js tests diff
Comment 10 Juan C. Montemayor 2011-06-16 13:50:19 PDT
Created attachment 97487 [details]
fast/js tests diff
Comment 11 Juan C. Montemayor 2011-06-16 14:40:56 PDT
Created attachment 97503 [details]
fast/js tests diff
Comment 12 Juan C. Montemayor 2011-06-16 15:39:04 PDT
Created attachment 97513 [details]
fast/js tests diff
Comment 13 Juan C. Montemayor 2011-06-16 16:47:45 PDT
Created attachment 97519 [details]
fast/js tests diff
Comment 14 Juan C. Montemayor 2011-06-16 17:08:10 PDT
Created attachment 97522 [details]
proposed patch
Comment 15 Oliver Hunt 2011-06-16 17:52:41 PDT
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
Comment 16 Juan C. Montemayor 2011-06-16 18:36:04 PDT
Created attachment 97535 [details]
hopefully final patch

fixed the whitespace issues and the style errors.

fixed the branching problems
Comment 17 Oliver Hunt 2011-06-16 19:07:47 PDT
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
Comment 18 Juan C. Montemayor 2011-06-17 09:19:40 PDT
Created attachment 97606 [details]
definitely final patch
Comment 19 Oliver Hunt 2011-06-17 09:30:59 PDT
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.
Comment 20 Juan C. Montemayor 2011-06-17 09:34:57 PDT
(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.
Comment 21 Juan C. Montemayor 2011-06-17 18:59:00 PDT
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.
Comment 22 Juan C. Montemayor 2011-06-17 19:01:23 PDT
Created attachment 97682 [details]
updated patch

fixed a rouge whitespace...
Comment 23 Darin Adler 2011-06-17 19:03:51 PDT
(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 24 Darin Adler 2011-06-17 19:05:21 PDT
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.
Comment 25 Oliver Hunt 2011-06-17 19:08:37 PDT
(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 26 Oliver Hunt 2011-06-17 19:14:09 PDT
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
Comment 27 Juan C. Montemayor 2011-06-17 19:28:55 PDT
Created attachment 97684 [details]
updated patch

thought I reverted them all ;)
Comment 28 Juan C. Montemayor 2011-06-17 19:36:38 PDT
Created attachment 97685 [details]
patch

sorry for the spam -- keep finding whitespace... I'm pretty sure it's good now.
Comment 29 Oliver Hunt 2011-06-19 20:44:52 PDT
Comment on attachment 97685 [details]
patch

r=me!!!!!
Comment 30 WebKit Review Bot 2011-06-19 20:47:26 PDT
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
Comment 31 Oliver Hunt 2011-06-19 21:13:57 PDT
(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?)
Comment 32 Oliver Hunt 2011-06-19 21:14:56 PDT
CCing eric as webkit-patch should not be producing invalid patches.
Comment 33 Eric Seidel 2011-06-19 21:16:28 PDT
Invalid patches, oh noes!  dbates is our expert on svn-create-patch/svn-apply, etc.
Comment 34 Daniel Bates 2011-06-19 23:55:44 PDT
(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.
Comment 35 Juan C. Montemayor 2011-06-20 10:14:57 PDT
Created attachment 97819 [details]
Patch
Comment 36 Oliver Hunt 2011-06-20 10:20:31 PDT
(In reply to comment #35)
> Created an attachment (id=97819) [details]
> Patch

Yay looks like that patch is applying properly.
Comment 37 Oliver Hunt 2011-06-20 10:49:47 PDT
Comment on attachment 97819 [details]
Patch

Clearing flags on attachment: 97819

Committed r89257: <http://trac.webkit.org/changeset/89257>
Comment 38 Oliver Hunt 2011-06-20 10:49:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Csaba Osztrogonác 2011-06-21 00:23:53 PDT
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.
Comment 41 Csaba Osztrogonác 2011-06-21 00:31:03 PDT
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
Comment 42 Oliver Hunt 2011-06-21 10:50:22 PDT
(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.
Comment 43 Oliver Hunt 2011-06-21 18:46:38 PDT
No need to reopen a bug unless the fix is rolled out
Comment 44 Eric Seidel 2011-06-29 13:28:28 PDT
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.
Comment 45 Gavin Barraclough 2011-08-07 23:42:12 PDT
*** Bug 25630 has been marked as a duplicate of this bug. ***
Comment 46 Gavin Barraclough 2011-08-12 01:10:36 PDT
*** Bug 42181 has been marked as a duplicate of this bug. ***
Comment 47 David Kilzer (:ddkilzer) 2012-06-19 13:51:57 PDT
<rdar://problem/5907896>