Bug 62613 - No context for javascript parse errors.
: No context for javascript parse errors.
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: InRadar
: 63052
:
  Show dependency treegraph
 
Reported: 2011-06-13 17:20 PST by
Modified: 2012-06-19 13:51 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-06-13 17:20:06 PST
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 From 2011-06-13 17:48:09 PST -------
Created an attachment (id=97047) [details]
proposed patch
------- Comment #2 From 2011-06-13 18:49:59 PST -------
(From update of attachment 97047 [details])
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 From 2011-06-14 11:12:19 PST -------
(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: ']'".
------- Comment #4 From 2011-06-14 11:23:01 PST -------
(In reply to comment #3)
> (From update of attachment 97047 [details] [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 From 2011-06-15 18:09:46 PST -------
Created an attachment (id=97382) [details]
proposed patch
------- Comment #6 From 2011-06-15 18:15:07 PST -------
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 From 2011-06-15 22:39:48 PST -------
(From update of attachment 97382 [details])
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 From 2011-06-16 12:37:08 PST -------
Created an attachment (id=97477) [details]
diff of the fast/js/ tests
------- Comment #9 From 2011-06-16 12:41:01 PST -------
Created an attachment (id=97479) [details]
fast/js tests diff
------- Comment #10 From 2011-06-16 13:50:19 PST -------
Created an attachment (id=97487) [details]
fast/js tests diff
------- Comment #11 From 2011-06-16 14:40:56 PST -------
Created an attachment (id=97503) [details]
fast/js tests diff
------- Comment #12 From 2011-06-16 15:39:04 PST -------
Created an attachment (id=97513) [details]
fast/js tests diff
------- Comment #13 From 2011-06-16 16:47:45 PST -------
Created an attachment (id=97519) [details]
fast/js tests diff
------- Comment #14 From 2011-06-16 17:08:10 PST -------
Created an attachment (id=97522) [details]
proposed patch
------- Comment #15 From 2011-06-16 17:52:41 PST -------
(From update of attachment 97522 [details])
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 From 2011-06-16 18:36:04 PST -------
Created an attachment (id=97535) [details]
hopefully final patch

fixed the whitespace issues and the style errors.

fixed the branching problems
------- Comment #17 From 2011-06-16 19:07:47 PST -------
(From update of attachment 97535 [details])
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 From 2011-06-17 09:19:40 PST -------
Created an attachment (id=97606) [details]
definitely final patch
------- Comment #19 From 2011-06-17 09:30:59 PST -------
(From update of attachment 97606 [details])
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 From 2011-06-17 09:34:57 PST -------
(In reply to comment #19)
> (From update of attachment 97606 [details] [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 From 2011-06-17 18:59:00 PST -------
Created an attachment (id=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 From 2011-06-17 19:01:23 PST -------
Created an attachment (id=97682) [details]
updated patch

fixed a rouge whitespace...
------- Comment #23 From 2011-06-17 19:03:51 PST -------
(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 From 2011-06-17 19:05:21 PST -------
(From update of attachment 97682 [details])
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 From 2011-06-17 19:08:37 PST -------
(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 From 2011-06-17 19:14:09 PST -------
(From update of attachment 97682 [details])
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 From 2011-06-17 19:28:55 PST -------
Created an attachment (id=97684) [details]
updated patch

thought I reverted them all ;)
------- Comment #28 From 2011-06-17 19:36:38 PST -------
Created an attachment (id=97685) [details]
patch

sorry for the spam -- keep finding whitespace... I'm pretty sure it's good now.
------- Comment #29 From 2011-06-19 20:44:52 PST -------
(From update of attachment 97685 [details])
r=me!!!!!
------- Comment #30 From 2011-06-19 20:47:26 PST -------
(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
------- Comment #31 From 2011-06-19 21:13:57 PST -------
(In reply to comment #30)
> (From update of attachment 97685 [details] [details])
> Rejecting attachment 97685 [details] [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 From 2011-06-19 21:14:56 PST -------
CCing eric as webkit-patch should not be producing invalid patches.
------- Comment #33 From 2011-06-19 21:16:28 PST -------
Invalid patches, oh noes!  dbates is our expert on svn-create-patch/svn-apply, etc.
------- Comment #34 From 2011-06-19 23:55:44 PST -------
(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 From 2011-06-20 10:14:57 PST -------
Created an attachment (id=97819) [details]
Patch
------- Comment #36 From 2011-06-20 10:20:31 PST -------
(In reply to comment #35)
> Created an attachment (id=97819) [details] [details]
> Patch

Yay looks like that patch is applying properly.
------- Comment #37 From 2011-06-20 10:49:47 PST -------
(From update of attachment 97819 [details])
Clearing flags on attachment: 97819

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