Closed Bug 1048659 Opened 7 years ago Closed 7 years ago

Returning unions from JS-implemented WebIDL doesn't work


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

Not set





(Reporter: bholley, Assigned: mccr8)


(Blocks 1 open bug)



(1 file, 1 obsolete file)


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

JS Implementation method:

pingPongDictionaryOrLong: function(dictOrLong) { return dictOrLong; }

Generated Code:

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.
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.
Flags: needinfo?(bzbarsky)
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.
I can take a look.
Assignee: nobody → continuation
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.

Flags: needinfo?(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.
I implemented something following bz's comment 4 and it seems to produce reasonable looking output that compiles in one case.
Why only in the non-nullable case?
(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.
I included a few basic tests, as the broken version still compiled, so that's obviously not sufficient.
Attachment #8468923 - Attachment is obsolete: true
Attachment #8469645 - Flags: review?(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.
Attachment #8469645 - Flags: review?(peterv) → review+
(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.
Blocks: 1036214
No longer depends on: 1036214
OS: Mac OS X → All
Hardware: x86 → All
Thanks for explaining how to fix this, Boris, it made it a lot easier.
Thank you for writing the patch and everything!
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.