Bug 63951 – ASSERT_NOT_REACHED running test 262

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

This is in JSParser::updateErrorMessageSpecialCase.

[Juan C. Montemayor]

Do you know which specific test(s) fail because of the assert?

[Juan C. Montemayor]

Created attachment 99851 [details]
patch

[Gavin Barraclough]

Comment on attachment 99851 [details]
patch

Ah! I should have guessed it was this!  Cheers for the fix JC.  Do you have commit privileges? - or shall I cq+?

[Juan C. Montemayor]

(In reply to comment #3)
> (From update of attachment 99851 [details])
> Ah! I should have guessed it was this!  Cheers for the fix JC.  Do you have commit privileges? - or shall I cq+?

Not yet ;) so you should cq+

[Juan C. Montemayor]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 99851 [details] [details])
> > Ah! I should have guessed it was this!  Cheers for the fix JC.  Do you have commit privileges? - or shall I cq+?
> 
> Not yet ;) so you should cq+

...but wait until I make sure this patch wont fail any tests... I was focused on 262 so I forgot about the webkit tests.

[Gavin Barraclough]

Comment on attachment 99851 [details]
patch

Actually, I gave a bad review here.  We really should have a regression test covering this, if possible.  Could you look into this?  We should be able to add something to a regression test to check this syntax error is thrown correctly.  Could you try to produce a simple test that triggers the bug, and add to a layout test?  (LayoutTests/fast/js/reserved-words might be a candidate if this is reserved-words specific, or there may be a existing syntax-errors related test, or you can add a new test case).  Hope that sounds reasonable.

[Juan C. Montemayor]

(In reply to comment #6)
> (From update of attachment 99851 [details])
> Actually, I gave a bad review here.  We really should have a regression test covering this, if possible.  Could you look into this?  We should be able to add something to a regression test to check this syntax error is thrown correctly.  Could you try to produce a simple test that triggers the bug, and add to a layout test?  (LayoutTests/fast/js/reserved-words might be a candidate if this is reserved-words specific, or there may be a existing syntax-errors related test, or you can add a new test case).  Hope that sounds reasonable.

On it!

[Juan C. Montemayor]

Created attachment 99892 [details]
updated patch

Layout test actually exposed a logic bug that is fixed with this patch.

[WebKit Review Bot]

Attachment 99892 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 9

Updating OpenSource
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': timed out waiting for server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 146.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Juan C. Montemayor]

(In reply to comment #9)
> Attachment 99892 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 9
> 

I ran check-webkit-style locally and it passed with no errors.

[WebKit Review Bot]

Comment on attachment 99892 [details]
updated patch

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

New failing tests:
fast/js/reserved-words-strict.html

[WebKit Review Bot]

Created attachment 99899 [details]
Archive of layout-test-results from ec2-cr-linux-01

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

[Juan C. Montemayor]

Created attachment 99903 [details]
updated patch

Hopefully will work with chromium now

[Gavin Barraclough]

Comment on attachment 99903 [details]
updated patch

Looks good to me, cheers JC.

[WebKit Review Bot]

Comment on attachment 99903 [details]
updated patch

Clearing flags on attachment: 99903

Committed r90535: <http://trac.webkit.org/changeset/90535>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.