Add [infallible] attribute to XPIDL

RESOLVED FIXED in mozilla17

Status

()

defect
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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...
Posted patch WIP v1 (obsolete) — Splinter Review
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

7 years ago
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.
> 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.
Posted patch Patch, v1 (obsolete) — Splinter Review
This implements option A.  I'll attach an example of generated output in a moment.
Attachment #649946 - Flags: review?(khuey)
  /* [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;
  }
(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

7 years ago
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
> 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

7 years ago
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...
Assignee: nobody → justin.lebar+bug
Posted patch Patch, v2Splinter Review
Don't allow [infallible] on non-[builtinclass] interfaces.
Attachment #650583 - Flags: review?(ted.mielczarek)
Attachment #649777 - Attachment is obsolete: true
Attachment #649946 - Attachment is obsolete: true
Attachment #649946 - Flags: review?(khuey)
Attachment #650583 - Flags: review?(ted.mielczarek) → review?(khuey)
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 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'.
Attachment #650583 - Flags: review?(khuey) → review+
Blocks: 784436
I can land this once we get bug 784436, which actually /uses/ [infallible], taken care of.
https://hg.mozilla.org/mozilla-central/rev/6c7efe053241
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.