Closed
Bug 1212986
Opened 9 years ago
Closed 9 years ago
Background ChildImpl does not delete its transport
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Keywords: memory-leak, Whiteboard: [MemShrink:P2])
Attachments
(1 file)
874 bytes,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 1•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=486ba168b5b1
Attachment #8672091 -
Flags: review?(wmccloskey)
Comment on attachment 8672091 [details] [diff] [review] BackgroundChildImpl should delete its Transport. Review of attachment 8672091 [details] [diff] [review]: ----------------------------------------------------------------- Asking Blake to take this since I don't understand whether this should go in BackgroundChildImpl or ChildImpl.
Attachment #8672091 -
Flags: review?(wmccloskey) → review?(mrbkap)
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #2) > Asking Blake to take this since I don't understand whether this should go in > BackgroundChildImpl or ChildImpl. Yeah, the deletion in the parent happens in ParentImpl, so maybe that would make more sense, if only for symmetry. But I have no idea about why these classes are split as they are.
Updated•9 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment 4•9 years ago
|
||
Comment on attachment 8672091 [details] [diff] [review] BackgroundChildImpl should delete its Transport. Review of attachment 8672091 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to mark r+ but I think the call should go in the ChildImpl (which matches the parent side where it happens in ParentImpl::MainThreadActorDestroy). ::: ipc/glue/BackgroundChildImpl.cpp @@ +99,5 @@ > { > // May happen on any thread! > + > + XRE_GetIOMessageLoop()->PostTask(FROM_HERE, > + new DeleteTask<Transport>(GetTransport())); I think the idea is that BackgroundChildImpl is a thin veneer only used to create new concrete child protocols and ChildImpl is the real brains of the operation. Therefore, this should probably go in the ChildImpl.
Attachment #8672091 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 6•9 years ago
|
||
I moved the PostTask to ChildImpl.
Summary: BackgroundChildImpl does not delete its transport → Background ChildImpl does not delete its transport
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fdd5ebadfbed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•