933497 - Teach ObjectWrapper.jsm how to deal with TypedArrays

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

ObjectWrapper.jsm treats typed arrays as dictionary objects.  This makes WebNFC very sad.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Give this a shot Arno?

Comment 2

[arno]

Comment on attachment 825576 [details] [diff] [review]
Patch

Review of attachment 825576 [details] [diff] [review]:
-----------------------------------------------------------------

E/GeckoConsole(  180): [JavaScript Error: "aCtxt[aObject.constructor.name] is not a constructor" {file: "resource://gre/modules/ObjectWrapper.jsm" line: 58}]

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok, we'll look at it in the office.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This works.

Comment 5

[Ken Chang[:kenkai]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #0)
> ObjectWrapper.jsm treats typed arrays as dictionary objects.  This makes
> WebNFC very sad.
It is 1.3+.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 825710 [details] [diff] [review]
Patch

Review of attachment 825710 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ObjectWrapper.jsm
@@ +54,5 @@
>          res.push(this.wrap(aObj, aCtxt));
>        }, this);
>        return res;
> +    } else if (TypedArrayThings.indexOf(kind) !== -1) {
> +      return new aCtxt.wrappedJSObject[kind](aObject);

Isn't avoiding the .wrappedJSObject here the whole point of blocking on bug 933681?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Er, yeah, I forgot this was the old version.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Arno, can you test this?

Bobby, I removed the wrappedJSObject bit which should be unnecessary now.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8339020 [details] [diff] [review]
Patch

Review of attachment 8339020 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ObjectWrapper.jsm
@@ +54,5 @@
>          res.push(this.wrap(aObj, aCtxt));
>        }, this);
>        return res;
> +    } else if (TypedArrayThings.indexOf(kind) !== -1) {
> +      return new aCtxt[kind](aObject);

So, aObject is going to end up as a COW, meaning we'll do a very slow, iterative copy, where each element copied needs to round-trip through the COW policy and discover that it's indexed access on a typed array and therefore allowed. Does performance matter at all here?

Comment 10

[Garner Lee]

I'll give the patch a try tomorrow (device busy...) if Arno isn't available.

Comment 11

[arno]

(In reply to Garner Lee from comment #10)
> I'll give the patch a try tomorrow (device busy...) if Arno isn't available.

Yes, Garner, that would be nice. Remember to remove the old patch we've gotten from Kyle that we committed to our branch.

Comment 12

[Garner Lee]

Hi Kyle, I rolled out the patch, and applied the new ObjectWrapper patch. It seems to work fine for NFC reads and writes.

Comment 13

[Jason Smith [:jsmith]]

NFC is a targeted feature for 1.3, not a committed feature. So this should not block the release.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Bobby Holley (:bholley) from comment #9)
> Comment on attachment 8339020 [details] [diff] [review]
> Patch
> 
> Review of attachment 8339020 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/ObjectWrapper.jsm
> @@ +54,5 @@
> >          res.push(this.wrap(aObj, aCtxt));
> >        }, this);
> >        return res;
> > +    } else if (TypedArrayThings.indexOf(kind) !== -1) {
> > +      return new aCtxt[kind](aObject);
> 
> So, aObject is going to end up as a COW, meaning we'll do a very slow,
> iterative copy, where each element copied needs to round-trip through the
> COW policy and discover that it's indexed access on a typed array and
> therefore allowed. Does performance matter at all here?

I don't understand this at all.  Surely we end up in http://mxr.mozilla.org/mozilla-central/source/js/src/vm/TypedArrayObject.cpp?force=1#2228 and take the branch at http://mxr.mozilla.org/mozilla-central/source/js/src/vm/TypedArrayObject.cpp?force=1#2336, which just does js_memcpy.

Regardless, I don't think performance matters for constructing these.  At least not for 1.3.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #14)
> > So, aObject is going to end up as a COW, meaning we'll do a very slow,
> > iterative copy, where each element copied needs to round-trip through the
> > COW policy and discover that it's indexed access on a typed array and
> > therefore allowed. Does performance matter at all here?
> 
> I don't understand this at all.  Surely we end up in
> http://mxr.mozilla.org/mozilla-central/source/js/src/vm/TypedArrayObject.
> cpp?force=1#2228 and take the branch at
> http://mxr.mozilla.org/mozilla-central/source/js/src/vm/TypedArrayObject.
> cpp?force=1#2336, which just does js_memcpy.

No, because |other| is a cross-compartment security wrapper, not a typed array. So other->is<TypedArrayObject>() will return false. When content touches the chrome-object, it'll be a COW, and thus will need to be duck-typed.
 
> Regardless, I don't think performance matters for constructing these.  At
> least not for 1.3.

Ok.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8339020 [details] [diff] [review]
Patch

Review of attachment 8339020 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a comment explaining the slowness. r=bholley with that.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is there any reason not to unwrap there?

Comment 18

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #17)
> Is there any reason not to unwrap there?

Unwrap what? The security wrapper? That's a pretty big security hazard that I'd like to avoid if possible.

Comment 19

[Garner Lee]

Any updates? Is this check-in needed?

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I need to add the comment and then I'll land it tomorrow.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c595dfa92c12

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c595dfa92c12

Comment 23

[Andrew Overholt [:overholt]]

This is in 28 which will be the basis for 1.3 so I'm going to remove 1.3? .

If someone feels strongly that we should have this as a blocker (to prevent backouts), please re-nom.