Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Assertion failure: !isIndex(&dummy), at vm/String.h:854

RESOLVED FIXED in Firefox 10

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: decoder, Assigned: Waldo)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla11
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox10 fixed)

Details

(Whiteboard: js-triage-done [qa+])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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");
Assignee: general → jwalden+bmo
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.
Attachment #581317 - Flags: review?(evilpies)
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.
Attachment #581317 - Flags: review?(evilpies) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2901886245e3
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: js-triage-needed → js-triage-done
Target Milestone: --- → mozilla11
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.
Attachment #581317 - Flags: approval-mozilla-aurora?

Comment 5

6 years ago
https://hg.mozilla.org/mozilla-central/rev/2901886245e3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 6

6 years ago
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.
Attachment #581317 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ff54f9f7888
status-firefox10: --- → fixed
Whiteboard: js-triage-done → js-triage-done [qa+]
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.
(Reporter)

Comment 13

5 years ago
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
Depends on: 886171
You need to log in before you can comment on or make changes to this bug.