If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Teach ObjectWrapper.jsm how to deal with TypedArrays

RESOLVED FIXED in mozilla28

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified
mozilla28
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

ObjectWrapper.jsm treats typed arrays as dictionary objects.  This makes WebNFC very sad.
Created attachment 825576 [details] [diff] [review]
Patch

Give this a shot Arno?
Attachment #825576 - Flags: feedback?(arno)

Comment 2

4 years ago
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}]
Ok, we'll look at it in the office.
Attachment #825576 - Attachment is obsolete: true
Attachment #825576 - Flags: feedback?(arno)
Created attachment 825710 [details] [diff] [review]
Patch

This works.
Blocks: 860906
Depends on: 933681

Updated

4 years ago
Blocks: 933585

Comment 5

4 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #0)
> ObjectWrapper.jsm treats typed arrays as dictionary objects.  This makes
> WebNFC very sad.
It is 1.3+.
blocking-b2g: --- → 1.3+
Attachment #825710 - Flags: review?(bobbyholley+bmo)
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?
Attachment #825710 - Flags: review?(bobbyholley+bmo) → review-
Er, yeah, I forgot this was the old version.
Created attachment 8339020 [details] [diff] [review]
Patch

Arno, can you test this?

Bobby, I removed the wrappedJSObject bit which should be unnecessary now.
Attachment #825710 - Attachment is obsolete: true
Attachment #8339020 - Flags: review?(bobbyholley+bmo)
Flags: needinfo?(arno)
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

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

Comment 11

4 years ago
(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.
Flags: needinfo?(arno)

Comment 12

4 years ago
Hi Kyle, I rolled out the patch, and applied the new ObjectWrapper patch. It seems to work fine for NFC reads and writes.
Flags: needinfo?(khuey)
NFC is a targeted feature for 1.3, not a committed feature. So this should not block the release.
blocking-b2g: 1.3+ → 1.3?
(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.
Flags: needinfo?(khuey) → needinfo?(bobbyholley+bmo)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Flags: needinfo?(bobbyholley+bmo)
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.
Attachment #8339020 - Flags: review?(bobbyholley+bmo) → review+
Is there any reason not to unwrap there?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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

4 years ago
Any updates? Is this check-in needed?
Flags: needinfo?(khuey)
I need to add the comment and then I'll land it tomorrow.
Flags: needinfo?(khuey)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c595dfa92c12
https://hg.mozilla.org/mozilla-central/rev/c595dfa92c12
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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.
blocking-b2g: 1.3? → ---

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.