1075198 - Generated Constructor code for Events with Uint8Array not working

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Garner Lee]

Code generation for event WebIDLs with a Uint8Array typed array field will result in a type mismatch compile error:

Ex:
[Constructor(DOMString type, optional HCIEventTransactionEventInit eventInitDict)]
interface HCIEventTransactionEvent: Event
{
  readonly attribute Uint8Array aid;
  readonly attribute DOMString origin;
};
 
dictionary HCIEventTransactionEventInit: EventInit
{
  Uint8Array aid;
  DOMString origin = "";
};

This outputs:
already_AddRefed<HCIEventTransactionEvent>
HCIEventTransactionEvent::Constructor(mozilla::dom::EventTarget* aOwner, const nsAString& aType, const HCIEventTransactionEventInit& aEventInitDict)
{
...
  e->mAid = aEventInitDict.mAid;
  e->mOrigin = aEventInitDict.mOrigin;
...
}

class HCIEventTransactionEvent : public Event
{
...
  JS::Heap<JSObject*> mAid;
  nsString mOrigin;
...
}

That causes this operator= error:
In file included from UnifiedBindings5.cpp:66:0:
HCIEventTransactionEvent.cpp: In static member function 'static already_AddRefed<mozilla::dom::HCIEventTransactionEvent> mozilla::dom::HCIEventTransactionEvent::Constructor(mozilla::dom::EventTarget*, const nsAString_internal&, const mozilla::dom::HCIEventTransactionEventInit&)':
HCIEventTransactionEvent.cpp:72:28: error: no match for 'operator=' in 'e.nsRefPtr<T>::operator-><mozilla::dom::HCIEventTransactionEvent>()->mozilla::dom::HCIEventTransactionEvent::mAid = aEventInitDict.mozilla::dom::HCIEventTransactionEventInit::mAid'
HCIEventTransactionEvent.cpp:72:28: note: candidates are:
In file included from ../../dist/include/mozilla/dom/AudioNodeBinding.h:6:0,
                 from GainNodeBinding.cpp:3,
                 from UnifiedBindings5.cpp:2:
../../dist/include/js/RootingAPI.h:243:14: note: JS::Heap<T>& JS::Heap<T>::operator=(T) [with T = JSObject*]
../../dist/include/js/RootingAPI.h:243:14: note:   no known conversion for argument 1 from 'const mozilla::dom::Optional<mozilla::dom::TypedArray<unsigned char, js::UnwrapUint8Array, JS_GetUint8ArrayData, js::GetUint8ArrayLengthAndData, JS_NewUint8Array> >' to 'JSObject*'
../../dist/include/js/RootingAPI.h:248:14: note: JS::Heap<T>& JS::Heap<T>::operator=(const JS::Heap<T>&) [with T = JSObject*]
../../dist/include/js/RootingAPI.h:248:14: note:   no known conversion for argument 1 from 'const mozilla::dom::Optional<mozilla::dom::TypedArray<unsigned char, js::UnwrapUint8Array, JS_GetUint8ArrayData, js::GetUint8ArrayLengthAndData, JS_NewUint8Array> >' to 'const JS::Heap<JSObject*>&'

Expected:
Event WebIDL code gen should output vaild a constructor if passed an dictionary initializer.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

*EventInit needs to either define default value for the properties, or mark properties
required.
Is the missing default value or missing 'required' causing this issue?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Though, I don't see any existing generated *EventInit using it, so could be just codegenerator
missing support for this.

Comment 3

[Garner Lee]

Just checked: Adding "required" to the init dictionary field still results in the same error.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

What about making the property nullable and defaulting to = null; ?

Comment 5

[Garner Lee]

Uint8Array? aid = null;

Similar error: 
HCIEventTransactionEvent.cpp:72:28: note: candidates are:
In file included from ../../dist/include/mozilla/dom/FunctionBinding.h:6:0,
                 from FunctionBinding.cpp:3,
                 from UnifiedBindings5.cpp:2:
../../dist/include/js/RootingAPI.h:243:14: note: JS::Heap<T>& JS::Heap<T>::operator=(T) [with T = JSObject*]
../../dist/include/js/RootingAPI.h:243:14: note:   no known conversion for argument 1 from 'const mozilla::dom::Nullable<mozilla::dom::TypedArray<unsigned char, js::UnwrapUint8Array, JS_GetUint8ArrayData, js::GetUint8ArrayLengthAndData, JS_NewUint8Array> >' to 'JSObject*'
../../dist/include/js/RootingAPI.h:248:14: note: JS::Heap<T>& JS::Heap<T>::operator=(const JS::Heap<T>&) [with T = JSObject*]
../../dist/include/js/RootingAPI.h:248:14: note:   no known conversion for argument 1 from 'const mozilla::dom::Nullable<mozilla::dom::TypedArray<unsigned char, js::UnwrapUint8Array, JS_GetUint8ArrayData, js::GetUint8ArrayLengthAndData, JS_NewUint8Array> >' to 'const JS::Heap<JSObject*>&'

