Closed Bug 1069518 Opened 5 years ago Closed 5 years ago

XPTCall should refuse to implement interfaces with [notxpcom] methods

Categories

(Core :: XPCOM, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

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.
Attached patch 1069518-xptcall-notxpcom (obsolete) — Splinter Review
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.
Attached patch 1069518-xptcall-notxpcom (obsolete) — Splinter Review
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.
Attachment #8491703 - Attachment is obsolete: true
Attachment #8492305 - Flags: review?(nfroyd)
Attachment #8492305 - Flags: review?(bobbyholley)
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 ();
};

?
Attachment #8492305 - Flags: review?(nfroyd) → review+
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.
Attachment #8492305 - Flags: review?(bobbyholley) → review+
Updated to comments and to remove CRLF line endings.
Attachment #8492305 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b41521572026
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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.
Indeed, this one broke the icaljs backend in Lightning. See bug 1081534. An update of documentation would be appreciated.
Depends on: 1081534
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]
(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

?
I don't think it should be restricted to debug builds.
You need to log in before you can comment on or make changes to this bug.