1069518 - XPTCall should refuse to implement interfaces with [notxpcom] methods

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

Currently if you ask xptcall to create a proxy for an interface, it will do so, even if the interface has [notxpcom] methods and cannot be implemented properly. NS_GetXPTCallSTub should just refuse to implement that.

Comment 1

[Benjamin Smedberg]

Attachment: 1069518-xptcall-notxpcom

Going to hold off on reviews until I have a try run: my browser starts, but I imagine this could cause havoc in weird places.

Comment 2

[Benjamin Smedberg]

Attachment: 1069518-xptcall-notxpcom

In addition to the XPCOM changes, when I was writing the test I discovered that XPConnect doesn't notice failure to create the xptcstub. This new version propagates that error correctly and now the test throws usefully. Appears to still work on try.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8492305 [details] [diff] [review]
1069518-xptcall-notxpcom

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

r=me on the XPCOM bits with the added test below.

::: xpcom/tests/NotXPCOMTest.idl
@@ +10,5 @@
> +    void method1();
> +};
> +
> +[scriptable, uuid(237d01a3-771e-4c6e-adf9-c97f9aab2950)]
> +interface ScriptableWithNotXPCOM : nsISupports

Can you add a test for an interface like:

[scriptable, uuid(blah)]
interface ScriptableButWithANotXPCOMParent : ScriptableWithNotXPCOM
{
    void method3 ();
};

?

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8492305 [details] [diff] [review]
1069518-xptcall-notxpcom

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

r=bholley on the XPConnect bits.

::: js/xpconnect/src/XPCWrappedJS.cpp
@@ +346,5 @@
>      JS::RootedObject rootJSObj(cx, clasp->GetRootJSObject(cx, jsObj));
>      if (!rootJSObj)
>          return NS_ERROR_FAILURE;
>  
> +    nsresult rv;

Given that this is only used as an out-param at the moment, I'd prefer it to be initialized with a known value.

@@ +387,5 @@
>        mClass(aClass),
>        mRoot(root ? root : MOZ_THIS_IN_INITIALIZER_LIST()),
>        mNext(nullptr)
>  {
> +    *rv = InitStub(GetClass()->GetIID());

Please add a comment saying that we need to press on here even in the case of failure to make sure that things are the right state when we get destroyed.

Comment 5

[Benjamin Smedberg]

Attachment: 1069518-xptcall-notxpcom

Updated to comments and to remove CRLF line endings.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b41521572026

Comment 8

[Makoto Kato [:m_kato]]

When addon developer or XPCOM developer implements XPCOM by JS, it may not work by this fix.  I think this should be marked by dev-doc-needed flag.

If unnecessary, please clear this flag.

Comment 9

[[:MakeMyDay]]

Indeed, this one broke the icaljs backend in Lightning. See bug 1081534. An update of documentation would be appreciated.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Also, is there a way to make logging for this type of error more obvious? All we get is this, without further explaination.

Error: NS_ERROR_XPC_CI_RETURNED_FAILURE: Component returned failure code: 0x80570015 (NS_ERROR_XPC_CI_RETURNED_FAILURE) [nsIJSCID.createInstance]

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Philipp Kewisch [:Fallen] from comment #10)
> Also, is there a way to make logging for this type of error more obvious?
> All we get is this, without further explaination.
> 
> Error: NS_ERROR_XPC_CI_RETURNED_FAILURE: Component returned failure code:
> 0x80570015 (NS_ERROR_XPC_CI_RETURNED_FAILURE) [nsIJSCID.createInstance]

What would you change about the logging that's already there:

https://hg.mozilla.org/mozilla-central/rev/b41521572026#l3.30

?

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I don't think it should be restricted to debug builds.