885158 - Don't let chromium IPC leak messages sent to closed channels

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Justin Lebar (not reading bugmail)]

mccr8 and I spent some time today trying to understand the "pickle" memory leaks in bug 880940 and bug 870588.

Our best guess is that what's actually happening is we're leaking IPC messages.  The majority of data in an IPC message is its data, which is generated by this Pickle object.

We looked for a way that the IPC code could possibly leak a Message object, and our best guess is that this is happening when we

 * close an IPC channel (which deletes all its pending messages)
 * send some messages on that channel (which enqueues them into an array)
 * don't delete the channel (perhaps because the channel's lifetime is tied to the lifetime of e.g. an <iframe mozbrowser> that we never kill).

Fixing this is simple enough, and I think it should be pretty safe.  I'm compiling a patch now.

Comment 1

[Andrew McCreight [:mccr8]]

There's a tantalizing comment in the code that suggests this was a known issue, but the comment was just deleted a few years later with a note that it was "no longer valid" and we couldn't figure out what might have fixed it.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The number of channels we have should be pretty small, are you sure one of them is closing?

Comment 3

[Justin Lebar (not reading bugmail)]

(In reply to ben turner [:bent] from comment #2)
> The number of channels we have should be pretty small, are you sure one of
> them is closing?

Do you mean, are we sure one of them is /not/ closing?

We can't reproduce this bug locally, so I don't know.  I think there's a good chance that this is causing the leak we're seeing in the DMD reports, but it's based only on code inspection.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 5

[Justin Lebar (not reading bugmail)]

Checking that this compiles on Windows: https://tbpl.mozilla.org/?tree=Try&rev=aae67cd82df6

Comment 6

[Andrew McCreight [:mccr8]]

This is the stack trace:
  https://bug870588.bugzilla.mozilla.org/attachment.cgi?id=747664
We have 255 blocks of Pickle data, for a total of about a megabyte.  There are similar things for volume adjustment, except with even more of it, around 7mb.  It seems like we're sending tons of stuff to the child, and they are building up in the queue.  In other monkey test runs, the DMD info is even worse.

Hooking up IPC to about:memory more might provide some better insight into what is happening.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Hooking up IPC to about:memory more might provide some better insight into
> what is happening.

This is going to be hard due to threading...

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 765433 [details] [diff] [review]
Patch, v1

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

r=me with the logging addressed:

::: ipc/chromium/src/chrome/common/ipc_channel_win.cc
@@ +124,5 @@
>    Logging::current()->OnSendMessage(message, L"");
>  #endif
>  
> +  if (closed_) {
> +    delete message;

I think we should add something to the logs when we do this otherwise we're going to confuse ourselves someday.

Comment 9

[Justin Lebar (not reading bugmail)]

Do you want a debug-time warning, or a release-time and debug-time warning?

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, I was thinking more along the lines of:

  #ifdef IPC_MESSAGE_LOG_ENABLED
    Logging::current()->OnSendMessageFailed(message, L"");
  #endif

But having a debug warning in stdout would also probably be good for non-IPC_MESSAGE_LOG builds

Comment 11

[Justin Lebar (not reading bugmail)]

I am totally behind adding the OnSendMessageFailed call.  But I realize now that an NS_WARNING can really clutter up stdout, and there's no easy way to turn it off.  Do you mind if I leave that out?

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Sounds fine for now. We can worry about it later if we need it.

Comment 13

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/projects/birch/rev/8c77605c9c5a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/projects/birch/rev/4d2681627e62

https://tbpl.mozilla.org/php/getParsedLog.php?id=24400466&tree=Birch

Comment 15

[Justin Lebar (not reading bugmail)]

Okay, that was silly of me.

Comment 16

[Justin Lebar (not reading bugmail)]

This should work better.

https://hg.mozilla.org/projects/birch/rev/1168d6e45277

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1168d6e45277

Comment 18

[Ed Morley [:emorley]]

Unfortunately had to back out for conflicts against bug 868919 which had landed on inbound earlier than this landed on birch (the birch merge just happened to be done a few mins before the inbound one). 

https://hg.mozilla.org/mozilla-central/rev/17908e758303

Comment 19

[Justin Lebar (not reading bugmail)]

Well, we'd been wondering how it would go when we hit a conflict between these two branches.  This doesn't seem so bad, if we don't hit this often.

Comment 20

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/projects/birch/rev/3392482de022

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3392482de022

Comment 22

[Justin Lebar (not reading bugmail)]

doug, can you please leo+ this?

Comment 23

[Doug Turner (:dougt)]

ask discussed, this will fix some/all(?) of the pickle leaking.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/9ab5e58cb884

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/9ab5e58cb884