Closed Bug 1212027 Opened 4 years ago Closed 4 years ago

use a more intelligent data structure for holding managed protocols

Categories

(Core :: IPC, defect)

43 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(11 files)

6.36 KB, patch
jld
: review+
Details | Diff | Splinter Review
1.02 KB, patch
jld
: review+
Details | Diff | Splinter Review
2.07 KB, patch
jld
: review+
Details | Diff | Splinter Review
1.92 KB, patch
jld
: review+
Details | Diff | Splinter Review
16.49 KB, patch
jld
: review+
Details | Diff | Splinter Review
1.04 KB, patch
erahm
: review+
Details | Diff | Splinter Review
11.62 KB, patch
jld
: review+
Details | Diff | Splinter Review
5.25 KB, patch
jld
: review+
Details | Diff | Splinter Review
3.90 KB, patch
nical
: review+
Details | Diff | Splinter Review
2.84 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
14.33 KB, patch
jld
: review+
billm
: review+
Details | Diff | Splinter Review
As shown in bug 1208840 comment 13, using arrays to hold managed protocols can bite us badly when we go to remove them (or even insert them).  This bug is for fixing that part of IPDL, since it is a significant sub-project in its own right.

This would be straightforward, except that we expose arrays of managed protocols outside of the protocols themselves, which means that changing the internal representation necessitates changing some 20 callsites outside of the code generator as well.
The functions:

- _callCxxArrayInsertSorted
- _callCxxArrayRemoveSorted
- _callCxxArrayClear
- _cxxArrayHasElementSorted

are only ever used to touch the managed sub-protocol arrays of a
protocol.  It would be better if they reflected the *intent* of what
they were doing, rather than what C++ function they were calling, since
we're about to change that.
Attachment #8670980 - Flags: review?(jld)
This type will come in handy when we have to add iteration over
hashtables, since it would be a pain to write out the iterator type.
Attachment #8670981 - Flags: review?(jld)
In $PROTOCOL::CloneManages, we reach directly into the other protocol's
sub-protocol arrays to copy them.  It would be better, from a
fewer-places-to-modify-when-things-change point of view if we used the
$PROTOCOL::Managed$SUBPROTOCOL array getter that already exists for this
purpose.  A good compiler should be able to remove the function call
overhead...but cloning managees is probably expensive anyway, so a
function call here doesn't matter much.

It's not immediately obvious to me why we clone and then iterate over
the clone, rather than iterating directly, but perhaps there are subtle
IPDL dragons lurking hereabouts.
Attachment #8670982 - Flags: review?(jld)
Similar to the last patch, we copy any sub-protocols a protocol owns
before destroying the sub-protocols.  We do this to ensure a stable
iteration over the sub-protocols, as destroying a sub-protocol may
trigger other sub-protocol deletions.  Let's permit an existing
interface method to do the copying for us, so if the details of how we
store sub-protocols change, this call site is insulated from the
details.
Attachment #8670984 - Flags: review?(jld)
A lot of existing code has variations on:

  if (ManagedPFooChild().Length()) {
    ...(ManagedPFooChild()[0])...
  }
  // Do something with nullptr, or some other action.

It's pretty reasonable to repeat this code when the managed protocols
are stored in an array; the code gets much less nice when managed
protocols are stored in a hashtable.  Let's write a small utility
function to handle those details for us.  Then when we change the
underlying storage, we only need to update this function, rather than a
bunch of callsites.

ProtocolUtils.h is included by all the generated IPDL headers, so
LoneManagedOrNull should be available everywhere the above pattern would
be encountered.
Attachment #8670985 - Flags: review?(jld)
It's a little more convenient than checking Count(), and also gives
nsTHashtable the same interface as nsTArray (for this operation, at
least), which seems worthwhile.
Attachment #8670986 - Flags: review?(erahm)
This part DOES NOT COMPILE on its own, but is separated out to ease review.

I can post a diff of a generated file if you would like.
Attachment #8670988 - Flags: review?(jld)
Thankfully, there's nothing iterating over the contents of the
sub-protocols here, so we can get by with a simple search-and-replace of
Length() with Count().
Attachment #8670989 - Flags: review?(jld)
These are straightforward pieces of code to write, but verifying their
correctness is a little tricky:

- The code in CompositorChild.cpp tried very hard to avoid iterating
  over parts of the array as it is being modified by the Destroy() call.
  The replacement code simply grabs a copy of all the transactions that
  it needs, so iterating over the local copy while Destroy()'ing things
  should be safe.

- To my untutored eyes, the code in LayerTransactionParent.cpp should
  have the same issues, as we're Destroy()'ing things as we iterate over
  them, but maybe that's not a problem here?

The code in LayerTransactionChild.cpp looks straightforward enough, but
maybe there's some IPC interactions that I'm not aware of here.
Attachment #8670990 - Flags: review?(nical.bugzilla)
It's not clear to me whether the reverse iteration over the arrays is
intended to cope with the following scenario:

- Call SendShutdown() on a protocol;

- Protocol removes itself from the appropriate Managed* list;

