780970 - Add [infallible] attribute to XPIDL

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Justin Lebar (not reading bugmail)]

I don't think it would be particularly hard to add an [infallible] attribute to XPIDL.  This would translate the following IDL

  [infallible] readonly attribute long foo;

to either the following C++

  // Option A
  NS_IMETHOD PRInt32 GetFoo() = 0;
  PRInt32 GetFoo() {
    PRInt32 result = 0;
    MOZ_ASSERT(NS_SUCCEEDED(GetFoo(&result)));
    return result;
  }

or the following C++

  // Option B
  virtual PRInt32 GetFoo() = 0;
  nsresult GetFoo(PRInt32* aFoo) {
    NS_ENSURE_ARG_POINTER(aFoo);
    *aFoo = GetFoo();
    return NS_OK;
  }

This would make interacting with XPCOM interfaces from C++ /so much better/.

For starters, I'd like to implement this only for attributes which return primitives.  We could of course expand it to attributes returning objects, and to interface methods.

Option B is cleaner, so although it's marginally trickier than option A, I'm trying it now...

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: WIP v1

This seems to generate reasonable header files.  I haven't tested it.

I'm concerned because option B (which this patch implements) results in two
virtual calls for every attribute access, where we used to have only one.  But
glandium may make a saving throw.

Comment 2

[Benjamin Smedberg]

What's the goal here? Faster scriptable methods or just better C++ accessibility? Would this attribute imply that the interface is [builtinclass] (since any interface implemented by script may fail in the impl)?

If you change the virtual signature at all currently, the methods will not be scriptable (or will only be scriptable via quickstubs/new DOM bindings, not via long-form xpconnect). If you don't care about scriptability, why not just use [notxpcom]?

Changing xptcall to work with methods of the form

NS_IMETHOD_(void) Method(PRInt32* result);

would be relatively straightforward or perhaps wouldn't require any xptcall changes. Changing the actual return type would be a lot more complicated, especially for any non-primitive types that require allocation/resource management.

Comment 3

[Justin Lebar (not reading bugmail)]

> What's the goal here? Faster scriptable methods or just better C++ accessibility?

I'm only concerned with C++ ergonomics.  It's my understanding that we're moving away from XPCOM method dispatch wherever we care about performance.

> Would this 
> attribute imply that the interface is [builtinclass] (since any interface implemented by script 
> may fail in the impl)?

Option B would imply this, I guess, yes.

> Changing the actual return type would be a lot more complicated, especially for any 
> non-primitive types that require allocation/resource management.

I was thinking that we'd support [infallible] only on attributes which return primitives, to start with.

I talked with jst about this for a bit, and concluded that option A is probably the way to go.  Option B changes the class's vtable, so we'd have to make XPCOM aware of the changes.  Plus, option B doesn't work with interfaces implemented in JS, while option A works fine.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

This implements option A.  I'll attach an example of generated output in a moment.

Comment 5

[Justin Lebar (not reading bugmail)]

  /* [infallible] readonly attribute boolean isBrowserElement; */
  NS_IMETHOD GetIsBrowserElement(bool *aIsBrowserElement) = 0;
  bool GetIsBrowserElement()
  {
    bool result;
    nsresult rv = GetIsBrowserElement(&result);
    MOZ_ASSERT(NS_SUCCEEDED(rv));
    return result;
  }

Comment 6

[Mike Hommey [:glandium]]

(In reply to Justin Lebar [:jlebar] from comment #3)
> I talked with jst about this for a bit, and concluded that option A is
> probably the way to go.  Option B changes the class's vtable, so we'd have
> to make XPCOM aware of the changes.  Plus, option B doesn't work with
> interfaces implemented in JS, while option A works fine.

More than that, it would add entries to the vtable. Option A has the advantage of being inlinable and, in the end, would yield more or less the same code (module the MOZ_ASSERT) as what it would replace. I couldn't think of anything better for what you were after. I was confused with the way you wrote option A initially, though.

Comment 7

[Benjamin Smedberg]

Comment on attachment 649946 [details] [diff] [review]
Patch, v1

Yeah, I was confused about option A also. I think this is ok, but I do want [infallible] to require that the interface be marked [builtinclass] in xpidl.py

Comment 8

[Justin Lebar (not reading bugmail)]

> but I do want [infallible] to require that the interface be marked [builtinclass] in xpidl.py

Under what circumstances might JS code that doesn't (transitively across invoked code) throw an exception cause us to return an nserror code?

Comment 9

[Benjamin Smedberg]

Well, you can sometimes make xpconnect throw an exception (perhaps by holding a reference to a dead compartment), but throwing a JS exception is how you normally end up with failure nsresults from scripted interfaces...

Comment 10

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v2

Don't allow [infallible] on non-[builtinclass] interfaces.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 650583 [details] [diff] [review]
Patch, v2

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

::: xpcom/idl-parser/header.py
@@ +296,5 @@
> +  {
> +    %(realtype)sresult;
> +    nsresult rv = %(nativename)s(%(argnames)s&result);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +    return result;

This will warn. You could use MOZ_ALWAYS_TRUE or DebugOnly

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 650583 [details] [diff] [review]
Patch, v2

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

::: xpcom/idl-parser/header.py
@@ +291,5 @@
>  
>  """
>  
> +attr_infallible_tmpl = """\
> +  %(realtype)s%(nativename)s(%(args)s)

Please make this 'inline'.

Comment 13

[Justin Lebar (not reading bugmail)]

I can land this once we get bug 784436, which actually /uses/ [infallible], taken care of.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6c7efe053241