1754037 - Use a Message{Reader,Writer} argument instead of IPC::Message in [IPDL]ParamTraits implementations

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

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

This is the first step towards bug 1754009, and is being written/landed first as it requires a bulk rewrite of every ParamTraits implementation in the tree, which will be difficult to build the IPDLParamTraits/ParamTraits merge patches on top of while they are changing.

Comment 1

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

We want to make these changes and re-integrate the two ParamTraits types because of the maintenance and flexibility burden it has become to have these two parallel systems. We already have a few places where people want to serialize a type declared in IPDL from within a context where an actor is not avaliable, and they pass nullptr for the actor argument (e.g. https://searchfox.org/mozilla-central/rev/7a1beb3d3ded4b9b356b590ceb9a50e44e01c957/dom/ipc/CSPMessageUtils.cpp#26, or https://searchfox.org/mozilla-central/rev/7a1beb3d3ded4b9b356b590ceb9a50e44e01c957/ipc/glue/ForkServiceChild.cpp#76). In addition, this makes it more difficult for potential future improvements to IPDL, such as support for sending messages without an actor.

With the re-integrated API, the actor is implicitly passed around behind the Message{Reader,Writer} type, and only a single interface type is needed.

This patch does not handle actually rewriting every instance of IPDLParamTraits, that will be handled in follow-ups (see bug 1754009).

Comment 2

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

Attachment: Bug 1754037 - Part 1: Introduce MessageWriter and MessageReader types, r=#ipc-reviewers

These types will replace IPC::Message in ParamTraits method arguments, and
combine the IPC::Message*, PickleIterator and IProtocol* arguments
together into a single argument.

In a future patch this will be used to remove IPDLParamTraits and merge it back
with ParamTraits.

Comment 3

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

Attachment: Bug 1754037 - Part 2: Forward declare Message{Reader,Writer} where Message is forward declared, r=#ipc-reviewers

This will allow the types to be more easily automatically substituted in place
of IPC::Message, as the type will already be declared in places it is used.

Depends on D139999

Comment 4

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

Attachment: Bug 1754037 - Part 3a: Manual changes to new ParamTraits API, r=#ipc-reviewers

This change does not build without the automatically rewritten changes from
part 3c, as every IPC::ParamTraits and IPDLParamTraits implementation needs to
be updated at once, but these are the manual changes which are required and not
handled by the automatic script.

Depends on D140000

Comment 5

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

Attachment: Bug 1754037 - Part 3b: Update IPDL generated code to new ParamTraits API, r=#ipc-reviewers

Updates to the IPDL code generator to make the generated code correctly use the
new ParamTraits APIs for serialization and deserialization.

Depends on D140001

Comment 6

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

Attachment: Bug 1754037 - DO NOT LAND: Script used to automatically rewrite all ParamTraits implementations, r=#ipc-reviewers

This sketchy script is used to automatically rewrite the tree to update all
ParamTraits implementations in part 3c. It will not be checked into the tree.

Depends on D140002

Comment 7

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

Attachment: Bug 1754037 - Part 3c: Automatically update all ParamTraits implementations, r=#ipc-reviewers

Automatically generated rewrites of all ParamTraits and IPDLParamTraits
implementations in-tree to use IPC::Message{Reader,Writer}.

Depends on D140003

Comment 8

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

Attachment: Bug 1754037 - Part 4: Implement IPDLParamTraits in terms of ParamTraits, r=#ipc-reviewers

This flips around the relationship between IPDLParamTraits and ParamTraits so
that callers can always use ParamTraits instead of IPDLParamTraits.

Depends on D140004

Comment 9

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

Attachment: Bug 1754037 - Part 5: Update IPC documentation for ParamTraits, r=#ipc-reviewers

The documentation now reflects the fact that IPDLParamTraits is no longer
necessary.

Depends on D140005

Comment 10

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

Attachment: Bug 1754037 - Part 6: Add a FatalError method to Message{Reader,Writer}, r=#ipc-reviewers

This will take the place of the existing uses of aActor->FatalError in
ipdlc-generated ParamTraits implementations, and will function without an
actor.

Depends on D140006

Comment 11

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

Attachment: Bug 1754037 - Part 7: Add missing namespaces, r=#ipc-reviewers

This was not producing errors as IPDLParamTraits was in the mozilla::ipc
namespace, however once ipdlc uses IPC::ParamTraits again, it will.

Depends on D140007

Comment 12

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

Attachment: Bug 1754037 - Part 8: Switch ipdlc to use ParamTraits instead of IPDLParamTraits, r=#ipc-reviewers

Depends on D140008

Comment 13

[Andrew McCreight [:mccr8]]

It looks like your patches are getting rid of the uses of Pickle::EndRead(). Is there a strong reason for doing this? I see that right now that it doesn't do anything, but in the past it has been used for an assertion about the iterator and for telemetry related to deserialization, so I feel like there might be some value in keeping it around. It looks like all that would be required would be to change the EndRead() calls to a new method MessageReader::End(). It doesn't even need any arguments. So it seems to me that keeping it around for now would be a lot easier than trying to pick out all of the places it would need to be added back in the future, but I will admit I've never heard of this function before, so I might be wrong.

If you want to go ahead with removing it, this change should be mentioned, along with an explanation, somewhere in the patch descriptions. I think you should also actually remove Pickle::EndRead() when you can (maybe in part 3b?).

Comment 14

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

Ah, I completely forgot about that. The main reason why I decided to remove the explicit calls to EndRead was that they roughly mapped to the lifetime of the MessageReader type, so they seemed redundant with adding a destructor to that type, though it's slightly different. I should probably remove it in a separate bug.

Comment 15

[Andrew McCreight [:mccr8]]

Removing it in a separate bug sounds good. Yeah, it is a little unfortunate that it isn't QUITE the same as the lifetime of MessageReader. Though in some cases, like ParseForkNewSubprocess, the extra stuff that happens after does seem reasonable to count against deserialization, but in the case of the generated code, the lifetime includes the Recv call which is not great. Of course, that could be worked around by adding an admittedly-ugly set of brackets around the actual deserialization part of the code. It looks like it wouldn't even be a huge change to the code generator, because the deserialization stuff is all produced in a big glob anyways.

Comment 16

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

Yeah, I think I'm just going to leave it in for now, and if we end up wanting to use it for something in the future, we can look into tweaking it at that point.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bce00e0e8dbf
Part 1: Introduce MessageWriter and MessageReader types, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/963b51f1dd01
Part 2: Forward declare Message{Reader,Writer} where Message is forward declared, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/3d088859f028
Part 3a: Manual changes to new ParamTraits API, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/aab84765766f
Part 3b: Update IPDL generated code to new ParamTraits API, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/7d894a317258
Part 3c: Automatically update all ParamTraits implementations, r=ipc-reviewers,media-playback-reviewers,bryce,mccr8
https://hg.mozilla.org/integration/autoland/rev/75133cd4e108
Part 4: Implement IPDLParamTraits in terms of ParamTraits, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/6ea7b76b26af
Part 5: Update IPC documentation for ParamTraits, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/3bded54dc0c2
Part 6: Add a FatalError method to Message{Reader,Writer}, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/66ff52fba040
Part 7: Add missing namespaces, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/74cffdcbe8a1
Part 8: Switch ipdlc to use ParamTraits instead of IPDLParamTraits, r=ipc-reviewers,mccr8

Comment 18

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/bce00e0e8dbf
https://hg.mozilla.org/mozilla-central/rev/963b51f1dd01
https://hg.mozilla.org/mozilla-central/rev/3d088859f028
https://hg.mozilla.org/mozilla-central/rev/aab84765766f
https://hg.mozilla.org/mozilla-central/rev/7d894a317258
https://hg.mozilla.org/mozilla-central/rev/75133cd4e108
https://hg.mozilla.org/mozilla-central/rev/6ea7b76b26af
https://hg.mozilla.org/mozilla-central/rev/3bded54dc0c2
https://hg.mozilla.org/mozilla-central/rev/66ff52fba040
https://hg.mozilla.org/mozilla-central/rev/74cffdcbe8a1