There's a case statement for sequence, and TypedArrays appear to fall to "else" in CodeGen.py (if that's the right place). It also seems like TypedArrays are a special case elsewhere?

Comment 6

[Garner Lee]

Hi, any updates?

Comment 7

[Garner Lee]

Attachment: 0003-Bug-1075198-Support-code-generation-for-TypedArrays-.patch

This patch should add support for TypedArrays for Codegen'ed based on IRC chats. Please take a look. Thanks!

Comment 8

[Garner Lee]

Attachment: 0003-Bug-1075198-Support-code-generation-for-TypedArrays-.patch

Wrong patch directory...re-upload.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8517896 [details] [diff] [review]
0003-Bug-1075198-Support-code-generation-for-TypedArrays-.patch

>+                    elif m.type.isSpiderMonkeyInterface():
>+                        if cxOfMembers == "":
>+                            cxOfMembers = dedent(
>+                                """
>+                                  AutoJSAPI jsapi;
>+                                  jsapi.Init();
Actually, you don't need this anymore, assuming you limit types to required  and nullable types.
(And this should have inited jsapi with the right global)


>+                                  JSContext *cx = jsapi.cx();
>+                                """)
>+                        srcname = "%s.%s" % (self.args[1].name, name)
>+                        members += fill("if (${varname}.WasPassed()) {\n", varname=srcname)
>+                        declType = ""
>+                        if m.type.nullable():
>+                            declType = m.type.inner.name
>+                        else:
>+                            declType = m.type.name
So we don't deal with the nullable part at all.
Based on my testing nullable typed array (the one with '?' ) and non-nullable will get exactly the same code. 
But since the dictionary has different values for them, the nullable one doesn't actually compile.


>+                        members += fill(
>+                            """
>+                              const ${declType}& ${varname} = ${srcname}.Value();
>+                              ${varname}.ComputeLengthAndData();
>+                              e->${varname} = ${varname}.Obj();
>+                            } else {
>+                              e->${varname} = ${declType}::Create(cx, aOwner, 0, nullptr);
>+                            }
This else part is unusual, 
I think we should not support
Uint8Array propertyName; in the dictionary, but only
required Uint8Array propertyName;
and 
Uint8Array? propertyName = null;
That would be consistent with other object-like types.

Comment 10

[Garner Lee]

Attachment: 0003-Bug-1075198-Support-code-generation-for-TypedArrays-.patch

I did some updates per review comments/irc. Typed arrays ( isSpiderMonkeyInterface()) is just stored as JS::Heap<JSObject*>, and now handles nulls.

Dictionary inits will only allow only required or optional with a default (the two cases above). Compile after Codegen fails otherwise as a side effect of types not matching. The event init arg type doesn't seem to be available to ask at generation time to throw an error earlier.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Ah, bindings layer ends up calling .setObjectOrNull(somejsobject); after calling the implementation method.

Comment 12

[Garner Lee]

Attachment: 0003-Bug-1075198-Support-code-generation-for-TypedArrays-.patch

Fixed indentation nit on else, and add r=.

Comment 13

[Garner Lee]

Trying: https://tbpl.mozilla.org/?tree=Try&rev=fc330698aae4

I'll probably create some test cases in the dependent bug or separate bug, unless desired in this bug.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

If you could still upload the generated code + some example .webidl which uses both required and '?' types.
Or pastebin at least.

Comment 15

[Garner Lee]

Attachment: Example Event WebIDL: HCIEventTransactionEvent.webidl

Comment 16

[Garner Lee]

Attachment: Example code output: HCIEventTransactionEvent.cpp

Relevant portion:
  // Dict: Uint8Array? aid = null;
  if (aEventInitDict.mAid.IsNull()) { 
    e->mAid = nullptr;
  } else {
    e->mAid = aEventInitDict.mAid.Value().Obj();
  }

  // Dict: required Uint8Array payload;
  e->mPayload.set(aEventInitDict.mPayload.Obj());

Comment 17

[Garner Lee]

Thanks. Try tests green on partial test set.

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Carsten Book [:Tomcat]]

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