If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Returning unions from JS-implemented WebIDL doesn't work

RESOLVED FIXED in mozilla34

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bholley, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

unspecified
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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.
(Assignee)

Comment 3

3 years ago
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.



2)
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.
(Assignee)

Comment 6

3 years ago
Created attachment 8468923 [details] [diff] [review]
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.
Why only in the non-nullable case?
(Assignee)

Comment 8

3 years ago
(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.
(Assignee)

Comment 9

3 years ago
Created attachment 8469645 [details] [diff] [review]
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.
Attachment #8468923 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 580070
(Assignee)

Updated

3 years ago
Attachment #8469645 - Flags: review?(peterv)
(Assignee)

Comment 10

3 years ago
green L64 debug try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6a53376d32a4
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+
(Assignee)

Comment 12

3 years ago
(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
(Assignee)

Updated

3 years ago
Blocks: 1036214
No longer depends on: 1036214
(Assignee)

Updated

3 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 13

3 years ago
Thanks for explaining how to fix this, Boris, it made it a lot easier.
Thank you for writing the patch and everything!
https://hg.mozilla.org/mozilla-central/rev/6673cfac7488
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.