483103 - TM: "Assertion failed: p->isQuad()" with str["-1"]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

On mozilla-central:

var t = new String("");
for (var j = 0; j < 3; ++j) { 
  var e = t["-1"];
}

Assertion failed: p->isQuad() (../nanojit/Nativei386.cpp:1602)

Comment 1

[Jesse Ruderman]

Happens on TM branch, too.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Related to bug 478512? autoBisect shows:

The first bad revision is:
changeset:   25416:707f96a1de28
parent:      25413:c63cf255ec3b
user:        Andreas Gal
date:        Thu Feb 26 19:01:02 2009 -0800
summary:     Trace reading undefined properties (478512, r=jwalden).

Comment 3

[Andreas Gal :gal]

It seems String("")[-1] and String("")["-1"] both return 0, and not the usual value we would expect here (undefined). We already catch the -1 case since its JSVAL_INT, but not the string version of it. The crash here is a result of the disagreement in type betwen tracer and interpreter. Tracer records an undefined, whereas the interpreter sees a number (0). This seems to be problematic for -1 only, which seems to be the length of the string and hence its always a number.

Comment 4

[Jeff Walden [:Waldo]]

str[-1] returns the length of the string; this isn't really an intended behavior, but it's what we've had.  I suspect perhaps str_resolve or something like that is doing more than we expect it to, or something like that -- more soon...

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

The offending code is in str_getProperty. Since string's length property has a tinyid of -1, when we actually try to get the property "-1" from str, we normalize it to a number and get it confused with the tinyid for "length".

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Don't give length a tinyid, don't make stringObj[-1] a special property

If you're using a String object, you're using the wrong tool, and we shouldn't optimize the length property's overhead for you.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Without superfluous resolve changes

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 367706 [details] [diff] [review]
Without superfluous resolve changes

