1440511 - Switch to using ParamTraits instead of per-actor picklers for IPDL message pickling

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P1)

Description

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

Pickling some types (such as actors, shared memory, etc.) require knowledge of the actor which they are being pickled for in order to serialize them. To move these pickling implementations to ParamTraits, we add a for_actor_ member on the IPC::Message type which tracks what type of actor to use for this purpose.

This allows picklers which need to know what actor they're being sent over to work implicitly. This will also be super handy for pickling types such as Blobs, as these need to construct actors to send over input streams.

I don't add any new picklers in this patch set, as it's large enough already, but I will write a follow-up bug to improve the ergonomics of pickling nsIInputStreams etc.

This patch also allows IPDL structs to be used in the implementation of other IPDL ParamTraits serializers, as the serialization methods for these types are now exposed to non-actors. This should make it easier to write correct ParamTraits impls for complex existing types.

Comment 1

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

Attachment: Part 1: Remove unnecessary ParamTraits<> specialization declaration from HangDetails.h

MozReview-Commit-ID: COnMiadbCjn

Comment 2

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

Attachment: Part 2: Correctly namespace nsHttpResponseHead in NeckoChannelParams.ipdlh

MozReview-Commit-ID: DVkTwAmqmkc

Comment 3

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

Attachment: Part 3: Support specializations in _splitMethodDefn

MozReview-Commit-ID: JFYgyUr4I4G

Comment 4

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

Attachment: Part 4: Record the actor which the message is being serialized/deserialized for on IPC::Message

MozReview-Commit-ID: 9WtF3Sm9xD9

Comment 5

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

Attachment: Part 5: Add a ParamTraits implementation for FileDescriptor

MozReview-Commit-ID: R1CiT5IvAi

Comment 6

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

Attachment: Part 6: Move all serialization logic into the ParmaTraits impl for Shmem

MozReview-Commit-ID: DsrfSVPgoo1

Comment 7

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

Attachment: Part 7: Support passing the fq flag to _cxxBareType in more places

MozReview-Commit-ID: K1lkVr6KdZo

Comment 8

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

Attachment: Part 8: Include c++ includes in ipdlh generated .cpp files

This is important as these includes are often used to define ParamTraits
implementations which are needed to serialize struct members and
parameters. The place where the code which uses these implementations
will be moving from the protocol where they are used to the file where
the structs are defined, which means we need to includes these files
there.

MozReview-Commit-ID: H1wqgyv5mel

Comment 9

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

Attachment: Part 9: Include ParamTraits impls which are used in ipdlh files

MozReview-Commit-ID: 9AfzlhyTgsY

Comment 10

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

Attachment: Part 10: Add support for serializing objects which take paramType by non-const reference in arrays

MozReview-Commit-ID: 4XKZOqpqrC7

Comment 11

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

Attachment: Part 11: Implement ParamTraits for IPDL generated types

MozReview-Commit-ID: 2ljPFJKM9Fg

Comment 12

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

Attachment: Part 12: Switch to calling ParamTraits directly when pickling and unpickling messages

MozReview-Commit-ID: 1t4oIp5JWcU

Comment 13

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

Attachment: Part 13: Remove the now-unused per-actor pickling code

MozReview-Commit-ID: 5ASQu4ly0od

Comment 14

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

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=874704f9ccb18a6b4c04c15c5ac7437ba46537f1

Comment 15

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

Attachment: Part 1: Remove unnecessary ParamTraits<> specialization declaration from HangDetails.h

MozReview-Commit-ID: COnMiadbCjn

Comment 16

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

Attachment: Part 2: Correctly namespace nsHttpResponseHead in NeckoChannelParams.ipdlh

MozReview-Commit-ID: DVkTwAmqmkc

Comment 17

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

Attachment: Part 3: Support specializations in _splitMethodDefn

MozReview-Commit-ID: JFYgyUr4I4G

Comment 18

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

Attachment: Part 4: Record the actor which the message is being serialized/deserialized for on IPC::Message

MozReview-Commit-ID: 9WtF3Sm9xD9

Comment 19

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

Attachment: Part 5: Add a ParamTraits implementation for FileDescriptor

MozReview-Commit-ID: R1CiT5IvAi

Comment 20

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

Attachment: Part 6: Move all serialization logic into the ParmaTraits impl for Shmem

