Closed Bug 1080096 Opened 7 years ago Closed 7 years ago

[e10s] UDPSocket and UDPSocketChild form a cycle

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
blocking-b2g 2.1+
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)

No description provided.
Assignee: nobody → continuation
We may need to also Traverse mSocketChild, but that will require CC-ifying UDPSocketChildBase.
Assignee: continuation → nobody
Attachment #8503285 - Flags: review?(bugs)
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)
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.
Summary: UDPSocket should Traverse mSocket → UDPSocket should Traverse mSocketBase
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]
The UDPSocketChildBase add an extra refcount which represents IPC channel is holding this object. Will this make UDPSocketChild still not cycle-collectable?
(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.)
I think this is a regression from bug 745283, so maybe it should get backported to B2G branches.
Blocks: 745283
This fixes the leak for me locally.
(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?
(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.
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.
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()|.
(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.
Blocks: 1087613
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+
[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?
https://hg.mozilla.org/mozilla-central/rev/8ebe9660d12e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(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.
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?
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)
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?
blocking-b2g: 2.1? → 2.1+
Attachment #8514564 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Unable to verify as it is a back-end issue.
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
Depends on: 1100686
No longer depends on: 1100686
You need to log in before you can comment on or make changes to this bug.