Leaking ipc::Transport from 'opens' protocols

RESOLVED FIXED in Firefox 22

Status

()

Core
Graphics: Layers
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
mozilla22
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(3 attachments)

Created attachment 714740 [details] [diff] [review]
Smoking gun

CrossProcessCompositorParent and ImageBridgeParent are given pointers to ipc::Transport when allocated.  They hand these down into AsyncChannel on Open().

AsyncChannel doesn't own the Transport, it only borrows it.  The Transport is owned by the thread or process host.

CrossProcessCompositorParent and ImageBridgeParent don't free the Transports so we accumulate a leak proportional to the number of apps opened and closed/lmk'd.  The amount of memory held by Transport* can be fairly substantial because of buffers and queues, into the 10-100KB range.  This can cause substantial b2g-process memory usage and severely destabilize the system.

We need to fix this to hit stability requirements.  Luckily the patch is small and safe.
I reproduced the symptom in bug 838869 with this patch.
Blocks: 838869
(If possible landing a fix for this before we start v1.0.1 stability test on Sunday night would be rad.)
I was totally going to suggest that you take the upcoming fix whether it's landed by then or not ;).
Created attachment 714755 [details] [diff] [review]
Part 1: Destroy the CrossProcessCompositorParent channel

Sorry for the review spam; this is a straightforward cleanup that doesn't require any knowledge of PCompositor, just a bit of general IPC.
Attachment #714755 - Flags: review?(roc)
Attachment #714755 - Flags: review?(nical.bugzilla)
Attachment #714755 - Flags: review?(kchen)
Attachment #714755 - Flags: review?(justin.lebar+bug)
Attachment #714755 - Flags: review?(bent.mozilla)
Created attachment 714756 [details] [diff] [review]
Part 2: Clean up ImageBridge

This one is trickier than I'd hoped, because we were previously just completely leaking ImageBridgeParent :/.

The changes here are
 - use the same memory management scheme for ImageBridgeParent as CrossProcessCompositorChild: it keeps itself alive until ActorDestroy()
 - make the same change as in Part 1 to clean up the Transport*, if there is one (there isn't one for cross-thread ImageBridge)
 - stop freeing the image map from ~ImageBridgeParent, since that's a static and there will be many ImageBridgeParents

Apologies again for review spam.
Attachment #714756 - Flags: review?
Comment on attachment 714756 [details] [diff] [review]
Part 2: Clean up ImageBridge

Damn you bz.
Attachment #714756 - Flags: review?(roc)
Attachment #714756 - Flags: review?(nical.bugzilla)
Attachment #714756 - Flags: review?(kchen)
Attachment #714756 - Flags: review?
The test case here is to apply the smoking gun patch and then launch and kill the camera over and over.  b2g process memory usage grows without bound.
Comment on attachment 714755 [details] [diff] [review]
Part 1: Destroy the CrossProcessCompositorParent channel

Looks fine to me. We should look into adding leak instrumentation to IPC::Channel and any other classes we can think of...
Attachment #714755 - Flags: review?(roc)
Attachment #714755 - Flags: review?(nical.bugzilla)
Attachment #714755 - Flags: review?(kchen)
Attachment #714755 - Flags: review?(justin.lebar+bug)
Attachment #714755 - Flags: review?(bent.mozilla)
Attachment #714755 - Flags: review+
New world's record! :)  Thanks much.

Yes, we should.  The problem is all that is in ipc/chromium ...  I'll file a bug though.
https://tbpl.mozilla.org/?tree=Try&rev=ca044c974dd9
(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> Yes, we should.  The problem is all that is in ipc/chromium ...

Well, instead of doing:

  typedef IPC::Channel Transport;

We could make our own class that inherits it and adds logging.
That's an option.  I filed bug 842003 btw.
Comment on attachment 714756 [details] [diff] [review]
Part 2: Clean up ImageBridge

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

::: gfx/layers/ipc/ImageBridgeParent.h
@@ +51,5 @@
> +  MessageLoop* mMessageLoop;
> +  Transport* mTransport;
> +  // This keeps us alive until ActorDestroy(), at which point we do a
> +  // deferred destruction of ourselves.
> +  nsRefPtr<ImageBridgeParent> mSelfRef;

Why do we need this field? Can't we just NS_ADDREF(this)?
Definitely.  But
 - that's what CrossProcessCompositorParent does
 - NS_ADDREF(this) creates a nonlocal invariant, so it's not obvious that the object holds a reference to itself

There's only one of these per process, so I don't think the sizeof(void*) memory difference is worth the programmer overhead.

I could certainly be convinced otherwise, though.
Whiteboard: [MemShrink]
> New world's record! :)  Thanks much.

Nah;  bz once r+'d a patch of mine less than 4 minutes after I posted it :)
Attachment #714756 - Flags: review?(kchen) → review+
(In reply to Nicholas Nethercote [:njn] from comment #15)
> Nah;  bz once r+'d a patch of mine less than 4 minutes after I posted it :)

/me buries the medal in an undisclosed location, cries himself to sleep.
Actually I lie;  it was 4 minutes 31 seconds:  bug 826521 comment 25 :)

Updated

4 years ago
Attachment #714756 - Flags: review?(nical.bugzilla) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> https://tbpl.mozilla.org/?tree=Try&rev=ca044c974dd9

Infra decided to ignore this for some reason, so going to push on a wing and a prayer.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a865bbcdc06d
Whiteboard: [MemShrink] → [MemShrink:P1]
https://hg.mozilla.org/mozilla-central/rev/a865bbcdc06d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c2dfc2565ae0
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/140d06fc3397
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → fixed
status-firefox20: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → fixed
Blocks: 841842
No longer blocks: 841842
Attachment #714756 - Flags: review?(roc)
You need to log in before you can comment on or make changes to this bug.