>+    if (JSVAL_IS_STRING(id) && id == ATOM_TO_JSID(cx->runtime->atomState.lengthAtom)) {

This compares a jsval to a jsid. An additional ID_TO_VALUE here would do this code well for type correctness (and is correct, since the jsval passed to getProperty is canonical, per the API).

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> (From update of attachment 367706 [details] [diff] [review])
> >+    if (JSVAL_IS_STRING(id) && id == ATOM_TO_JSID(cx->runtime->atomState.lengthAtom)) {

Use ATOM_KEY not ATOM_TO_JSID. This way you can drop the JSVAL_IS_STRING.

> This compares a jsval to a jsid. An additional ID_TO_VALUE here would do this
> code well for type correctness (and is correct, since the jsval passed to
> getProperty is canonical, per the API).

ID_TO_VALUE(ATOM_TO_JSID(...)) === ATOM_KEY(...).

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 367706 [details] [diff] [review]
Without superfluous resolve changes

> static JSPropertySpec string_props[] = {
>-    {js_length_str,     STRING_LENGTH,
>-                        JSPROP_READONLY|JSPROP_PERMANENT|JSPROP_SHARED, 0,0},
>+    {js_length_str, 0, JSPROP_READONLY|JSPROP_PERMANENT|JSPROP_SHARED, 0,0},
>     {0,0,0,0,0}
> };

This still has a tinyid (shortid), 0. string_props should be removed since you are handling the length id in the class getter.

/be

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Or string_props

Comment 12

[Jeff Walden [:Waldo]]

Attachment: And with ATOM_KEY and without forgetting any past comments

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 367876 [details] [diff] [review]
And with ATOM_KEY and without forgetting any past comments

Great, thanks.

/be

Comment 14

[Jeff Walden [:Waldo]]

Attachment: And with the resolving code again

Comment 15

[Jeff Walden [:Waldo]]

Attachment: And with ATOM_KEY in str_resolve

This is my favorite bug ever.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 367882 [details] [diff] [review]
And with the resolving code again

>@@ -621,7 +607,21 @@ str_resolve(JSContext *cx, JSObject *obj
>     JSString *str, *str1;
>     jsint slot;
> 
>+    if (flags & JSRESOLVE_ASSIGNING)
>+        return JS_TRUE;
>+
>+    if (id == ATOM_TO_JSID(cx->runtime->atomState.lengthAtom)) {
>+        v = OBJ_GET_SLOT(cx, obj, JSSLOT_PRIVATE);
>+        str = JSVAL_TO_STRING(v);
>+        if (!OBJ_DEFINE_PROPERTY(cx, obj, id, INT_TO_JSVAL(17), NULL, NULL,

What's INT_TO_JSVAL(17) for?

>+                                 JSPROP_READONLY | JSPROP_PERMANENT | JSPROP_SHARED, NULL)) {

So resolve will run on the direct object before String.prototype, making an "own" property. So no point in JSPROP_SHARED. SHARED PERMANENT in the prototype emulates own in each instance.

So alternatively we could just define 'length' on String.prototype as S+P but with no tinyid. js_InitStringClass would do this with a follow js_DefineProperty call passing the proto r.v. of JS_InitClass. Then there's no need for this resolve code, which is a bit fatter than the alternative code in js_InitStringClass, and certainly costlier at runtime (if you're making String objects and asking for their length props -- doing that loses for sure, but no need to make it worse).

We should have a test for hasOwnProperty too, since length is supposed to be own.

/be

Comment 17

[Brendan Eich [:brendan]]

Something got checked into tm before my final r+ :-(.

http://hg.mozilla.org/tracemonkey/rev/196d83a0bb84

/be

Comment 18

[Jeff Walden [:Waldo]]

Aargh, didn't see your very very last comment before pushing.  Looking more...

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Please let this be the last patch for this bug

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/196d83a0bb84

Comment 21

[Bob Clary [:bc] (inactive)]

this caused Bug 483974

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 367905 [details] [diff] [review]
Please let this be the last patch for this bug

>+    if (!OBJ_DEFINE_PROPERTY(cx, proto, ATOM_TO_JSID(cx->runtime->atomState.lengthAtom),
>+                             JSVAL_VOID, NULL, NULL, JSPROP_PERMANENT | JSPROP_SHARED,

ES3 15.5.5.1 says ReadOnly -- isn't that needed here to keep String.prototype.length = 42 from sticking?

/be

Comment 23

[Jeff Walden [:Waldo]]

It's probably better for brainprint to add it, but I don't think so (verified empirically too), because any get of the property will go through str_getProperty and return the real value.

Comment 24

[Jeff Walden [:Waldo]]

Also for bug 483974's sake -- anything else beyond that?  I'd rather not attach a new patch just for that, I've made enough of a mess of this bug as it is.

Comment 25

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/1a750cb5593e

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-483103.js,v  <--  regress-483103.js
initial revision: 1.1

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 367905 [details] [diff] [review]
Please let this be the last patch for this bug

>@@ -2861,6 +2847,12 @@ js_InitStringClass(JSContext *cx, JSObje
>         return NULL;
>     STOBJ_SET_SLOT(proto, JSSLOT_PRIVATE,
>                    STRING_TO_JSVAL(cx->runtime->emptyString));
>+    if (!OBJ_DEFINE_PROPERTY(cx, proto, ATOM_TO_JSID(cx->runtime->atomState.lengthAtom),
>+                             JSVAL_VOID, NULL, NULL, JSPROP_PERMANENT | JSPROP_SHARED,
>+                             NULL)) {

Could use js_DefineNativeProperty here, cut out some middle-men

r=me other than that, thanks. I'm assuming this passes all the tests including new ones.

/be

Comment 27

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/54c4ec3f20ac

Reopening so this gets tracked into m-c correctly...

Comment 28

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/54c4ec3f20ac

Comment 29

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bf1ff3de875e

Comment 30

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 31

[Bob Clary [:bc] (inactive)]

js1_8_1/trace/trace-test.js	
http://hg.mozilla.org/tracemonkey/rev/61892f57b46a

Comment 33

[Bob Clary [:bc] (inactive)]

cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/js/tests/shell.js,v  <--  shell.js