Closed Bug 885158 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

(Whiteboard: [MemShrink][LeoVB+])

Attachments

(1 file)

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.
Whiteboard: [MemShrink]
Assignee: nobody → justin.lebar+bug
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.
The number of channels we have should be pretty small, are you sure one of them is closing?
(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.
Attached patch Patch, v1Splinter Review
Attachment #765433 - Flags: review?(bent.mozilla)
Checking that this compiles on Windows: https://tbpl.mozilla.org/?tree=Try&rev=aae67cd82df6
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.
(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 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.
Attachment #765433 - Flags: review?(bent.mozilla) → review+
Do you want a debug-time warning, or a release-time and debug-time warning?
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
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?
Sounds fine for now. We can worry about it later if we need it.
Okay, that was silly of me.
https://hg.mozilla.org/mozilla-central/rev/1168d6e45277
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
https://hg.mozilla.org/mozilla-central/rev/3392482de022
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
doug, can you please leo+ this?
blocking-b2g: --- → leo?
Flags: needinfo?(doug.turner)
ask discussed, this will fix some/all(?) of the pickle leaking.
blocking-b2g: leo? → leo+
Flags: needinfo?(doug.turner)
Whiteboard: [MemShrink] → [MemShrink][LeoVB+]
You need to log in before you can comment on or make changes to this bug.