Last Comment Bug 761121 - JS_ResolveStandardClass checks wrong JSClass for Typed Arrays
: JS_ResolveStandardClass checks wrong JSClass for Typed Arrays
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on:
Blocks: 758415
  Show dependency treegraph
 
Reported: 2012-06-04 05:42 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-06-06 08:44 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Check the right JSClass for typed arrays during resolve. v1 (1.06 KB, patch)
2012-06-04 09:22 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Add JSCLASS_HAS_CACHED_PROTO to the typed array instance JSClass. v1 r=bhackett (1.59 KB, patch)
2012-06-05 07:54 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 05:42:15 PDT
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 1 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 09:22:22 PDT
Created attachment 629809 [details] [diff] [review]
Check the right JSClass for typed arrays during resolve. v1
Comment 2 Luke Wagner [:luke] 2012-06-04 10:04:18 PDT
Comment on attachment 629809 [details] [diff] [review]
Check the right JSClass for typed arrays during resolve. v1

Forwarding to someone more typed array-knowing.
Comment 3 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-06-04 16:05:41 PDT
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.
Comment 4 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-06-04 22:34:09 PDT
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.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-06-05 07:54:32 PDT
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.
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-06-05 10:09:13 PDT
green on try (see bug 758415). Landed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/b9f15febda50
Comment 7 Ed Morley [:emorley] 2012-06-06 08:44:07 PDT
https://hg.mozilla.org/mozilla-central/rev/b9f15febda50

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