Last Comment Bug 710192 - Assertion failure: !isIndex(&dummy), at vm/String.h:854
: Assertion failure: !isIndex(&dummy), at vm/String.h:854
js-triage-done [qa+]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- critical (vote)
: mozilla11
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on: 886171
Blocks: langfuzz
  Show dependency treegraph
Reported: 2011-12-13 05:48 PST by Christian Holler (:decoder)
Modified: 2013-06-23 15:27 PDT (History)
9 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch and test (1.27 KB, patch)
2011-12-13 10:16 PST, Jeff Walden [:Waldo] (remove +bmo to email)
evilpies: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2011-12-13 05:48:38 PST
The following test asserts on mozilla-central revision 63bff373cb94 (options -m -n -a):

function f(a, b, c) {
    arguments[('4294967295')] = 2;
assertEq(f(1), "1234");
Comment 1 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-13 10:16:52 PST
Created attachment 581317 [details] [diff] [review]
Patch and test

No real complexity, just undoing a hunk of the patch where I made the inverse of this change...

This sort of thing is exactly why I'm propagating PropertyName-ness from the tokenizer up through the parser and into the bytecode format.  It's too easy to get it wrong when you have a PropertyName and when you have an index.
Comment 2 User image Tom Schuster [:evilpie] 2011-12-13 10:46:03 PST
Comment on attachment 581317 [details] [diff] [review]
Patch and test

Review of attachment 581317 [details] [diff] [review]:

SetName was confusing at first, but it all makes sense now \o/. I think if this function was called SetPropCached or something, the bug would have been more obvious. To fix this for PropertyName we need to consider that we can't easily put the whole uint32_t range into bytecode.
Comment 3 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-15 13:59:06 PST
Comment 4 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-15 15:12:05 PST
Comment on attachment 581317 [details] [diff] [review]
Patch and test

I don't remember whether anyone specifies ops such that this will go wrong in branches, but if they do, this could turn kind of ugly -- crash, mis-cast, and so on.  We should take this on branches.
Comment 5 User image Ed Morley [:emorley] 2011-12-16 06:12:44 PST
Comment 6 User image Alex Keybl [:akeybl] 2011-12-16 12:57:41 PST
Comment on attachment 581317 [details] [diff] [review]
Patch and test

[Triage Comment]
Approved for Aurora. Please land on Monday 12/19 or earlier in order to make the cut-over to Beta.
Comment 7 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-16 13:11:13 PST
Comment 8 User image Paul Silaghi, QA [:pauly] 2012-01-10 06:32:52 PST
Could you please tell me how to test this?
Comment 9 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-01-10 07:38:12 PST
(In reply to Paul Silaghi [QA] from comment #8)
> Could you please tell me how to test this?

You will need to have a build set up with a JS Shell and run the test in the attached patch. If you are unsure and want to learn, please ping one of the QA people on IRC.
Comment 10 User image Mihaela Velimiroviciu (:mihaelav) 2012-01-26 05:34:30 PST
Considering comment #1, the patch comes with a test. Wasn't that test automatically run when the build was created? If it was, isn't that enough to verify the fix?
Thank you!
Comment 11 User image Mihaela Velimiroviciu (:mihaelav) 2012-01-30 00:53:23 PST
Mac OS X 10.6, Ubuntu 11.04, autoconf v2.13

I got the beta source with the latest revision (mozilla-beta-eba00d400eb1) and built the js debug shell. 
After running the testcase from the patch (setprop-with-index.js) no error messages appeared, but the testcase from comment #0 returned "Error: Assertion failed: got (void 0), expected "1234""

Is this expected?

This will still need verification for Windows platform.

Comment 12 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-30 01:28:11 PST
The assertEq in the testcase from comment 0 is a red herring, and proper behavior would be to get that particular assertion-failed message.  The buggy behavior originally reported was that that code was hitting a JS assertion in the engine itself, not in the test.

Honestly, I don't think it's worth spending time manually verifying bugs discovered by fuzzers if the patch that landed includes a testcase.  I'm willing to assume reviewers will make sure the test in the patch is an adequate substitute for the test in the bug.  And a test in a patch, which will be run automatically upon each commit, is a much better way to check for absence of failure than a laborious manual test.
Comment 13 User image Christian Holler (:decoder) 2013-01-19 14:18:56 PST
Automatically extracted testcase for this bug was committed:

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