843261 - Methods with sequence<DOMString> arguments generate invalid code in callback interfaces

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

callback interface Foo {
  void passStringSequence(sequence<DOMString> arg);
};

The code produced by this does not compile:

/Users/amccreight/mz/cent/obj-dbg/dom/bindings/JSImplBinding.cpp:49:16: error: no matching function for call to 'NonVoidStringToJsval'
          if (!xpc::NonVoidStringToJsval(cx, arg[i], &tmp)) {
               ^~~~~~~~~~~~~~~~~~~~~~~~~
/Users/amccreight/mz/cent/js/xpconnect/src/xpcpublic.h:283:6: note: candidate function not viable: no known conversion from 'const elem_type' (aka 'const nsString') to 'nsAString_internal &' for 2nd argument; 
bool NonVoidStringToJsval(JSContext *cx, nsAString &str, JS::Value *rval);
     ^
/Users/amccreight/mz/cent/js/xpconnect/src/xpcpublic.h:298:6: note: candidate function not viable: no known conversion from 'const elem_type' (aka 'const nsString') to 'mozilla::dom::DOMString &' for 2nd argument; 
bool NonVoidStringToJsval(JSContext* cx, mozilla::dom::DOMString& str,
     ^
1 error generated.


On the other hand, the output from this method does compile:
  void passCastableObjectSequence(sequence<Foo> arg);

Comment 1

[Boris Zbarsky [:bzbarsky]]

From CallbackMember.getArgConversion:

        if arg.type.isString():
            # XPConnect string-to-JS conversion wants to mutate the string.  So
            # let's give it a string it can mutate
            # XXXbz if we try to do a sequence of strings, this will kinda fail.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So one interesting option is to just add non-mutating version of NonVoidStringToJsval and whatnot that take a const string and make sure to deal accordingly...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Add support for sequence<DOMString> arguments in WebIDL callbacks. does allow people to accidentally hit the slower path through use

of non-const strings, but I think that's OK now that we're mostly
autogenerating this stuff

Comment 4

[Boris Zbarsky [:bzbarsky]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/1d8af801d850

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, I had to backout because something in this push caused a spike in mochitest-a11y assertions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb572a342efa

https://tbpl.mozilla.org/php/getParsedLog.php?id=20189591&tree=Mozilla-Inbound

A quick skim of the log shows them to be of the type:
###!!! ASSERTION: bad size recorded: 'aInstanceSize == 0 || entry->GetClassSize() == aInstanceSize', file ../../../xpcom/base/nsTraceRefcntImpl.cpp, line 441

Comment 6

[Boris Zbarsky [:bzbarsky]]

That wasn't due to this patch.  Repushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/38578014b8a8

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38578014b8a8