1090570 - Protocols created via bridging shouldn't have to delete their own channel

Status: RESOLVED DUPLICATE of bug 1283744

(Core :: IPC, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Here is an example:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentBridgeChild.cpp?rev=08ad036e00fe#31

This just seems unnecessary. It would be nice if IPDL automatically deleted the channel. I haven't looked into how hard this would be though.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Make MessageLink own the Transport. WIP

Bill suggested what to do here.  Basically, MessageLink just does the deleting of Transport, and the other code that was doing it before is deleted.  With this, a short run of some random Mochitests has no leaks detected by LSan (and also has no UAFs).

The scariest part is the de-UniquePtr'ing of the chromium IPC stuff.  Bill suggested that for those we could instead add a flag to MessageLink, and leave the ownership as is for them.

We'll see if that holds up for a full try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28013e3191a4

Comment 2

[Andrew McCreight [:mccr8]]

For this patch, there also needs to be some trivial additional work to remove all of the mTransport fields that are no longer used.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8549281 [details] [diff] [review]
Make MessageLink own the Transport. WIP

Do you have any thoughts on this patch?  The main sketchy part here is that it makes ChildProcessHost and ChildThread have weak pointers to their channel_, though it does pass an ASan run.  I don't know enough about these classes to know if we're guaranteed to have the lifetime nest appropriately with the MessageLink.  Bill said he'd look through this code and try to see if he can make some sense of it.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8549281 [details] [diff] [review]
Make MessageLink own the Transport. WIP

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

Looks ok, but:

::: ipc/glue/MessageLink.cpp
@@ +78,5 @@
>  }
>  
>  ProcessLink::~ProcessLink()
>  {
> +    delete mTransport;

Is there any guarantee that this happens the IO thread?

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

MessageChannel owns the MessageLink, so I don't think that's a problem.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Make MessageLink own the Transport

A bunch more places do this now, so I had to fix those. I'm going to read over this again so I remember what is going on and try to get this fixed.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Remove unused mTransport fields.

Comment 8

[Andrew McCreight [:mccr8]]

I've been avoiding fixing this, because I don't entirely understand the relationship between these classes. Looking at a local run of this, I think I can just fix the leak of specific classes and leave this reorganization for later. I filed bug 1212984 and bug 1212986 for that.  I filed bug 1212987 for making it so that we detect Channel leaks with LSan and the XPCOM leak detector.