Closed Bug 1754037 Opened 5 months ago Closed 4 months ago

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

Categories

(Core :: IPC, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
99 Branch
Tracking Status
firefox99 --- fixed

People

(Reporter: nika, Assigned: nika)

References

(Blocks 2 open bugs)

Details

Attachments

(10 files, 1 obsolete file)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

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.

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).

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.

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

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

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

Depends on D140001

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

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

Depends on D140003

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

Depends on D140004

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

Depends on D140005

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

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

Blocks: 1757802

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?).

Flags: needinfo?(nika)

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.

Flags: needinfo?(nika)

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.

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.

Attachment #9265964 - Attachment is obsolete: true
Pushed by nlayzell@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.