1775062 - Consider avoiding requiring default constructors for IPDL unions.

Status: RESOLVED FIXED

(Core :: IPC, task)

Description

[Emilio Cobos Álvarez (:emilio)]

We need to manually define default constructors in the relevant places to keep IPDL happy, but it seems that they shouldn't be needed.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1775062 - Implement ipdl union operator= by using move/copy constructors. r=nika

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1775062 - Implement ipdl reads without needing default ctors. r=nika

Depends on D149742

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Nika, wdyt about something like comment 1? With that the only other default ctor is in the ipdl reads:

            mozilla::StyleOffsetRotate tmp = mozilla::StyleOffsetRotate();       
            (*(aVar)) = std::move(tmp);
            if ((!(IPC::ReadParam(aReader, (&((aVar)->get_StyleOffsetRotate())))))) {
                aReader->FatalError("Error deserializing variant TStyleOffsetRotate of union Animatable");
                return false;                                                    
            }

I was thinking of doing something like:

TrackedRead<mozilla::StyleOffsetRotate> tmp;
if (!IPC::ReadParam(aReader, &tmp)) {
  return false;
}
(*(aVar)) = std::move(*tmp.addr());

Or something, where TrackedRead is basically an AlignedStorage2 which tracks whether it holds a valid value.

But that's a bit of code to implement plus avoids the initialization of C++ objects, so it'd probably need to be opt-in... Do you have any thoughts on this?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Some relevant chat with nika:

emilio: Yeah indeed. I suspect ~all C++ deserializers would rely on the data being already initialized
nika: Yeah
emilio: I guess we could change ReadParam to work in terms of returning Maybe<T> or something
nika: The big problem is that the API was designed pre-c++11
So std::move didn't exist
emilio: I see
nika: but now that it and guaranteed NRVO exists, we can just return Maybe<T>
But rewriting every impl would be a nightmare
It might be possible to use overload sets to shim from one to the other
Yeah, that might work, but again, jank :p
(also probably issues with non-moveable types) (or copy-only types)
emilio: Do we really have non-moveable types? I know we have move-only types but...
nika: we have plenty of non-moveable types (every refcounted type for one :p)
If you want to give it a shot, you could probably do it all in https://searchfox.org/mozilla-central/rev/c30349265c87047a324a471d2f39a216b6749262/ipc/chromium/src/chrome/common/ipc_message_utils.h#296-299
emilio: I can try, thanks
nika: You'd need 4 overloads, and some SFINAE
ReadParam(MessageReader*, P*) for the old case + new case, and P ReadParam(MessageReader*) for the new case
Oh, I know a problem you'll run into https://searchfox.org/mozilla-central/rev/c30349265c87047a324a471d2f39a216b6749262/ipc/chromium/src/chrome/common/ipc_message_utils.h#715-742 - these forward to T* methods
but we definitely don't want to return raw pointers
You'll definitely need some SFINAE magic to handle those cases
Maybe just get a proof of concept together and we can chat again to make sure
it's not too gross or going to run into too many problems first

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Okay, I think what I came up with is not too gross. Nika, thoughts welcome, thanks!

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

Okay, I think what I came up with is not too gross. Nika, thoughts welcome, thanks!

I'll probably get back to this once I'm back from PTO - not going to have time to look into it today. Thanks for looking into this :-)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: WIP: Bug 1775062 - Rather hacky progress towards using Maybe<T> for reads in ipdl.

Lots of code can be cleaned up. Some of the stuff should be split to
separate patches, but just for reference.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ef480aa9f4b
Implement ipdl union operator= by using move/copy constructors. r=nika

Comment 9

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/9ef480aa9f4b

Comment 10

[Nika Layzell [:nika] (ni? for response)]

These patches came up again when I was thinking about how nice it'd be to support NotNull<T> types in IPDL rather than the current situation where most pointer-like types are nullable by default. This would be especially nice for when we switch code from e.g. PrincipalInfo to nsIPrincipal now that it is threadsafe, as the new type is now nullable and so needs extra checks.

This wouldn't necessarily fully handle the situation, as Maybe<NotNull<RefPtr<T>>> is only copyable, not movable, and Maybe<NotNull<UniquePtr<T>>> isn't copyable or movable, because NotNull can only be copied, not moved (https://searchfox.org/mozilla-central/rev/c5eeb9f4fcfe52b028e7774054444f2982feccc3/mfbt/NotNull.h#176-181). If we want to support it, we'd probably need to add some special casing for Maybe<NotNull<T>> to allow the type to be moved in that case, sort-of like how we have special handling for Maybe<T&> (https://searchfox.org/mozilla-central/rev/c5eeb9f4fcfe52b028e7774054444f2982feccc3/mfbt/NotNull.h#176-181), though that may be unnecessary.

We'd also need to start using the new ReadParam signature for reading parameters in situations like nsTArray - though we wouldn't want to regress performance for POD types in the process (I have a feeling the API for ReadSequenceParam might get a bit more complex unfortunately, as that depends on default-constructors right now).

Chatted with :emilio a bit on matrix about it, and leaving a ni? as a reminder to rebase & fix review comments at some point.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1775062 - Fix various using statements to use properly qualified names. r=nika

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0c90102908a
Fix various using statements to use properly qualified names. r=nika,necko-reviewers,valentin

Comment 13

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/f0c90102908a

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05923c6cf455
Implement ipdl reads without needing default ctors. r=nika

Comment 15

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/05923c6cf455