This patchset takes the following approach to handling refcounted types in IPDL: Add a new syntax for importing refcounted structs/classes (Part 2): using refcounted {struct,class} nsClass from "some/header/File.h"; This type can be used in IPDL like any other type: struct Foo { nsClass myField; }; protocol PFoo { /* ... */ async Go(nsClass aArg); } When the IPDL is compiled, the refcounted type is compiled to the C++ type RefPtr<T>. The `in` type is updated as well to be a `T*` instead of a `const RefPtr<T>&` for ergonomics reasons. Defining ParamTraits on these types is also changed. I made a new template helper class ParamTraitsSelector<T> which strips off the outermost pointer to select the inner type in the case of a reference counted type. Namely: RefPtr<T> -> T nsCOMPtr<T> -> T T* -> T T -> T This helper is used in both ipc_message_utils.h and IPDLParamTraits.h to select the ParamTraits impl in {Read,Write}{IPDL,}Param (Part 3A). This change also affected a few existing ParamTraits impls: 1. Actor ParamTraits impls had to be updated to be IPDLParamTraits<PActorChild> instead of IPDLParamTraits<PActorChild*> - this was a very small code generator change (Part 3F) 2. GeoLocation and nsIAlertNotification were using hacky workarounds to serialize reference counted types over IPC. Namely they were implementing ParamTraits<RefCountedType*> and simply either never addrefing the outparam or always addrefing it and assuming the caller will Do The Right Thing. I fixed these code paths to use the new system, making them safer and a bit less scary. (Parts 3B and 3C). 3. In previous bugs I've made nsCOMPtr<nsIInputStream> and RefPtr<mozilla::dom::BlobImpl> serializable through a somewhat hacky mechanism, and I moved these types over to the new mechanism (Parts 3D and 3E). Part 1 is necessary as the modified callsite was used to pass arguments as inputs to IPDL functions. This would mean that we would try to assign a Move(RefPtr<T>) into a T*, which uses a deleted operator T*() && on RefPtr. Not calling Move() when it is a no-op avoids this. --- N.B. The tree doesn't build within Part 3A-F, so they will be squashed in the final push.

MozReview-Commit-ID: BcX6aPdXdSD

MozReview-Commit-ID: FZQpDCPfAJF

MozReview-Commit-ID: GRrhaaXUzIZ

MozReview-Commit-ID: 1Ih4XLN7ok2

MozReview-Commit-ID: 3X0NEoEnnMf

MozReview-Commit-ID: 4VoawMZJrW8

MozReview-Commit-ID: FDUqCdmjnHV

MozReview-Commit-ID: IAb1iTm4RJu

(N.B. Another feature which we could consider adding in the future here would be support for the `nullable` attribute on refcounted types. The IPDL layer would then do the same thing it does with this attribute for Actors, which is check after deserializing (and before sending) if the type is null and the `nullable` attribute is not set.

So just to make sure I understand the semantics... right now are the T* passed in guaranteed non-null? Or is it basically "whatever caller did"?

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10) > So just to make sure I understand the semantics... right now are the T* > passed in guaranteed non-null? Or is it basically "whatever caller did"? They are not guaranteed to be non-null right now. All of the serializers I've written so far write a isNull tag out at the beginning because of that. I don't know if it's worth coming up with other tools for handling this. For example, I _could_ say that they must be non-null, and do the null check in IPDL for nullable types, but that would add complexity.

I should also note that some serializers (e.g. BlobImpl) will send a null object across the pipe even if the original object is not-null if the value can't be serialized due to a non-fatal error. That doesn't work super nicely with ensuring non-nullability.

OK. But I guess my issue was that looking at the patches some of the Recv* methods assume the arg is non-null. What allows them to assume that? I guess if Send* always sends non-null that's a start, but even then comment 12 says you might get null?

Comment on attachment 8957003 [details] [diff] [review] Part 3B: Move nsIAlertNotification serialization to the refcounted system >+ContentParent::RecvShowAlert(nsIAlertNotification* aAlert) Please keep the null-check for aAlert unless there's some way to prove it's not null. r=me with that.

Comment on attachment 8957004 [details] [diff] [review] Part 3C: Move geolocation serialization to the refcounted system > *aResult = new nsGeoPosition(coords, timeStamp); This will do an extra addref/release. If we care, this is the only caller of this constructor signature and we could change it to take an already_AddRefed or whatnot. > ContentParent::HandleEvent(nsIDOMGeoPosition* postion) Want to fix the typo on the arg name while you're here? r=me

Comment on attachment 8957001 [details] [diff] [review] Part 2: Support parsing `using refcounted class` imports in IPDL Review of attachment 8957001 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/ipdl/ipdl/lower.py @@ +526,5 @@ > t.ref = 1 > return t > + if ipdltype.isCxx() and ipdltype.isRefcounted(): > + # Use T* instead of const RefPtr<T>& > + t = t.T Does this need to perform some kind of clone or deep copy, so we're not modifying t.T? Oh, no, because t is some freshly allocated thing, and we're passing back the inner type anyway, which is...also freshly allocated? OK.

