Closed Bug 1090570 Opened 7 years ago Closed 5 years ago

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

Categories

(Core :: IPC, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1283744

People

(Reporter: billm, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak)

Attachments

(2 files, 1 obsolete file)

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.
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
Assignee: nobody → continuation
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 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.
Attachment #8549281 - Flags: feedback?(bent.mozilla)
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?
Attachment #8549281 - Flags: feedback?(bent.mozilla) → feedback+
MessageChannel owns the MessageLink, so I don't think that's a problem.
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.
Attachment #8549281 - Attachment is obsolete: true
No longer blocks: 1089816
Blocks: LSan
Keywords: mlk
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.
Assignee: continuation → nobody
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1283744
You need to log in before you can comment on or make changes to this bug.