Closed Bug 761121 Opened 10 years ago Closed 10 years ago

JS_ResolveStandardClass checks wrong JSClass for Typed Arrays


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bholley, Assigned: bholley)




(1 file, 1 obsolete file)

Typed arrays appear to have two JSClasses - one for the prototype, and one for the actual object. The details of this part are a bit fuzzy to me.

Suppose we're resolving "Uint8Array". During JS_ResolveStandardClass, we find stdnm in standard_class_names, and check IsStandardClassResolved to see if it's already been resolved. However, stdnm->clasp is the 'fast class', rather than the prototype class. And in jstypedarray.cpp, we do JSCLASS_HAS_CACHED_PROTO on |protoClass|, not on |class|.

So when we pass in stdnm->clasp, JSCLASS_CACHED_PROTO_KEY(clasp) returns 0, which is actually JSProto_Null.

This worked so far because JSProto_Null is unused, so it was always undefined, so we'd always re-resolve. But over in bug 758415 I'm using that slot (we actually always used it, but I'm causing it to be non-void more often - see bug 760095).

I've got a patch that seems to fix the issue, but this isn't my area of expertise. So hopefully someone who knows more can verify that it's correct.
Comment on attachment 629809 [details] [diff] [review]
Check the right JSClass for typed arrays during resolve. v1

Forwarding to someone more typed array-knowing.
Attachment #629809 - Flags: review?(luke) → review?(sphink)
Comment on attachment 629809 [details] [diff] [review]
Check the right JSClass for typed arrays during resolve. v1

Review of attachment 629809 [details] [diff] [review]:

Typed arrays I sort of understand. PROTO_CACHE gunk, not so much. But from what I can tell by digging into this, your change definitely seems correct.

But you need to do &ArrayBufferClass -> &ArrayBufferObject::protoClass and &DataViewClass -> &DataViewObject::protoClass too, then.

Looking into this also pointed out that we're setting HAS_CACHED_PROTO_KEY inconsistently in the 3 varieties of typed array Classes. I'll look at that separately.
Attachment #629809 - Flags: review?(sphink)
I made an attempt at changing everything that seemed fishy, in bug 761506. If by some miracle bhackett says that what I'm doing there is legit, then this bug can become a dupe.
Adding the patch that bhackett r+ed in bug 761506 comment 3.
Attachment #629809 - Attachment is obsolete: true
Attachment #630168 - Flags: review+
green on try (see bug 758415). Landed to m-i:
Target Milestone: --- → mozilla16
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.