Last Comment Bug 780970 - Add [infallible] attribute to XPIDL
: Add [infallible] attribute to XPIDL
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks: 784436
  Show dependency treegraph
 
Reported: 2012-08-07 12:41 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-08-23 11:09 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v1 (9.37 KB, patch)
2012-08-07 13:32 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch, v1 (5.60 KB, patch)
2012-08-07 20:31 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch, v2 (5.84 KB, patch)
2012-08-09 08:42 PDT, Justin Lebar (not reading bugmail)
khuey: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-08-07 12:41:12 PDT
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) 2012-08-07 13:32:26 PDT
Created attachment 649777 [details] [diff] [review]
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 [:bsmedberg] 2012-08-07 14:21:01 PDT
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) 2012-08-07 18:34:59 PDT
> 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) 2012-08-07 20:31:13 PDT
Created attachment 649946 [details] [diff] [review]
Patch, v1

This implements option A.  I'll attach an example of generated output in a moment.
Comment 5 Justin Lebar (not reading bugmail) 2012-08-07 20:32:11 PDT
  /* [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] 2012-08-07 23:07:00 PDT
(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 [:bsmedberg] 2012-08-08 13:56:33 PDT
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) 2012-08-08 14:03:11 PDT
> 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 [:bsmedberg] 2012-08-08 14:25:03 PDT
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) 2012-08-09 08:42:20 PDT
Created attachment 650583 [details] [diff] [review]
Patch, v2

Don't allow [infallible] on non-[builtinclass] interfaces.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2012-08-13 11:01:40 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-08-21 09:33:44 PDT
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) 2012-08-21 17:06:20 PDT
I can land this once we get bug 784436, which actually /uses/ [infallible], taken care of.
Comment 14 Ed Morley [:emorley] 2012-08-23 03:45:02 PDT
https://hg.mozilla.org/mozilla-central/rev/6c7efe053241

Note You need to log in before you can comment on or make changes to this bug.