Closed
Bug 841993
Opened 11 years ago
Closed 11 years ago
Leaking ipc::Transport from 'opens' protocols
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
People
(Reporter: cjones, Assigned: cjones)
References
Details
(Whiteboard: [MemShrink:P1])
Attachments
(3 files)
5.50 KB,
patch
|
Details | Diff | Splinter Review | |
2.78 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
7.17 KB,
patch
|
kanru
:
review+
nical
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
I reproduced the symptom in bug 838869 with this patch.
Blocks: 838869
Comment 2•11 years ago
|
||
(If possible landing a fix for this before we start v1.0.1 stability test on Sunday night would be rad.)
Assignee | ||
Comment 3•11 years ago
|
||
I was totally going to suggest that you take the upcoming fix whether it's landed by then or not ;).
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
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?
Assignee | ||
Comment 6•11 years ago
|
||
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?
Assignee | ||
Comment 7•11 years ago
|
||
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+
Assignee | ||
Comment 9•11 years ago
|
||
New world's record! :) Thanks much. Yes, we should. The problem is all that is in ipc/chromium ... I'll file a bug though.
Assignee | ||
Comment 10•11 years ago
|
||
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.
Assignee | ||
Comment 12•11 years ago
|
||
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)?
Assignee | ||
Comment 14•11 years ago
|
||
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.
Updated•11 years ago
|
Whiteboard: [MemShrink]
![]() |
||
Comment 15•11 years ago
|
||
> New world's record! :) Thanks much.
Nah; bz once r+'d a patch of mine less than 4 minutes after I posted it :)
Updated•11 years ago
|
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.
![]() |
||
Comment 17•11 years ago
|
||
Actually I lie; it was 4 minutes 31 seconds: bug 826521 comment 25 :)
Updated•11 years ago
|
Attachment #714756 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 18•11 years ago
|
||
(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.
Assignee | ||
Comment 19•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a865bbcdc06d
![]() |
||
Updated•11 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a865bbcdc06d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 21•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Attachment #714756 -
Flags: review?(roc)
You need to log in
before you can comment on or make changes to this bug.
Description
•