843264 - Can't return sequence of interfaces in callback interface methods

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

This is a known issue, going by the comments in the code, but I figured I'd file a bug anyways.

callback interface Foo {
  sequence<Foo> bar();
};

Attempting to generate the bindings for this causes the following error:

[...]
 File "/Users/amccreight/mz/cent/dom/bindings/Codegen.py", line 7771, in __init__
    for sig in m.signatures()]
  File "/Users/amccreight/mz/cent/dom/bindings/Codegen.py", line 8044, in __init__
    self.singleOperation)
  File "/Users/amccreight/mz/cent/dom/bindings/Codegen.py", line 8002, in __init__
    needThisHandling)
  File "/Users/amccreight/mz/cent/dom/bindings/Codegen.py", line 7826, in __init__
    self.body = self.getImpl()
  File "/Users/amccreight/mz/cent/dom/bindings/Codegen.py", line 7832, in getImpl
    "returnResult": self.getResultConversion(),
  File "/Users/amccreight/mz/cent/dom/bindings/Codegen.py", line 7878, in getResultConversion
    False)[2]).substitute(replacements)
IndexError: tuple index out of range

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow returning sequences of non-primitive types from callback methods.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow returning sequences of non-primitive types from callback methods.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 740615 [details] [diff] [review]
Allow returning sequences of non-primitive types from callback methods.

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

::: dom/bindings/Codegen.py
@@ +2284,5 @@
>                                      isNullOrUndefined=False,
>                                      exceptionCode=None,
>                                      lenientFloatCode=None,
> +                                    allowTreatNonCallableAsNull=False,
> +                                    isCallbackReturnValue=False):

Maybe add a comment down below about the ways in which isCallbackReturnValue changes the behavior?  One at the top of this function is probably more useful than scattered around where it is used.

@@ +2515,5 @@
>              arrayRef = "${declName}"
>          # If we're optional or a member, the const will come from the Optional
>          # or whatever we're a member of.
>          mutableTypeName = typeName
> +        if not isOptional and not isMember and not isCallbackReturnValue:

|not (isOptional or isMember or isCallbackReturnValue)| to be consistent with the Sequence vs AutoSequence test, maybe?

@@ +2800,5 @@
>  
>          # This is an interface that we implement as a concrete class
>          # or an XPCOM interface.
>  
>          # Allow null pointers for nullable types and old-binding classes

comment is out of date

@@ +7782,5 @@
>                  # Decl is a raw pointer
>                  returnCode = ("NS_IF_ADDREF(${declName});\n"
>                                "return ${declName};")
>              else:
> +                # Decl is a raw pointer.

Maybe say that it is a non-null raw pointer?  It looks a little weird to have two cases in a row with identical comments but different code.  I assume this change is because callback return stuff uses foo*, but never actually returns null?  And NonNull stuff is going to automatically coerce so that you don't actually need the .Ptr()?

::: dom/bindings/test/TestJSImplGen.webidl
@@ -226,5 @@
> -  //sequence<TestJSImplInterface?> receiveNullableCastableObjectSequence();
> -  //sequence<TestCallbackInterface?> receiveNullableCallbackObjectSequence();
> -  //sequence<TestJSImplInterface>? receiveCastableObjectNullableSequence();
> -  //sequence<TestJSImplInterface?>? receiveNullableCastableObjectNullableSequence();
> -  // Callback interface ignores 'resultNotAddRefed'. See bug 843272.

Does this mean the patch also fixes bug 843272?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Maybe add a comment down below about the ways in which isCallbackReturnValue
> changes the behavior?

Added:

    If isCallbackReturnValue is true, then the declType may be adjusted to make
    it easier to return from a callback.  Since that type is never directly
    observable by any consumers of the callback code, this is OK.

> |not (isOptional or isMember or isCallbackReturnValue)| to be consistent
> with the Sequence vs AutoSequence test, maybe?

OK, done.

> comment is out of date

Fixed.

> Maybe say that it is a non-null raw pointer?

Agreed, done.

> I assume this change is because callback return stuff uses foo*, but never
> actually returns null?

It's actually a side-effect.  I wanted to switch to Sequence<nsRefPtr<> > for sequence return values instead of Sequence<OwningNonNull<> >, but because of how we determine the signature for those it also switched non-sequence return values from NonNull to pointer.

> And NonNull stuff is going to automatically coerce
> so that you don't actually need the .Ptr()?

The change is that the stack variable that the return value is now stored in is a pointer instead of a NonNull.  The pointer is still never null.

> Does this mean the patch also fixes bug 843272?

I don't think so.

So what I see is that the JSNative for receiveWeakCastableObjectSequence, say, has:

  nsTArray<nsRefPtr<mozilla::dom::TestJSImplInterface > > result;

and this of course matches the types in the actual callback codegen.  So it all works out because the callbacks codegen consistently ignores the resultNotAddRefed bits.

I just tried uncommenting all the tests that are claimed to be commented out due to bug 843272 and they all pass, for what it's worth....

Comment 6

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/96803f97e1ee

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96803f97e1ee