JS_ResolveStandardClass checks wrong JSClass for Typed Arrays

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 629809 [details] [diff] [review]
Check the right JSClass for typed arrays during resolve. v1
Attachment #629809 - Flags: review?(luke)

Comment 2

5 years ago
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.
Created attachment 630168 [details] [diff] [review]
Add JSCLASS_HAS_CACHED_PROTO to the typed array instance JSClass. v1 r=bhackett

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:

http://hg.mozilla.org/integration/mozilla-inbound/rev/b9f15febda50
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla16

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/b9f15febda50
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.