MozReview-Commit-ID: DsrfSVPgoo1

Comment 21

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

Attachment: Part 7: Support passing the fq flag to _cxxBareType in more places

MozReview-Commit-ID: K1lkVr6KdZo

Comment 22

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

Attachment: Part 8: Include c++ includes in ipdlh generated .cpp files

This is important as these includes are often used to define ParamTraits
implementations which are needed to serialize struct members and
parameters. The place where the code which uses these implementations
will be moving from the protocol where they are used to the file where
the structs are defined, which means we need to includes these files
there.

MozReview-Commit-ID: H1wqgyv5mel

Comment 23

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

Attachment: Part 9: Include ParamTraits impls which are used in ipdlh files

MozReview-Commit-ID: 9AfzlhyTgsY

Comment 24

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

Attachment: Part 10: Add support for serializing objects which take paramType by non-const reference in arrays

MozReview-Commit-ID: 4XKZOqpqrC7

Comment 25

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

Attachment: Part 11: Implement ParamTraits for IPDL generated types

MozReview-Commit-ID: 2ljPFJKM9Fg

Comment 26

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

Attachment: Part 12: Switch to calling ParamTraits directly when pickling and unpickling messages

MozReview-Commit-ID: 1t4oIp5JWcU

Comment 27

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

Attachment: Part 13: Remove the now-unused per-actor pickling code

MozReview-Commit-ID: 5ASQu4ly0od

Comment 28

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

I changed the way that actors are attached to messages to make it cleaner and more reliable, as that was the cause of the bug in the last try push. 

new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffa582be77f87bfa6f102c189e15294e225f35e2

Comment 29

[Nathan Froyd [:froydnj]]

Comment on attachment 8953584 [details] [diff] [review]
Part 3: Support specializations in _splitMethodDefn

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

::: ipc/ipdl/ipdl/cxx/ast.py
@@ +481,5 @@
>  
>  class MethodDecl(Node):
>      def __init__(self, name, params=[ ], ret=Type('void'),
>                   methodspec=MethodSpec.NONE, const=0, warn_unused=0,
> +                 force_inline=0, typeop=None, T=None, cls=None):

IIUC, it is never correct to provide both T and cls, but all other combinations may be provided.  Would you please assert that this is the case?

A short bit of documentation here would be ideal, but given that it would be so wildly inconsistent with the rest of the ipdl module...

Comment 30

[Nathan Froyd [:froydnj]]

Comment on attachment 8953588 [details] [diff] [review]
Part 7: Support passing the fq flag to _cxxBareType in more places

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

Am I correct in assuming that `fq` is ipdl for "fully qualified"?

Comment 31

[Nathan Froyd [:froydnj]]

Comment on attachment 8953589 [details] [diff] [review]
Part 8: Include c++ includes in ipdlh generated .cpp files

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

I have questions.

::: ipc/ipdl/ipdl/lower.py
@@ +1444,5 @@
>          self.cppfile = None      # what will become Protocol.cpp
>          self.cppIncludeHeaders = []
>          self.structUnionDefns = []
>          self.funcDefns = []
> +        self.protocolHeaders = []

This field is not actually used?  If it gets used in a later patch, please move it to that patch.

@@ +1537,5 @@
>      def visitBuiltinCxxInclude(self, inc):
>          self.hdrfile.addthing(CppDirective('include', '"'+ inc.file +'"'))
>  
> +    def visitCxxInclude(self, inc):
> +        self.cppIncludeHeaders.append(inc.file)

Are you sure about this?  I have vague memories of trying to put code like this here and getting into a world of pain.

