1346919 - Document the runnable/function helpers in nsThreadUtils.h better

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[(no longer active)]

It seems like we should be able to make it easier to understand how to use helpers like NewRunnableMethod with better documentation, especially when it comes to how to pass arguments.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: add documentation for NewRunnableMethod; r=erahm,f=ehsan

Ehsan was complaining about this earlier this week.  These comments reflect my
understanding of how things work.

(We should probably just get rid of the zero-argument overloads, as they're
subsumed by the variadic templates.)

Comment 2

[(no longer active)]

Comment on attachment 8847260 [details] [diff] [review]
add documentation for NewRunnableMethod; r=erahm,f=ehsan

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

This is great, thank you, and my apologies for such a long delay in getting back to you!

::: xpcom/threads/nsThreadUtils.h
@@ +955,5 @@
> +// you are 100% certain that `myObject` will live long enough, you can
> +// use NewNonOwningRunnableMethod instead, which will, as its name implies,
> +// take a non-owning reference.  The use of this function is discouraged,
> +// as it can easily lead to use-after-free errors and security
> +// vulnerabilities.

Please add something here saying that usage of NewNonOwningRunnableMethod should be accompanied with some kind of proof explaining how the invariants ensuring the lifetime of `myObject` at the time the patch is written and also until the end of time and that it should be restricted only to cases where there is a profile available showing that the virtual calls to AddRef and Release can be a performance problem.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (busy) from comment #2)
> Please add something here saying that usage of NewNonOwningRunnableMethod
> should be accompanied with some kind of proof explaining how the invariants
> ensuring the lifetime of `myObject` at the time the patch is written and
> also until the end of time and that it should be restricted only to cases
> where there is a profile available showing that the virtual calls to AddRef
> and Release can be a performance problem.

I will add the bit about a proof explanation, but given that NewNonOwningRunnableMethod can be used with non-refcounted classes, I don't think the second part is appropriate.  WDYT?

Comment 4

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> (In reply to :Ehsan Akhgari (busy) from comment #2)
> > Please add something here saying that usage of NewNonOwningRunnableMethod
> > should be accompanied with some kind of proof explaining how the invariants
> > ensuring the lifetime of `myObject` at the time the patch is written and
> > also until the end of time and that it should be restricted only to cases
> > where there is a profile available showing that the virtual calls to AddRef
> > and Release can be a performance problem.
> 
> I will add the bit about a proof explanation, but given that
> NewNonOwningRunnableMethod can be used with non-refcounted classes, I don't
> think the second part is appropriate.  WDYT?

Oh right.  I still think we should include that part but we should qualify it with "if the type is refcounted" and mention that otherwise this restriction doesn't apply.  My intention is to scare people off of using this in the common case for refcounted type, since in my experience, almost every time when the programmer thinks an object lives long enough, they're wrong.  :-)

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8847260 [details] [diff] [review]
add documentation for NewRunnableMethod; r=erahm,f=ehsan

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

::: xpcom/threads/nsThreadUtils.h
@@ +975,5 @@
> +// you would if you were writing out the class by hand:
> +//
> +//   nsCOMPtr<nsIRunnable> event =
> +//     mozilla::NewRunnableMethod<RefPtr<T>, nsTArray<U>>
> +//         (myObject, &MyClass::HandleEvent, arg1, arg2);

Thanks for adding this, the whole storage bit was hard to understand by just looking at the code.

@@ +993,5 @@
> +//
> +// Finally, all of the functions discussed above have additional overloads
> +// that take a `const char*` as their first parameter.  If provided, this
> +// string specifies the "name" of the runnable, which can be used by
> +// various introspection tools in the browser.

We should probably emphasize that this is the preferred method for new code.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: add documentation for NewRunnableMethod; r=erahm,f=ehsan

Updated per comments and added a few bits and pieces.  Probably worth a
re-review.

Comment 7

[Eric Rahm [:erahm]]

Comment on attachment 8854946 [details] [diff] [review]
add documentation for NewRunnableMethod; r=erahm,f=ehsan

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

Looks good, just a comment about comment placement.

::: xpcom/threads/nsThreadUtils.h
@@ +999,5 @@
> +//
> +//
> +//   RefPtr<T> ptr = ...;
> +//   nsTArray<U> array = ...;
> +//   nsCOMPtr<nsIRunnable> event = 

nit: trailing space

@@ +1026,5 @@
> +//
> +// Finally, all of the functions discussed above have additional overloads
> +// that do not take a `const char*` as their first parameter; you may see
> +// these in older code.  The `const char*` overload is preferred and
> +// should be used in new code exclusively.

This is great, my only thought is that maybe we should move it before all the detail stuff (so around line 305ish) and then down here we could just have a small comment referring back.

We could do it the other way too: put a small comment for this section before the detail block and then refer to the detailed comment at NewRunnableMethod.

TBH it would be nice to move all the detail stuff to a separate header, it's really hard to browse this header and understand what the API is. I don't know how doable that is and certainly it would be for a follow up.

Comment 8

[Nathan Froyd [:froydnj]]

I am inclined to leave the comment where it is, as I suspect most people will find their way to NewRunnableMethod through grep/dxr/searchfox/etc.  If the header was more standalone, I think the case for putting it at the top, before the mozilla::detail:: bits would be stronger.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9284a56b46d6
add documentation for NewRunnableMethod; r=erahm; f=ehsan

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9284a56b46d6