- Iteration continues OK, because we're not removing a protocol out from
  underneath ourselves, as we would be if we iterated forwards.

...or maybe whether the iteration is just "more efficient" if we do it
this way.

If it's the latter, then this version should be fine.  If it's the
former, then what we should really be doing is grabbing a local copy of
the children, and iterating over our local copy, which is guaranteed to
be stable.
Attachment #8670991 - Flags: review?(cpearce)
Asking Jed (IPC stuff generally) and Bill (IPC stuff, but also DOM e10s
wizardry) both to look at this.  If one of you want to claim it for yourself,
feel free.
Attachment #8670994 - Flags: review?(wmccloskey)
Attachment #8670994 - Flags: review?(jld)
Comment on attachment 8670986 [details] [diff] [review]
part 6 - add an IsEmpty method to nsTHashtable

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

r=me Perhaps update the tests if they exist?
Attachment #8670986 - Flags: review?(erahm) → review+
Attachment #8670980 - Flags: review?(jld) → review+
Attachment #8670981 - Flags: review?(jld) → review+
Comment on attachment 8670982 [details] [diff] [review]
part 3 - use class interfaces when cloning managees

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

r=me with the disclaimer that this is the first time I've encountered protocol cloning, so I can't answer the question about why it might need an array copy, but this patch looks safe enough.
Attachment #8670982 - Flags: review?(jld) → review+
Attachment #8670984 - Flags: review?(jld) → review+
Comment on attachment 8670985 [details] [diff] [review]
part 5 - add LoneManagedOrNull for simplifying a lot of upcoming code

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

Looks reasonable.  I like the length assertion; I wonder if we'll discover anything “interesting” that way, especially about the PBrowser instances.
Attachment #8670985 - Flags: review?(jld) → review+
Attachment #8670989 - Flags: review?(jld) → review+
Attachment #8670988 - Flags: review?(jld) → review+
Comment on attachment 8670994 [details] [diff] [review]
part 7e - modify Gecko generally to cope with hashtable storage for managed protocols

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

This all looks pretty straightforward to me.
Attachment #8670994 - Flags: review?(jld) → review+
(In reply to Nathan Froyd [:froydnj] from comment #10)
> It's not clear to me whether the reverse iteration over the arrays is
> intended to cope with the following scenario:
> 
> - Call SendShutdown() on a protocol;
> 
> - Protocol removes itself from the appropriate Managed* list;
> 
> - Iteration continues OK, because we're not removing a protocol out from
>   underneath ourselves, as we would be if we iterated forwards.

Something like that (I don't remember exactly off hands). I think that the thing this was working around doesn't apply anymore because the __delete__ message has become asynchronous or something along those lines (that was a while ago).
Attachment #8670990 - Flags: review?(nical.bugzilla) → review+
(In reply to Nicolas Silva [:nical] from comment #16)
> (In reply to Nathan Froyd [:froydnj] from comment #10)
> > It's not clear to me whether the reverse iteration over the arrays is
> > intended to cope with the following scenario:
> > 
> > - Call SendShutdown() on a protocol;
> > 
> > - Protocol removes itself from the appropriate Managed* list;

With the current state of the IPC code this shouldn't be a problem — the parent can get the Shutdown and respond with __delete__, but the child won't handle that __delete__ (by removing the actor) until it gets to an event loop.

> Something like that (I don't remember exactly off hands). I think that the
> thing this was working around doesn't apply anymore because the __delete__
> message has become asynchronous or something along those lines (that was a
> while ago).

For Compositor/LayerTransaction it looks like the history is bug 966284: it used to Send__delete__ the last element until the array was empty, and later changed to a two-way handshake (requesting that the other side __delete__ in order to avoid races).

The GMP case… it looks like the reverse iteration was carried over from something that wasn't an IPC managee array.
Comment on attachment 8670994 [details] [diff] [review]
part 7e - modify Gecko generally to cope with hashtable storage for managed protocols

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

I'm a bit concerned about the use of auto on the container type, especially since you have to look in generated code to find the real type. Can we create a templated type alias in ProtocolUtils.h that people can use instead?
Attachment #8670994 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #18)
> I'm a bit concerned about the use of auto on the container type, especially
> since you have to look in generated code to find the real type. Can we
> create a templated type alias in ProtocolUtils.h that people can use instead?

You mean something like:

template<typename Protocol>
using ManagedContainer = nsTHashtable<nsPtrHashKey<Protocol>>;

?
Flags: needinfo?(wmccloskey)
Yes, precisely.
Flags: needinfo?(wmccloskey)
Comment on attachment 8670991 [details] [diff] [review]
part 7d - modify gmp code to cope with hashtable storage for managed protocols

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

I think this is OK, because I don't see how we could process any other messages that would cause the GMPContentChild's managed lists to change while we're sending the shutdown messages here.
Attachment #8670991 - Flags: review?(cpearce) → review+
Depends on: 1217250
Depends on: 1223056
Duplicate of this bug: 551121
You need to log in before you can comment on or make changes to this bug.