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");

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 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 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 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.

Could you please tell me how to test this?

(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.

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!

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. Thanks!

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.

Automatically extracted testcase for this bug was committed: https://hg.mozilla.org/mozilla-central/rev/efaf8960a929