1080096 - [e10s] UDPSocket and UDPSocketChild form a cycle

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Andrew McCreight [:mccr8]]

We may need to also Traverse mSocketChild, but that will require CC-ifying UDPSocketChildBase.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: UDPSocket should Traverse mSocket.

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

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.

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

The UDPSocketChildBase add an extra refcount which represents IPC channel is holding this object. Will this make UDPSocketChild still not cycle-collectable?

Comment 7

[Andrew McCreight [:mccr8]]

(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.)

Comment 8

[Andrew McCreight [:mccr8]]

I think this is a regression from bug 745283, so maybe it should get backported to B2G branches.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Cycle collect UDPSocketChildBase.

This fixes the leak for me locally.

Comment 10

[Andrew McCreight [:mccr8]]

(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.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Rather than cycle collecting could we just drop mSocket in ReleaseIPDLReference?

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: Clear UDPSocketChildBase::mSocket in ReleaseIPDLReference(). (alternate fix)

Comment 14

[Andrew McCreight [:mccr8]]

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

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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()|.

Comment 16

[Andrew McCreight [:mccr8]]

(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.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: Clear UDPSocketChildBase::mSocket in ReleaseIPDLReference().

try run looks okay: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4836d88b6fb5

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 19

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ebe9660d12e

Comment 20

[Andrew McCreight [:mccr8]]

[Blocking Requested - why for this release]: leaks from feature added in bug 745283, which looks like it landed for 2.1.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ebe9660d12e

Comment 22

[Andrew McCreight [:mccr8]]

(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 23

[Andrew McCreight [:mccr8]]

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

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 25

[Andrew McCreight [:mccr8]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/e6f4797bcc1b

Comment 27

[Yeojin Chung [:YeojinC]]

Unable to verify as it is a back-end issue.