Background ChildImpl does not delete its transport

RESOLVED FIXED in Firefox 44

Status

()

Core
IPC
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({memory-leak})

Trunk
mozilla44
memory-leak
Points:
---

Firefox Tracking Flags

(e10s+, firefox44 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Blocks: 1212987
(Assignee)

Updated

3 years ago
No longer blocks: 1212987
tracking-e10s: --- → ?
Keywords: mlk
Whiteboard: [MemShrink]
(Assignee)

Updated

3 years ago
Blocks: 1212987
(Assignee)

Comment 1

3 years ago
Created attachment 8672091 [details] [diff] [review]
BackgroundChildImpl should delete its Transport.

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

3 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.
Whiteboard: [MemShrink] → [MemShrink:P2]
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

3 years ago
I moved the PostTask to ChildImpl.
Summary: BackgroundChildImpl does not delete its transport → Background ChildImpl does not delete its transport
https://hg.mozilla.org/mozilla-central/rev/fdd5ebadfbed
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.