@@ +1546,5 @@
>                      'include', '"'+ _ipdlhHeaderName(inc.tu) +'.h"'))
> +        else:
> +            self.cppIncludeHeaders += [
> +                _protocolHeaderName(inc.tu.protocol, 'parent') + '.h',
> +                _protocolHeaderName(inc.tu.protocol, 'child') + '.h',

Nope, we're not running on a case-insensitive file syste, it's just ipdl.

So, for some protocol P, for every protocol Q that it uses, we're going #include headers for QParent and QChild?  Why can the ParamTraits specializations not be placed in the Q.h files instead?

Comment 32

[Nathan Froyd [:froydnj]]

Comment on attachment 8953585 [details] [diff] [review]
Part 4: Record the actor which the message is being serialized/deserialized for on IPC::Message

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

I am not sure whether the whole idea of this bug is a good idea or not; I am quite taken with the idea of being able to define more things in terms of ParamTraits, though.

Some comments below.

::: ipc/chromium/src/chrome/common/ipc_message.h
@@ +325,5 @@
> +      if (!aMsg->for_actor_) {
> +        msg_ = aMsg;
> +        msg_->for_actor_ = aActor;
> +      }
> +      MOZ_ASSERT(aActor && msg_->for_actor_ == aActor);

Couple of questions:

1. Should this take a `const Message&`, so we don't have to worry about nullptr messages (as we do in the destructor, for which see below)?
2. Do we really have code that would create multiple AutoForActor structures, nested on the stack, for the same Message?  That seems really bad.
3. If the answer to #2 is "no", maybe we should just MOZ_DIAGNOSTIC_ASSERT(!aMsg->for_actor_) here?

@@ +329,5 @@
> +      MOZ_ASSERT(aActor && msg_->for_actor_ == aActor);
> +    }
> +
> +    ~AutoForActor() {
> +      if (msg_) {

Why are we checking for non-nullptr here when we didn't check for it in the constructor and we have no way of changing `msg_` through any public interface?  Are you (ugh!) adding such a public interface in a later patch?

@@ +512,5 @@
>      return file_descriptor_set_.get();
>    }
>  #endif
>  
> +  // NOTE: This field isn't copied in the copy of assignment constructors.

Nit: "the copy of assignment constructors"?  I am really unsure of what you mean here. :)

::: ipc/ipdl/ipdl/lower.py
@@ +4960,5 @@
>              senterrorcall = errfnSentinel('Error deserializing ' + paramtype)
>          ifbad.addifstmts(errorcall)
>  
> +        block = StmtBlock()
> +        block.addstmts([ _autoForActor(msgexpr, None), ifbad ])

Wait, what?  We're going to AutoForActor(msg, nullptr)?\

OK, wait, that's not what we're doing.  Can we please not make `_autoForActor` do magic, and just write the straightforward code here?

Comment 33

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

