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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: Waldo)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86
Mac OS X
assertion, regression, testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

9 years ago
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)
(Reporter)

Comment 1

9 years ago
Happens on TM branch, too.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
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).
Blocks: 478512
Flags: blocking1.9.1?
Keywords: regression

Comment 3

9 years ago
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.
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...
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".

Updated

9 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Created attachment 367701 [details] [diff] [review]
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.
Attachment #367701 - Flags: review?(mrbkap)
Created attachment 367706 [details] [diff] [review]
Without superfluous resolve changes
Attachment #367701 - Attachment is obsolete: true
Attachment #367706 - Flags: review?(mrbkap)
Attachment #367701 - Flags: review?(mrbkap)

Updated

9 years ago
Attachment #367706 - Flags: review?(mrbkap) → review+
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).
(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 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
Attachment #367706 - Flags: review+ → review-
Created attachment 367868 [details] [diff] [review]
Or string_props
Attachment #367706 - Attachment is obsolete: true
Attachment #367868 - Flags: review?(brendan)
Created attachment 367876 [details] [diff] [review]
And with ATOM_KEY and without forgetting any past comments
Attachment #367876 - Flags: review?(brendan)
Attachment #367868 - Attachment is obsolete: true
Attachment #367868 - Flags: review?(brendan)
Comment on attachment 367876 [details] [diff] [review]
And with ATOM_KEY and without forgetting any past comments

Great, thanks.

/be
Attachment #367876 - Flags: review?(brendan) → review+
Created attachment 367882 [details] [diff] [review]
And with the resolving code again
Attachment #367876 - Attachment is obsolete: true
Attachment #367882 - Flags: review?(brendan)
Created attachment 367884 [details] [diff] [review]
And with ATOM_KEY in str_resolve

This is my favorite bug ever.
Attachment #367882 - Attachment is obsolete: true
Attachment #367884 - Flags: review+
Attachment #367882 - Flags: review?(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
Something got checked into tm before my final r+ :-(.

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

/be
Aargh, didn't see your very very last comment before pushing.  Looking more...
Created attachment 367905 [details] [diff] [review]
Please let this be the last patch for this bug
Attachment #367884 - Attachment is obsolete: true
Attachment #367905 - Flags: review?(brendan)

Comment 20

9 years ago
http://hg.mozilla.org/mozilla-central/rev/196d83a0bb84
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Depends on: 483974

Comment 21

9 years ago
this caused Bug 483974
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
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.
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

9 years ago
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
Flags: in-testsuite+
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
Attachment #367905 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/54c4ec3f20ac

Reopening so this gets tracked into m-c correctly...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: fixed-in-tracemonkey

Comment 28

9 years ago
http://hg.mozilla.org/mozilla-central/rev/54c4ec3f20ac
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 30

9 years ago
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Duplicate of this bug: 419365

Comment 33

9 years ago
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
You need to log in before you can comment on or make changes to this bug.