Closed
Bug 1080096
Opened 11 years ago
Closed 10 years ago
[e10s] UDPSocket and UDPSocketChild form a cycle
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
Tracking | Status | |
---|---|---|
e10s | + | --- |
firefox34 | --- | wontfix |
firefox35 | --- | wontfix |
firefox36 | --- | fixed |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | fixed |
b2g-v2.2 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Keywords: memory-leak, Whiteboard: [MemShrink])
Attachments
(1 file, 3 obsolete files)
806 bytes,
patch
|
schien
:
review+
bajaj
:
approval-mozilla-b2g34+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → continuation
Assignee | ||
Comment 1•11 years ago
|
||
We may need to also Traverse mSocketChild, but that will require CC-ifying UDPSocketChildBase.
Assignee | ||
Updated•11 years ago
|
Assignee: continuation → nobody
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #8503285 -
Flags: review?(bugs)
Assignee | ||
Comment 3•11 years ago
|
||
Comment on attachment 8503285 [details] [diff] [review]
UDPSocket should Traverse mSocket.
I was confused somehow. The only implementation of the mSocket class is a non-CCed thing.
Attachment #8503285 -
Attachment is obsolete: true
Attachment #8503285 -
Flags: review?(bugs)
Assignee | ||
Comment 4•11 years ago
|
||
Maybe this isn't needed, but mSocketChild can be implemented by UDPSocketChildBase, which has a strong reference to a UDPSocket, so maybe we should make UDPSocketChildBase CCed.
Assignee | ||
Updated•11 years ago
|
Summary: UDPSocket should Traverse mSocket → UDPSocket should Traverse mSocketBase
Assignee | ||
Comment 5•10 years ago
|
||
If you run just the directory dom/network/tests/ using e10s, then we leak some windows associated with test_udpsocket.html. Looking at the heap, it seems like there is a cycle between a UDPSocket and UDPSocketChild. Looking at the class declarations, UDPSocketChild inherits from UDPSocketChildBase, which has a field nsCOMPtr<nsIUDPSocketInternal> mSocket, which is probably the UDPSocket. UDPSocket has a field nsCOMPtr<nsIUDPSocketChild> mSocketChild, so this forms a strong cycle. The fix is to cycle collect UDPSocketChild, and add all references to it to the CC graph, I think.
Assignee: nobody → continuation
Blocks: 1051230
tracking-e10s:
--- → ?
Keywords: mlk
Summary: UDPSocket should Traverse mSocketBase → [e10s] UDPSocket and UDPSocketChild form a cycle
Whiteboard: [MemShrink]
Comment 6•10 years ago
|
||
The UDPSocketChildBase add an extra refcount which represents IPC channel is holding this object. Will this make UDPSocketChild still not cycle-collectable?
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #6)
> The UDPSocketChildBase add an extra refcount which represents IPC channel is
> holding this object. Will this make UDPSocketChild still not
> cycle-collectable?
That should be okay. The cycle collector will just never collect it as long as the channel is holding that extra reference. (We could improve the CC performance on this object by taking advantage of that, but there's no need to do that unless we see that it is a problem.)
Assignee | ||
Comment 8•10 years ago
|
||
I think this is a regression from bug 745283, so maybe it should get backported to B2G branches.
Blocks: 745283
Assignee | ||
Comment 9•10 years ago
|
||
This fixes the leak for me locally.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #8)
> I think this is a regression from bug 745283, so maybe it should get
> backported to B2G branches.
Well, I suppose this is a child process leak, so if child processes are getting OOM killed all the time in practice it probably isn't too big of a deal. But something to keep in mind.
Rather than cycle collecting could we just drop mSocket in ReleaseIPDLReference?
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> Rather than cycle collecting could we just drop mSocket in
> ReleaseIPDLReference?
That also fixes the leak for me locally, and would be simpler.
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
Well, that second patch actually crashes. So I guess I didn't rebuild properly when I was testing it the first time. But maybe that can be worked around. I'll look into it tomorrow.
Comment 15•10 years ago
|
||
Maybe move the |mSocket = nullptr| above the |this->Release()| can solve the crash. I assume the instance of UDPSocketChild will be deleted while doing |this->Release()|.
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #15)
> Maybe move the |mSocket = nullptr| above the |this->Release()| can solve the
> crash. I assume the instance of UDPSocketChild will be deleted while doing
> |this->Release()|.
That fixes the crash, thanks.
Assignee | ||
Comment 17•10 years ago
|
||
try run looks okay: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4836d88b6fb5
Attachment #8513904 -
Attachment is obsolete: true
Attachment #8513911 -
Attachment is obsolete: true
Attachment #8514564 -
Flags: review?(schien)
Comment 18•10 years ago
|
||
Comment on attachment 8514564 [details] [diff] [review]
Clear UDPSocketChildBase::mSocket in ReleaseIPDLReference().
Review of attachment 8514564 [details] [diff] [review]:
-----------------------------------------------------------------
Nice and simple! Thanks for working on this.
Attachment #8514564 -
Flags: review?(schien) → review+
Assignee | ||
Comment 19•10 years ago
|
||
![]() |
||
Updated•10 years ago
|
Assignee | ||
Comment 20•10 years ago
|
||
[Blocking Requested - why for this release]: leaks from feature added in bug 745283, which looks like it landed for 2.1.
blocking-b2g: --- → 2.1?
Comment 21•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #20)
> [Blocking Requested - why for this release]: leaks from feature added in bug
> 745283, which looks like it landed for 2.1.
This is a pretty nasty leak, in that it can leak windows and documents. The mitigating factor is that it is in the child process, so a leak will just eventually OOM kill it. Running locally with multiprocess desktop Firefox, I saw this every time I ran the dom/network/test directory, but on TBPL it was fairly intermittent, so I'm not sure how common this is.
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8514564 [details] [diff] [review]
Clear UDPSocketChildBase::mSocket in ReleaseIPDLReference().
Approval Request Comment
[Feature/regressing bug #]: bug 745283
[User impact if declined]: bad child process leaks on B2G
[Describe test coverage new/current, TBPL]: TBPL tests
[Risks and why]: no risk for desktop, seems low risk for B2G
[String/UUID change made/needed]: none
Attachment #8514564 -
Flags: approval-mozilla-beta?
Attachment #8514564 -
Flags: approval-mozilla-aurora?
Comment 24•10 years ago
|
||
From comment 23 I take it that this fix doesn't impact desktop or android at all. If that's correct, there is no value in uplifting to Beta or Aurora. Instead you should request uplift for:
2.1: b2g34
2.0: b2g32
1.4: b2g30
Flags: needinfo?(continuation)
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 8514564 [details] [diff] [review]
Clear UDPSocketChildBase::mSocket in ReleaseIPDLReference().
Alright. I was thinking I could just land there and have it automerged over from beta, but I can just do it like this instead.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 745283
User impact if declined: possible severe child process memory leaks
Testing completed: TBPL
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Flags: needinfo?(continuation)
Attachment #8514564 -
Flags: approval-mozilla-beta?
Attachment #8514564 -
Flags: approval-mozilla-b2g34?
Attachment #8514564 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
blocking-b2g: 2.1? → 2.1+
Updated•10 years ago
|
Attachment #8514564 -
Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Comment 26•10 years ago
|
||
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → fixed
status-b2g-v2.2:
--- → fixed
status-firefox34:
--- → wontfix
status-firefox35:
--- → wontfix
status-firefox36:
--- → fixed
Comment 27•10 years ago
|
||
Unable to verify as it is a back-end issue.
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
You need to log in
before you can comment on or make changes to this bug.
Description
•