Closed Bug 1440511 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: IPC, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 28 obsolete files)

1.25 KB, patch
Details | Diff | Splinter Review
1.19 KB, patch
Details | Diff | Splinter Review
6.86 KB, patch
Details | Diff | Splinter Review
3.11 KB, patch
Details | Diff | Splinter Review
3.56 KB, patch
Details | Diff | Splinter Review
3.08 KB, patch
Details | Diff | Splinter Review
3.49 KB, patch
Details | Diff | Splinter Review
2.94 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
17.59 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
14.00 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
29.66 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
6.35 KB, patch
Details | Diff | Splinter Review
3.55 KB, patch
Details | Diff | Splinter Review
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.
MozReview-Commit-ID: COnMiadbCjn
Attachment #8953283 - Flags: review?(nfroyd)
MozReview-Commit-ID: DVkTwAmqmkc
Attachment #8953284 - Flags: review?(nfroyd)
MozReview-Commit-ID: JFYgyUr4I4G
Attachment #8953285 - Flags: review?(nfroyd)
MozReview-Commit-ID: 9WtF3Sm9xD9
Attachment #8953286 - Flags: review?(nfroyd)
MozReview-Commit-ID: R1CiT5IvAi
Attachment #8953287 - Flags: review?(nfroyd)
MozReview-Commit-ID: DsrfSVPgoo1
Attachment #8953288 - Flags: review?(nfroyd)
MozReview-Commit-ID: K1lkVr6KdZo
Attachment #8953289 - Flags: review?(nfroyd)
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
Attachment #8953290 - Flags: review?(nfroyd)
MozReview-Commit-ID: 9AfzlhyTgsY
Attachment #8953291 - Flags: review?(nfroyd)
MozReview-Commit-ID: 2ljPFJKM9Fg
Attachment #8953293 - Flags: review?(nfroyd)
MozReview-Commit-ID: 1t4oIp5JWcU
Attachment #8953294 - Flags: review?(nfroyd)
MozReview-Commit-ID: 5ASQu4ly0od
Attachment #8953295 - Flags: review?(nfroyd)
MozReview-Commit-ID: COnMiadbCjn
Attachment #8953582 - Flags: review?(nfroyd)
Attachment #8953283 - Attachment is obsolete: true
Attachment #8953284 - Attachment is obsolete: true
Attachment #8953285 - Attachment is obsolete: true
Attachment #8953283 - Flags: review?(nfroyd)
Attachment #8953284 - Flags: review?(nfroyd)
Attachment #8953285 - Flags: review?(nfroyd)
Attachment #8953286 - Attachment is obsolete: true
Attachment #8953287 - Attachment is obsolete: true
Attachment #8953288 - Attachment is obsolete: true
Attachment #8953289 - Attachment is obsolete: true
Attachment #8953290 - Attachment is obsolete: true
Attachment #8953291 - Attachment is obsolete: true
Attachment #8953292 - Attachment is obsolete: true
Attachment #8953293 - Attachment is obsolete: true
Attachment #8953294 - Attachment is obsolete: true
Attachment #8953295 - Attachment is obsolete: true
Attachment #8953286 - Flags: review?(nfroyd)
Attachment #8953287 - Flags: review?(nfroyd)
Attachment #8953288 - Flags: review?(nfroyd)
Attachment #8953289 - Flags: review?(nfroyd)
Attachment #8953290 - Flags: review?(nfroyd)
Attachment #8953291 - Flags: review?(nfroyd)
Attachment #8953292 - Flags: review?(nfroyd)
Attachment #8953293 - Flags: review?(nfroyd)
Attachment #8953294 - Flags: review?(nfroyd)
Attachment #8953295 - Flags: review?(nfroyd)
MozReview-Commit-ID: DVkTwAmqmkc
Attachment #8953583 - Flags: review?(nfroyd)
MozReview-Commit-ID: JFYgyUr4I4G
Attachment #8953584 - Flags: review?(nfroyd)
MozReview-Commit-ID: 9WtF3Sm9xD9
Attachment #8953585 - Flags: review?(nfroyd)
MozReview-Commit-ID: R1CiT5IvAi
Attachment #8953586 - Flags: review?(nfroyd)
MozReview-Commit-ID: DsrfSVPgoo1
Attachment #8953587 - Flags: review?(nfroyd)
MozReview-Commit-ID: K1lkVr6KdZo
Attachment #8953588 - Flags: review?(nfroyd)
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
Attachment #8953589 - Flags: review?(nfroyd)
MozReview-Commit-ID: 9AfzlhyTgsY
Attachment #8953590 - Flags: review?(nfroyd)
MozReview-Commit-ID: 2ljPFJKM9Fg
Attachment #8953592 - Flags: review?(nfroyd)
MozReview-Commit-ID: 1t4oIp5JWcU
Attachment #8953593 - Flags: review?(nfroyd)
MozReview-Commit-ID: 5ASQu4ly0od
Attachment #8953594 - Flags: review?(nfroyd)
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
Blocks: 1440771
Attachment #8953582 - Flags: review?(nfroyd) → review+
Attachment #8953583 - Flags: review?(nfroyd) → review+
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...
Attachment #8953584 - Flags: review?(nfroyd) → review+
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"?
Attachment #8953588 - Flags: review?(nfroyd) → review+
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?
Attachment #8953589 - Flags: review?(nfroyd)
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?
Attachment #8953585 - Flags: review?(nfroyd)
(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.
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
(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 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?
Attachment #8953586 - Flags: review?(nfroyd) → review+
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.
Attachment #8953587 - Flags: review?(nfroyd) → review+
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?
Attachment #8953590 - Flags: review?(nfroyd) → review+
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?
Attachment #8953591 - Flags: review?(nfroyd)
Attachment #8953582 - Attachment is obsolete: true
Attachment #8953583 - Attachment is obsolete: true
Attachment #8953584 - Attachment is obsolete: true
Attachment #8953585 - Attachment is obsolete: true
Attachment #8953586 - Attachment is obsolete: true
Attachment #8953587 - Attachment is obsolete: true
Attachment #8953588 - Attachment is obsolete: true
Attachment #8953589 - Attachment is obsolete: true
Attachment #8953590 - Attachment is obsolete: true
Attachment #8953591 - Attachment is obsolete: true
Attachment #8953592 - Attachment is obsolete: true
Attachment #8953593 - Attachment is obsolete: true
Attachment #8953594 - Attachment is obsolete: true
Attachment #8953592 - Flags: review?(nfroyd)
Attachment #8953593 - Flags: review?(nfroyd)
Attachment #8953594 - Flags: review?(nfroyd)
MozReview-Commit-ID: JFYgyUr4I4G
MozReview-Commit-ID: JTsQJ292A09
Attachment #8954536 - Flags: review?(nfroyd)
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
Attachment #8954540 - Flags: review?(nfroyd)
MozReview-Commit-ID: KEFduw2Pn8r
Attachment #8954542 - Flags: review?(nfroyd)
MozReview-Commit-ID: 2ljPFJKM9Fg
Attachment #8954543 - Flags: review?(nfroyd)
MozReview-Commit-ID: 5ASQu4ly0od
Attachment #8954545 - Flags: review?(nfroyd)
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.
Attachment #8954536 - Flags: review?(nfroyd) → review+
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.
Attachment #8954540 - Flags: review?(nfroyd) → review+
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. :)
Attachment #8954542 - Flags: review?(nfroyd)
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.
Attachment #8954543 - Flags: review?(nfroyd) → review+
(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 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/
Attachment #8954544 - Flags: review?(nfroyd) → review+
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+!
Attachment #8954545 - Flags: review?(nfroyd) → review+
(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 on attachment 8954542 [details] [diff] [review]
Part 10: Correct serialization and deserialization of arrays of Shmem objects

See previous comment.
Attachment #8954542 - Flags: review?(nfroyd)
Attachment #8954536 - Attachment is obsolete: true
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
Attachment #8954540 - Attachment is obsolete: true
(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.
Attachment #8954542 - Flags: review?(nfroyd) → review+
Pushed by nika@thelayzells.com:
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
Depends on: 1443748
Is there a particular reason this needed to land during soft freeze for 60?
You need to log in before you can comment on or make changes to this bug.