Closed Bug 933497 Opened 6 years ago Closed 6 years ago

Teach ObjectWrapper.jsm how to deal with TypedArrays

Categories

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

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: khuey, Assigned: khuey)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

ObjectWrapper.jsm treats typed arrays as dictionary objects.  This makes WebNFC very sad.
Attached patch Patch (obsolete) — Splinter Review
Give this a shot Arno?
Attachment #825576 - Flags: feedback?(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}]
Ok, we'll look at it in the office.
Attachment #825576 - Attachment is obsolete: true
Attachment #825576 - Flags: feedback?(arno)
Attached patch Patch (obsolete) — Splinter Review
This works.
Depends on: 933681
Blocks: 933585
(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+
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.
Attached patch PatchSplinter Review
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?
I'll give the patch a try tomorrow (device busy...) if Arno isn't available.
(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)
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.
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/mozilla-central/rev/c595dfa92c12
Status: NEW → RESOLVED
Closed: 6 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? → ---
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.