(In reply to Nathan Froyd [:froydnj] from comment #29)
> IIUC, it is never correct to provide both T and cls, but all other
> combinations may be provided.  Would you please assert that this is the case?

Yes, I believe this is true. I'll add an assert.

(In reply to Nathan Froyd [:froydnj] from comment #30)
> Am I correct in assuming that `fq` is ipdl for "fully qualified"?

Yes.

(In reply to Nathan Froyd [:froydnj] from comment #31)
> This field is not actually used?  If it gets used in a later patch, please
> move it to that patch.

Oops, that was from an earlier patch and accidentally got committed, I'll remove it.

> > +    def visitCxxInclude(self, inc):
> > +        self.cppIncludeHeaders.append(inc.file)
> 
> Are you sure about this?  I have vague memories of trying to put code like
> this here and getting into a world of pain.

It seems to work for me. I need these includes in order to get the ParamTraits implementations inside of the .cpp file.

These includes are only being added to the .cpp file.

> @@ +1546,5 @@
> >                      'include', '"'+ _ipdlhHeaderName(inc.tu) +'.h"'))
> > +        else:
> > +            self.cppIncludeHeaders += [
> > +                _protocolHeaderName(inc.tu.protocol, 'parent') + '.h',
> > +                _protocolHeaderName(inc.tu.protocol, 'child') + '.h',
> 
> Nope, we're not running on a case-insensitive file syste, it's just ipdl.

Yup, ipdl is "great"

> So, for some protocol P, for every protocol Q that it uses, we're going
> #include headers for QParent and QChild?  Why can the ParamTraits
> specializations not be placed in the Q.h files instead?

This was simpler to do, as the information is needed in different places. These includes are going to only be #include-d inside the .cpp file. This seemed like it'd be fine as we're doing unified builds for the ipdl generated files anyway.

(In reply to Nathan Froyd [:froydnj] from comment #32)
> I am not sure whether the whole idea of this bug is a good idea or not; I am
> quite taken with the idea of being able to define more things in terms of
> ParamTraits, though.

The other option which may be nicer would be to simply add an IProtocol parameter to ParamTraits::Write and ParamTraits::Read, but that would be a much larger change. It seemed reasonable to work around this by simply attaching the target actor into the message.

I could also use some template trickery to only pass this information as a parameter if it's actually used, allowing us to not attach the info to the message object.

> ::: ipc/chromium/src/chrome/common/ipc_message.h
> @@ +325,5 @@
> > +      if (!aMsg->for_actor_) {
> > +        msg_ = aMsg;
> > +        msg_->for_actor_ = aActor;
> > +      }
> > +      MOZ_ASSERT(aActor && msg_->for_actor_ == aActor);
> 
> Couple of questions:
> 
> 1. Should this take a `const Message&`, so we don't have to worry about
> nullptr messages (as we do in the destructor, for which see below)?

Sure.

> 2. Do we really have code that would create multiple AutoForActor
> structures, nested on the stack, for the same Message?  That seems really
> bad.

Yes, we currently do because every write call generated by IPDL creates an AutoForActor. I can do a small change to avoid this unnecessary code generation.

> 3. If the answer to #2 is "no", maybe we should just
> MOZ_DIAGNOSTIC_ASSERT(!aMsg->for_actor_) here?

Sure.

> @@ +329,5 @@
> > +      MOZ_ASSERT(aActor && msg_->for_actor_ == aActor);
> > +    }
> > +
> > +    ~AutoForActor() {
> > +      if (msg_) {
> 
> Why are we checking for non-nullptr here when we didn't check for it in the
> constructor and we have no way of changing `msg_` through any public
> interface?  Are you (ugh!) adding such a public interface in a later patch?

If aMsg->for_actor_ is non-null, then msg_ is never set, so msg_ can be null here.

> Nit: "the copy of assignment constructors"?  I am really unsure of what you
> mean here. :)

Uhh, I'll clean up the comment / remove this 

> Wait, what?  We're going to AutoForActor(msg, nullptr)?\
> 
> OK, wait, that's not what we're doing.  Can we please not make
> `_autoForActor` do magic, and just write the straightforward code here?

Yup.

=====

I think I'm going to give the extra parameter + some template trickery idea a shot, as thinking about it it won't be super difficult. Basically I'll allow ParamTraits implementations to optionally take an IProtocol& parameter, and pass it from IPDL code. Existing ParamTraits impls which don't take an IProtocol& will be supported by template trickery.

IPDL generated ParamTraits implementations will always require an IProtocol& parameter, as will other code which requires a protocol to serialize over, but existing 2-parameter impls will still continue to work as they do today, and we won't attach protocols to actors.

Comment 34

[Nathan Froyd [:froydnj]]

(In reply to Nika Layzell [:mystor] from comment #33)
> (In reply to Nathan Froyd [:froydnj] from comment #29)
> Yup, ipdl is "great"
> 
> > So, for some protocol P, for every protocol Q that it uses, we're going
> > #include headers for QParent and QChild?  Why can the ParamTraits
> > specializations not be placed in the Q.h files instead?
> 
> This was simpler to do, as the information is needed in different places.
> These includes are going to only be #include-d inside the .cpp file. This
> seemed like it'd be fine as we're doing unified builds for the ipdl
> generated files anyway.

OK, I suppose putting them in the .cpp is not so bad.

> (In reply to Nathan Froyd [:froydnj] from comment #32)
> > I am not sure whether the whole idea of this bug is a good idea or not; I am
> > quite taken with the idea of being able to define more things in terms of
> > ParamTraits, though.
> 
> The other option which may be nicer would be to simply add an IProtocol
> parameter to ParamTraits::Write and ParamTraits::Read, but that would be a
> much larger change. It seemed reasonable to work around this by simply
> attaching the target actor into the message.
> 
> I could also use some template trickery to only pass this information as a
> parameter if it's actually used, allowing us to not attach the info to the
> message object.

The template trickery option sounds nice, as that would make it clear when we need the actor and that something special is going on here.  What would the error messages look like if you forget the actor when it's actually needed, though?  And do we have to worry about people passing the wrong actor?

> > ::: ipc/chromium/src/chrome/common/ipc_message.h
> > @@ +325,5 @@
> > > +      if (!aMsg->for_actor_) {
> > > +        msg_ = aMsg;
> > > +        msg_->for_actor_ = aActor;
> > > +      }
> > > +      MOZ_ASSERT(aActor && msg_->for_actor_ == aActor);
> > 
> > 2. Do we really have code that would create multiple AutoForActor
> > structures, nested on the stack, for the same Message?  That seems really
> > bad.
> 
> Yes, we currently do because every write call generated by IPDL creates an
> AutoForActor. I can do a small change to avoid this unnecessary code
> generation.

Ah, that makes total sense, then; it's not like we're setting up an AutoForActor and threading that through the necessary calls.  OK.

> > @@ +329,5 @@
> > > +      MOZ_ASSERT(aActor && msg_->for_actor_ == aActor);
> > > +    }
> > > +
> > > +    ~AutoForActor() {
> > > +      if (msg_) {
> > 
> > Why are we checking for non-nullptr here when we didn't check for it in the
> > constructor and we have no way of changing `msg_` through any public
> > interface?  Are you (ugh!) adding such a public interface in a later patch?
> 
> If aMsg->for_actor_ is non-null, then msg_ is never set, so msg_ can be null
> here.

Oh.  OK.  But then AutoForActor wasn't initializing msg_ anywhere else, so I think we'd actually be touching undefined memory at that point?

> I think I'm going to give the extra parameter + some template trickery idea
> a shot, as thinking about it it won't be super difficult. Basically I'll
> allow ParamTraits implementations to optionally take an IProtocol&
> parameter, and pass it from IPDL code. Existing ParamTraits impls which
> don't take an IProtocol& will be supported by template trickery.
> 
> IPDL generated ParamTraits implementations will always require an IProtocol&
> parameter, as will other code which requires a protocol to serialize over,
> but existing 2-parameter impls will still continue to work as they do today,
> and we won't attach protocols to actors.

This would be pretty snazzy.  I have vague concerns about compile time here, as well as the concerns noted above, but we can try it and see how it turns out?

Comment 35

[Nathan Froyd [:froydnj]]

Comment on attachment 8953586 [details] [diff] [review]
Part 5: Add a ParamTraits implementation for FileDescriptor

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

To be 100% clear, this code is the new way to do the code that will be removed in part 13, correct?  (implementFDPickling, AFAICT)  And we can't actually remove that lower.py code until a bunch of other prerequisite work is done--part 12, again AFAICT--correct?  So the upshot is that we have this weird halfway point commit that doesn't get used for a while, correct?

It would be *super* helpful to note this sort of stuff in the commit message. ;)

::: ipc/glue/FileDescriptor.cpp
@@ +252,5 @@
> +  }
> +
> +  *aResult = FileDescriptor(FileDescriptor::IPDLPrivate(), pfd);
> +  if (!aResult->IsValid()) {
> +    printf_stderr("IPDL protocol Error: [%s] Received an invalid file descriptor\n",

Why are we not using ProtocolErrorBreakpoint here?  Include issues?

Actually, this is sort of a bizarre decision.  If we *know* we received an FD that doesn't work, is it reasonable to just crash, assuming that either a) the process on the other side has become corrupted; or b) somebody is trying to hack us over IPC?

Comment 36

[Nathan Froyd [:froydnj]]

Comment on attachment 8953587 [details] [diff] [review]
Part 6:  Move all serialization logic into the ParmaTraits impl for Shmem

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

Commit message nit: "ParamTraits", not "ParmaTraits".

Have to say, this is sooo much nicer than trying to decode lower.py code, or having the same code duplicated in a bunch of different actors.

::: ipc/glue/Shmem.cpp
@@ +534,5 @@
> +      rawmem, id);
> +    return true;
> +  }
> +  *aResult = Shmem();
> +  return true;

Love the error handling here.

::: ipc/glue/Shmem.h
@@ -274,5 @@
>    typedef mozilla::ipc::Shmem paramType;
>  
> -  // NB: Read()/Write() look creepy in that Shmems have a pointer
> -  // member, but IPDL internally uses mId to properly initialize a
> -  // "real" Shmem

Good riddance to creepiness.

Comment 37

[Nathan Froyd [:froydnj]]

Comment on attachment 8953590 [details] [diff] [review]
Part 9: Include ParamTraits impls which are used in ipdlh files

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

To be clear, this stems from types in these .ipdlh files, once ParamTraits generation is done, needing to serialize types in the #include'd C++ headers, and therefore needing the definitions of ParamTraits from said C++ headers?

Comment 38

[Nathan Froyd [:froydnj]]

Comment on attachment 8953591 [details] [diff] [review]
Part 10: Add support for serializing objects which take paramType by non-const reference in arrays

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

I do not understand the rationale for this patch, so canceling review.  Maybe an example of what's going wrong would help?

Comment 39

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

Attachment: Part 1: Remove unnecessary ParamTraits<> specialization declaration from HangDetails.h

MozReview-Commit-ID: COnMiadbCjn

Comment 40

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

Attachment: Part 2: Correctly namespace nsHttpResponseHead in NeckoChannelParams.ipdlh

MozReview-Commit-ID: DVkTwAmqmkc

Comment 41

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

Attachment: Part 3: Support specializations in _splitMethodDefn

MozReview-Commit-ID: JFYgyUr4I4G

Comment 42

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

Attachment: Part 4: Add IPDLParamTraits which supports passing in an actor when {un,}pickling

MozReview-Commit-ID: JTsQJ292A09

Comment 43

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

Attachment: Part 5: Add an IPDLParamTraits implementation for FileDescriptor

MozReview-Commit-ID: R1CiT5IvAi

Comment 44

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

Attachment: Part 6: Move all serialization logic into an IPDLParamTraits impl for Shmem

MozReview-Commit-ID: DsrfSVPgoo1

Comment 45

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

Attachment: Part 7: Support passing the fq flag to _cxxBareType in more places

MozReview-Commit-ID: K1lkVr6KdZo

Comment 46

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

Attachment: Part 8: Include c++ includes in ipdlh generated .cpp files

This is important as these includes are often used to define ParamTraits
implementations which are needed to serialize struct members and
parameters. The place where the code which uses these implementations
will be moving from the protocol where they are used to the file where
the structs are defined, which means we need to includes these files
there.

MozReview-Commit-ID: H1wqgyv5mel

Comment 47

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

Attachment: Part 9: Include ParamTraits impls which are used in ipdlh files

MozReview-Commit-ID: 9AfzlhyTgsY

Comment 48

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

Attachment: Part 10: Correct serialization and deserialization of arrays of Shmem objects

MozReview-Commit-ID: KEFduw2Pn8r

Comment 49

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

Attachment: Part 11: Implement IPDLParamTraits for IPDL generated types

MozReview-Commit-ID: 2ljPFJKM9Fg

Comment 50

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

Attachment: Part 12: Switch to calling IPDLParamTraits directly when pickling and unpickling messages

MozReview-Commit-ID: 1t4oIp5JWcU

Comment 51

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

Attachment: Part 13: Remove the now-unused per-actor pickling code

MozReview-Commit-ID: 5ASQu4ly0od

Comment 52

[Nathan Froyd [:froydnj]]

Comment on attachment 8954536 [details] [diff] [review]
Part 4: Add IPDLParamTraits which supports passing in an actor when {un,}pickling

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

This looks fine.  r=me with comment change; style points for fixing the `else` block.

::: ipc/glue/IPDLParamTraits.h
@@ +27,5 @@
> +  // IPDLParamTraits.
> +  static inline void Write(IPC::Message* aMsg, IProtocol*, const P& aParam) {
> +    IPC::ParamTraits<P>::Write(aMsg, aParam);
> +  }
> +  static inline void Write(IPC::Message* aMsg, IProtocol*, P& aParam) { // HACK

Mmmm, I think we need something a little more explanatory than HACK.  Or is that related to the nsTArray special-casing below?

@@ +104,5 @@
> +      }
> +
> +      T* elements = aResult->AppendElements(length);
> +      return aMsg->ReadBytesInto(aIter, elements, pickledLength.value());
> +    } else {

While we're here, we might as well remove this `else`, since the previous block `return`'d.

Comment 53

[Nathan Froyd [:froydnj]]

Comment on attachment 8954540 [details] [diff] [review]
Part 8: Include c++ includes in ipdlh generated .cpp files

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

Thank you for writing the commit message.  r=me with the fix below.

::: ipc/ipdl/ipdl/lower.py
@@ +1440,5 @@
>          self.cppfile = None      # what will become Protocol.cpp
>          self.cppIncludeHeaders = []
>          self.structUnionDefns = []
>          self.funcDefns = []
> +        self.protocolHeaders = []

This field is still here, despite comments to the contrary.

Comment 54

[Nathan Froyd [:froydnj]]

Comment on attachment 8954542 [details] [diff] [review]
Part 10: Correct serialization and deserialization of arrays of Shmem objects

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

I still do not understand the rationale for this patch, and no explanation has been forthcoming. :)

Comment 55

[Nathan Froyd [:froydnj]]

Comment on attachment 8954543 [details] [diff] [review]
Part 11: Implement IPDLParamTraits for IPDL generated types

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

I am kind of assuming this is correct and corresponds to the serde stuff we already do for these types.

Comment 56

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #55)
> I am kind of assuming this is correct and corresponds to the serde stuff we
> already do for these types.

Blue-sky: it would be most excellent if we could figure out some way to move a lot of this code out into C++, probably through some template magic, and have lower.py generate just enough code for the templates to work their magic.  I don't know whether the compile-time hit would be too much, though...

Comment 57

[Nathan Froyd [:froydnj]]

Comment on attachment 8954544 [details] [diff] [review]
Part 12: Switch to calling IPDLParamTraits directly when pickling and unpickling messages

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

\o/

Comment 58

[Nathan Froyd [:froydnj]]

Comment on attachment 8954545 [details] [diff] [review]
Part 13: Remove the now-unused per-actor pickling code

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

Any patch deleting that much code from lower.py gets an enthusiastic r+!

Comment 59

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

(In reply to Nathan Froyd [:froydnj] from comment #53)
> > +        self.protocolHeaders = []
> 
> This field is still here, despite comments to the contrary.

Hmm. It seems like this patch got merged somewhere random during the rebase >.<

(In reply to Nathan Froyd [:froydnj] from comment #54)
> I still do not understand the rationale for this patch, and no explanation
> has been forthcoming. :)

I'd link SearchFox here, but it's in generated code, so the links will break as soon as this patch lands, so instead I'll post the relevant code snippets inline.

Currently the WebRender code serializes an array of Shmem objects to pass over ipdl. As before this patch, arrays were always passed by const reference in ipdl, this meant that the ipdl code was handling a const nsTArray<Shmem>&:

auto PWebRenderBridgeParent::Write(
        const nsTArray<Shmem>& v__,
        Message* msg__) -> void
{
    uint32_t length = (v__).Length();
    Write(length, msg__);
    // Sentinel = ('length', 'Shmem[]')
    (msg__)->WriteSentinel(3935819020);

    for (auto& elem : v__) {
        Write(elem, msg__);
        // Sentinel = 'Shmem[]'
        (msg__)->WriteSentinel(1748494619);
    }
}

Shmem is usually serialized from a non-const reference, so the Shmem overload looked like the following:

auto PWebRenderBridgeParent::Write(
        Shmem& v__,
        Message* msg__) -> void
{
    IPC::WriteParam(msg__, v__);
    (v__).RevokeRights(Shmem::IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead());
    (v__).forget(Shmem::IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead());
}

The `Write()` call in the nsTArray<Shmem> serializer however did not match that overload, as it only had a const Shmem& reference. Thus, the called function was actually the generic write implementation: 

template<typename T>
void
Write(
        const T& v__,
        Message* msg__)
{
    IPC::WriteParam(msg__, v__);
}

Which called the "creepy" implementation of ParamTraits in the Write case:
https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/ipc/glue/Shmem.h#271-298

Fortunately, this ended up working "correctlyish" in the current code, as the deserialization code was called correctly, and only the serialization code was wrong. The extra work the serialization code did exists only for safety, as far as I can tell, by clearing out the state inside of the Shmem object and changing some page permissions. "correct" code wouldn't act differently if the code wasn't run.

This change causes the code to run the correct overload. Some changes were also needed at the sender callsite, as the caller was incorrectly Move(..)-ing in the arguments which should instead be passed by non-const lvalue reference.

(In reply to Nathan Froyd [:froydnj] from comment #56)
> Blue-sky: it would be most excellent if we could figure out some way to move
> a lot of this code out into C++, probably through some template magic, and
> have lower.py generate just enough code for the templates to work their
> magic.  I don't know whether the compile-time hit would be too much,
> though...

The code could definitely be simplified, by moving things like the error handling (which is pretty much always "crash"), and sentinel checking into a helper function. The generated code would then just call something like WriteIPDLParamWithSentinel(aMsg, aActor, aField, SENTINEL_VALUE); or ReadIPDLParamWithSentinel(aMsg, aIter, aActor, aField, SENTINEL_VALUE).

I think this should be done in a follow-up though. This patch is big enough already.

Comment 60

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

Comment on attachment 8954542 [details] [diff] [review]
Part 10: Correct serialization and deserialization of arrays of Shmem objects

See previous comment.

Comment 61

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

Attachment: Part 4: Add IPDLParamTraits which supports passing in an actor when {un,}pickling

MozReview-Commit-ID: JTsQJ292A09

Comment 62

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

Attachment: Part 8: Include c++ includes in ipdlh generated .cpp files

This is important as these includes are often used to define ParamTraits
implementations which are needed to serialize struct members and
parameters. The place where the code which uses these implementations
will be moving from the protocol where they are used to the file where
the structs are defined, which means we need to includes these files
there.

MozReview-Commit-ID: H1wqgyv5mel

Comment 63

[Nathan Froyd [:froydnj]]

(In reply to Nika Layzell [:mystor] from comment #59)
> (In reply to Nathan Froyd [:froydnj] from comment #56)
> > Blue-sky: it would be most excellent if we could figure out some way to move
> > a lot of this code out into C++, probably through some template magic, and
> > have lower.py generate just enough code for the templates to work their
> > magic.  I don't know whether the compile-time hit would be too much,
> > though...
> 
> The code could definitely be simplified, by moving things like the error
> handling (which is pretty much always "crash"), and sentinel checking into a
> helper function. The generated code would then just call something like
> WriteIPDLParamWithSentinel(aMsg, aActor, aField, SENTINEL_VALUE); or
> ReadIPDLParamWithSentinel(aMsg, aIter, aActor, aField, SENTINEL_VALUE).
> 
> I think this should be done in a follow-up though. This patch is big enough
> already.

Absolutely.  I did not intend to make this bug any larger.

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1517d3c49681
Part 1: Correctly namespace nsHttpResponseHead in NeckoChannelParams.ipdlh, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/36db285e47c5
Part 2: Support specializations in _splitMethodDefn, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbf6ac93e48c
Part 3: Add IPDLParamTraits which supports passing in an actor when {un,}pickling, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/5467d67a2577
Part 4: Add an IPDLParamTraits implementation for FileDescriptor, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/016d3604bf18
Part 5: Move all serialization logic into an IPDLParamTraits impl for Shmem, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dcd0320b88c
Part 6: Support passing the fq flag to _cxxBareType in more places, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/53b7368786a6
Part 7: Include c++ includes in ipdlh generated .cpp files, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/13bcebbf334e
Part 8: Include ParamTraits impls which are used in ipdlh files, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/5005aa065ba5
Part 9: Correct serialization and deserialization of arrays of Shmem objects, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/73a096b8506c
Part 10: Implement IPDLParamTraits for IPDL generated types, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/df74e15398ad
Part 11: Switch to calling IPDLParamTraits directly when pickling and unpickling messages, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c593ecf725
Part 12: Remove the now-unused per-actor pickling code, r=froydnj

Comment 65

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1517d3c49681
https://hg.mozilla.org/mozilla-central/rev/36db285e47c5
https://hg.mozilla.org/mozilla-central/rev/bbf6ac93e48c
https://hg.mozilla.org/mozilla-central/rev/5467d67a2577
https://hg.mozilla.org/mozilla-central/rev/016d3604bf18
https://hg.mozilla.org/mozilla-central/rev/3dcd0320b88c
https://hg.mozilla.org/mozilla-central/rev/53b7368786a6
https://hg.mozilla.org/mozilla-central/rev/13bcebbf334e
https://hg.mozilla.org/mozilla-central/rev/5005aa065ba5
https://hg.mozilla.org/mozilla-central/rev/73a096b8506c
https://hg.mozilla.org/mozilla-central/rev/df74e15398ad
https://hg.mozilla.org/mozilla-central/rev/14c593ecf725

Comment 66

[Julien Cristau [:jcristau]]

Is there a particular reason this needed to land during soft freeze for 60?