1048659 - Returning unions from JS-implemented WebIDL doesn't work

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

WebIDL:

(TestInterfaceJSUnionableDictionary or long) pingPongDictionaryOrLong(optional (TestInterfaceJSUnionableDictionary or long) dictOrLong);

JS Implementation method:

pingPongDictionaryOrLong: function(dictOrLong) { return dictOrLong; }

Generated Code: https://pastebin.mozilla.org/5813809

Note the lack of setting aRetVal in TestInterfaceJSJSImpl::PingPongDictionaryOrLong. It seems pretty clear that we need to fix something in getResultConversion, but I don't want to get sidetracked with that right now.

Test coverage for this can be easily enabled by removing the try/catch in test_bug1036214.

Comment 1

[Bobby Holley (:bholley)]

I'm guessing that this is something that Boris can fix in a matter of minutes, so I'm putting it in his NI bucket for when he gets back from vacation. No hurry at all here Boris.

Comment 2

[Bobby Holley (:bholley)]

Actually, I had to remove the try/catch, because we end up throwing an uncatchable exception - I just made the return type of the method |long| for now.

Comment 3

[Andrew McCreight [:mccr8]]

I can take a look.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Mmm... the tests for this are commented out in TestJSImplGen, lovely.

This used to just fail codegen until I partially implemented it (by accident) in bug 949264; now it just does the wrong thing.

I think all that's needed here is to change getJSToNativeConversionTemplate for unions to do two things:

1)  Replace most (all?) uses of isMember by "isMember or isCallbackReturnValue".  That will make the conversion happen directly into declName, which will be of owning union type.  Maybe make a new isOwningUnion boolean?

2)  Crib what the isMozMap() case in getJSToNativeConversionTemplate does to declType/declArgs when not isMember and isCallbackReturnValue.  Just make sure to not clobber earlier sets of declArgs, if any.



2)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and the point is that (2) will make the declName be a reference to aRetval, so the data will end up in the right place.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Return the union we construct for a callback. WIP

I implemented something following bz's comment 4 and it seems to produce reasonable looking output that compiles in one case.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Why only in the non-nullable case?

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to On vacation Aug 5-18.  Please do not request review. from comment #7)
> Why only in the non-nullable case?

Oops, thanks for pointing that out.

The TestExampleGen.webidl cases that return unions were also commented out.  Uncommenting them produces:
void ReceiveUnion(OwningCanvasPatternOrCanvasGradient& aRetVal);
void ReceiveUnion2(JSContext* cx, OwningObjectOrLong& aRetVal);
void ReceiveUnionContainingNull(OwningCanvasPatternOrNullOrCanvasGradient& aRetVal);
void ReceiveNullableUnion(Nullable<OwningCanvasPatternOrCanvasGradient>& aRetVal);
void ReceiveNullableUnion2(JSContext* cx, Nullable<OwningObjectOrLong>& aRetVal);
void GetWritableUnion(OwningCanvasPatternOrCanvasGradient& aRetVal) const;
void SetWritableUnion(const CanvasPatternOrCanvasGradient& arg);
void GetWritableUnionContainingNull(OwningCanvasPatternOrNullOrCanvasGradient& aRetVal) const;
void SetWritableUnionContainingNull(const CanvasPatternOrNullOrCanvasGradient& arg);
void GetWritableNullableUnion(Nullable<OwningCanvasPatternOrCanvasGradient>& aRetVal) const;
void SetWritableNullableUnion(const Nullable<CanvasPatternOrCanvasGradient>& arg);

These look reasonable to me, I guess.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Return the union we construct in a JS callback.

I included a few basic tests, as the broken version still compiled, so that's obviously not sufficient.

Comment 10

[Andrew McCreight [:mccr8]]

green L64 debug try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6a53376d32a4

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 8469645 [details] [diff] [review]
Return the union we construct in a JS callback.

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

::: dom/bindings/test/test_returnUnion.html
@@ +26,5 @@
> +    is(t.pingPongUnionContainingNull(t2), t2, "ping pong union containing null right case should be identity");
> +
> +    is(t.pingPongNullableUnion(t2), t2, "ping pong nullable union left case should be identity");
> +    is(t.pingPongNullableUnion(12), 12, "ping pong nullable union for right case should be identity");
> +    is(t.pingPongNullableUnion(null), null, "ping pong nullable union for null case should be identity");

Maybe you should use this form of the phrase with "for" everywhere, it's slightly more readable.

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Peter Van der Beken [:peterv] from comment #11)
> Maybe you should use this form of the phrase with "for" everywhere, it's
> slightly more readable.

Good point.  Done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6673cfac7488

Comment 13

[Andrew McCreight [:mccr8]]

Thanks for explaining how to fix this, Boris, it made it a lot easier.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Thank you for writing the patch and everything!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6673cfac7488