929609 - WebIDL JS implemented Uint8Array not working

Status: RESOLVED FIXED

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

Description

[Garner Lee]

Uint8Array in a JS implemented WebIDL appears to be unsupported.

Exmaple:
[Constructor(Uint8Array payload),
 JSImplementation="@mozilla.org/nfc/FooRecord;1"]
interface FooRecord
{
  readonly attribute Uint8Array payload;
};

In generated C++ FooRecordBindings code, on compilation, it will complain that the symbol Uint8Array is missing (Found in TypedArray.h?)

On IRC, mccr8 pointed to comments in TestJSImplGen.webidl:
  // ArrayBuffer is handled differently in callback interfaces and the example generator.
  // Need to figure out what should be done there.  Seems like other typed array stuff is
  // similarly not working in the JS implemented generator.  Probably some other issues
  // here as well.

Comment 1

[Garner Lee]

WebNFC (Bug 674741) would like to use it, but is not blocked by this.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I have a patch for this, which I will attach here just for posterity.  But Bobby wants us to get the security story here straight before we land such a patch.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Rename jsObjectsArePtr to typedArraysAreStructs

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2. The meat of the changes.

Comment 5

[Boris Zbarsky [:bzbarsky]]

In particular, we need Xrays for typed arrays before we can land this, as I understand.  Bobby, do we have a bug tracking that?

Comment 6

[psiddh]

Requesting for an update on possibility of using typed arrays in WebIDL on JS implemented interfaces.
We are currently proposing interfaces for Secure Element (https://bugzilla.mozilla.org/show_bug.cgi?id=879861 - comment#87 )  that may have to use typed arrays on JS implemented interfaces.

Comment 7

[Bobby Holley (:bholley)]

Yes, we have Xrays to TypedArrays now. They basically just throw and say "hey, you should clone this instead", but things should be secure-by-default with the new wrappers in place.

Comment 9

[Andrew McCreight [:mccr8]]

Boris, is it possible to fix this now, per comment 7?  It sounds like some B2G people would like this for some WebIDL they are working on and they are asking about what kind of timeline etc. there is for fixing this.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  It seems to me like the setup described in 7 will make actually implementing such an API pretty painful.  But we can certainly do the codegen bits; life will just suck in the JS implementation. 

I guess for bug 929609 we just need the typed array return value, not an argument, so maybe it doesn't matter too much.

I'll try to get to this in the next few days.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Rename "jsObjectsArePtr" to "typedArraysAreStructs" (and invert its meaning, of course), since that's what it's really used for nowadays

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Fix passing of typed arrays to JS-implemented WebIDL to actually work

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Fix passing of typed arrays to JS-implemented WebIDL to actually work

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 8476495 [details] [diff] [review]
part 1.  Rename "jsObjectsArePtr" to "typedArraysAreStructs" (and invert its meaning, of course), since that's what it's really used for nowadays

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

::: dom/bindings/Codegen.py
@@ +12048,3 @@
>          """
> +        If typedArraysAreStructs is false, typed arrays will be passed as
> +        JS::Handle<JSObject*>.

Maybe also make it clear what true means, since that explains the name :-).

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 8476711 [details] [diff] [review]
part 2.  Fix passing of typed arrays to JS-implemented WebIDL to actually work

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

::: dom/bindings/Codegen.py
@@ +1039,5 @@
> +                        # Since we can't forward-declare typed array types
> +                        # (because they're typedefs), we have to go ahead and
> +                        # just include their header if we need to have functions
> +                        # taking references to them declared in that header.
> +                        headerSet = declareIncludes

Hmm, 'k. Let's hope no one uses headerSet after this to add other headers for the general isInterface() case.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Let's hope no one uses headerSet after this to add other headers for the general
> isInterface() case.

I think we should be ok; I can't think of headers one would want for both SpiderMonkey and Gecko interfaces.

Comment 17

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/dede44b94883
   https://hg.mozilla.org/integration/mozilla-inbound/rev/1a3d7ec1a840

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dede44b94883
https://hg.mozilla.org/mozilla-central/rev/1a3d7ec1a840