Last Comment Bug 722023 - Assertion failure: !isIndex(&dummy), at js/src/vm/String.h:857
: Assertion failure: !isIndex(&dummy), at js/src/vm/String.h:857
Status: RESOLVED FIXED
[js-triage-done]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla12
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 532972 langfuzz
  Show dependency treegraph
 
Reported: 2012-01-28 02:45 PST by Christian Holler (:decoder)
Modified: 2013-01-19 13:56 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch with test (1.32 KB, patch)
2012-01-29 12:31 PST, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-01-28 02:45:10 PST
The following test asserts on mozilla-central revision 8a59519e137e (options -m -n -a, 32 bit):


var obj = new Object();
var index = [ (null ), 1073741824, 1073741825 ];
for (var j in index) { 
  testProperty(index[j]);
}
function testProperty(i) { 
  actual = obj[String(i)];
}


This bug pops up way too often during fuzzing (and I thought it was already filed/fixed because the same assert was there some weeks ago already), would be good if someone could fix it :D
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-28 04:09:57 PST
This assertion's at a pinch-point, so you have to look at the caller location to bucket instances of it.  And if you hit it, please do report it quickly.  When the actual property storage split happens, this assertion will become significantly more worrisome if hit.  Right now, because everything still uses the same property store, an atom that's not a PropertyName is somewhat less likely to indicate substantial cause for alarm, in terms of the failure mode afterward in a release build.

You're likely to find more of this assertion when fuzzing.  Prior code didn't make the distinction PropertyName is meant to enforce, and it's not always easy to make it because the values in question flow in from only semi-typed sources (like immediates in bytecode).  There's also the problem of conflation of concepts here amongst jsid, JSAtom*, and integer-valued jsids.  Here the problem lies with this code and jsid/atom:

(gdb) up
#4  0x00000000007da90c in js::mjit::ic::GetElementIC::update (this=0xbe25c0, f=..., obj=0x7ffff150f740, v=..., id=..., vp=0x7ffff1741138)
    at /home/jwalden/moz/slots/js/src/methodjit/PolyIC.cpp:2611
2611	        return attachGetProp(f, obj, v, JSID_TO_ATOM(id)->asPropertyName(), vp);
(gdb) lis
2606	     * GETPROP IC assumes the id has already gone through filtering for string
2607	     * indexes in the emitter, i.e. js_GetProtoIfDenseArray is only valid to
2608	     * use when looking up non-integer identifiers.
2609	     */
2610	    if (v.isString() && js_CheckForStringIndex(id) == id)
2611	        return attachGetProp(f, obj, v, JSID_TO_ATOM(id)->asPropertyName(), vp);
...
(gdb) p js_DumpAtom(this)
JSAtom* (0x7ffff1627f40) = JSString* (0x7ffff1627f40) = jschar * (0x7ffff1627f50) = "1073741825"

The reference to "string indexes" refers to the distinction jsid makes, which says -2**31 to 2**31 - 1 (possibly with some exponent off-by-ones or something) is a special kind of id.  But "index" in the context of PropertyName means 0 to 2**32 - 1.  You can see how this is more than a bit confusing.

The fix here probably involves reverting some portions of this change, until the actual split happens a bit more:

changeset:   83717:d0e3133d19e2
user:        Jeff Walden <jwalden@mit.edu>
date:        Tue Dec 27 02:27:02 2011 -0600
summary:     Bug 713183 - Make JSOP_*PROP and JSOP_*NAME store a PropertyName immediate, not a JSAtom immediate, and take advantage of this fact.  r=bhackett
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-28 23:28:35 PST
I should probably take this and finish it before the merge.  Patch tomorrow.
Comment 3 Bob Clary [:bc:] 2012-01-29 08:58:08 PST
This is reproducible at http://www.nfl.com/gamecenter/2012012201/2011/POST20/giants%4049ers#menu=drivechart%26tab=track and another url which I'll omit since it locates someones house.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-29 12:31:02 PST
Created attachment 592528 [details] [diff] [review]
Patch with test
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-30 19:35:00 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/02e01981f362
Comment 6 Ed Morley [:emorley] 2012-01-31 06:52:45 PST
https://hg.mozilla.org/mozilla-central/rev/02e01981f362
Comment 7 Christian Holler (:decoder) 2013-01-19 13:56:38 PST
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929

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