Comment on attachment 8957000 [details] [diff] [review] Part 1: Only Move() arguments when necessary Review of attachment 8957000 [details] [diff] [review]: ----------------------------------------------------------------- Canceling review for patches without a decent commit message or explanation.

Comment on attachment 8957002 [details] [diff] [review] Part 3A: Strip pointers from the argument to WriteParam and WriteIPDLParam before selecting the ParamTraits impl Review of attachment 8957002 [details] [diff] [review]: ----------------------------------------------------------------- I think this is reasonable. What happens if people mistakenly write IPDLParamTraits like: Read(..., T* out) where T is actually refcounted (or maybe even T**, expecting XPCOM outparam semantics)? Are the error messages reasonable? ::: ipc/chromium/src/chrome/common/ipc_message_utils.h @@ +118,5 @@ > +struct StripPointers<T*> { typedef T Type; }; > +template<typename T> > +struct StripPointers<RefPtr<T>> { typedef T Type; }; > +template<typename T> > +struct StripPointers<nsCOMPtr<T>> { typedef T Type; }; We have written this helper sooo many times. =/

Comment on attachment 8957000 [details] [diff] [review] Part 1: Only Move() arguments when necessary I should've put it in the commit message, but there was an explanation in comment 0: -- Part 1 is necessary as the modified callsite was used to pass arguments as inputs to IPDL functions. This would mean that we would try to assign a Move(RefPtr<T>) into a T*, which uses a deleted operator T*() && on RefPtr. Not calling Move() when it is a no-op avoids this. -- Basically, this makes us call Move(X) only if the argument is not a const ref. ParamTraits<T>::Write(Message*, T*) is the signature for these XPCOM Write methods, and calling Move() on a RefPtr causes the operator T* to be deleted.

Comment on attachment 8957000 [details] [diff] [review] Part 1: Only Move() arguments when necessary Review of attachment 8957000 [details] [diff] [review]: ----------------------------------------------------------------- I am sort of willing to believe that this patch actually works for you, but I am quite confused as to *how*... ::: ipc/ipdl/ipdl/lower.py @@ +594,5 @@ > > + def mayMoveExpr(self): > + if self.isCopyable(): > + return self.var() > + return ExprMove(self.var()) OK, I see the explanation now. This condition reads a little *too* generally, though; one could have a copyable type that is also movable, and then we'd wind up doing something inefficient for the movable semantics case below? But isCopyable() is just: not _cxxTypeNeedsMove(...) and _cxxTypeNeedsMove(...) returns true iff the passed in type is actually an ipdl type. So doesn't isCopyable catch *way* too many cases here?

Comment on attachment 8957007 [details] [diff] [review] Part 3F: Correctly implement ParamTraits for actors after the ParamTraits changes Review of attachment 8957007 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense.

(In reply to Nathan Froyd [:froydnj] from comment #20) > Comment on attachment 8957000 [details] [diff] [review] > Part 1: Only Move() arguments when necessary > > Review of attachment 8957000 [details] [diff] [review]: > ----------------------------------------------------------------- > > I am sort of willing to believe that this patch actually works for you, but > I am quite confused as to *how*... > > ::: ipc/ipdl/ipdl/lower.py > @@ +594,5 @@ > > > > + def mayMoveExpr(self): > > + if self.isCopyable(): > > + return self.var() > > + return ExprMove(self.var()) > > OK, I see the explanation now. > > This condition reads a little *too* generally, though; one could have a > copyable type that is also movable, and then we'd wind up doing something > inefficient for the movable semantics case below? But isCopyable() is just: > > not _cxxTypeNeedsMove(...) > > and _cxxTypeNeedsMove(...) returns true iff the passed in type is actually > an ipdl type. So doesn't isCopyable catch *way* too many cases here? So, yes and no. This is an example of how lower.py is immensely confusing because the names don't actually match what they mean or make any sense... isCopyable() would definitely seem to catch too many cases, however there are some design decisions which were made in the earlier days of IPDL which mean that it doesn't matter. Currently, every single type which isn't explicitly marked as _cxxTypeNeedsMove() always has its parameter type defined as `const T&` (unless there is an override in _cxxConstRefType https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/ipc/ipdl/ipdl/lower.py#510-527). This means that the function arguments where we're passing the value which we're Move(...)-ing for all types which don't have _cxxTypeNeedsMove() set are all const T& types, meaning that the Move() serves no purpose.

Pushed by nika@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2973a36696 Part 1: Only Move() arguments when necessary, r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/c313243cda55 Part 2: Support parsing `using refcounted class` imports in IPDL, r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/7f79db23bc0f Part 3: Add support for RefCounted types to IPDL, r=bz,froydnj,baku

https://hg.mozilla.org/mozilla-central/rev/dc2973a36696 https://hg.mozilla.org/mozilla-central/rev/c313243cda55 https://hg.mozilla.org/mozilla-central/rev/7f79db23bc0f