1217640 - MessageChannel::Call() leaks a message when the channel is not connected

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Andrew McCreight [:mccr8]]

In bug 1089837, I added some patches to track whether we leak IPC::Messages. It turns out, we do. At least one of the tests that leaks is dom/plugins/test/mochitest/mochitest.ini, which leaks 12 Messages. This could possibly have been detected by LSan, but the crash reporter is disabled in ASan builds, so we don't run this test there.

Comment 1

[Andrew McCreight [:mccr8]]

Oops, that should be dom/plugins/test/mochitest/test_crashing.html.

Comment 2

[Andrew McCreight [:mccr8]]

test_crashing2.html does not leak, but there might be at least one other test that leaks, as there are a total of 15 messages leaking in a dom/plugins run.

Comment 3

[Andrew McCreight [:mccr8]]

I think this is an IPDL bug. I used XPCOM_MEM_LOG_CLASSES=IPC::Message to get the allocation stacks for the leaking objects. I'll attach that. One of the leaking places was in this call:

    bool sendok__ = (mChannel)->Call(msg__, (&(reply__)));
    if ((!(sendok__))) {
        return false;
    }

I'm guessing the send fails, because the child crashed. I'm also guessing that normally the Call takes ownership of the message but I guess it doesn't here? I'll look into that some more. Anyways, if I add in delete msg__; before that return then the number of leaks goes down from 12 to 7. There are some other methods involved, so presumably they have the same issue.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: allocation stacks of leaked messages

Comment 5

[Andrew McCreight [:mccr8]]

I'm not sure exactly where the error is happening. I traced things through by reading the source code as far as Channel::ChannelImpl::OutputQueuePush(Message* msg) and it looks okay to me.

Comment 6

[Andrew McCreight [:mccr8]]

The bug is here:

    if (!Connected()) {
        ReportConnectionError("MessageChannel::Call", aMsg);
        return false;
    }

aMsg should be deleted. The fix is to move the nsAutoPtr<> on aMsg up earlier.

Comment 7

[Andrew McCreight [:mccr8]]

I think this may have been introduced in the refactoring of bug 901789, but I can't really tell.

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #7)
> I think this may have been introduced in the refactoring of bug 901789, but
> I can't really tell.

Never mind, it looks like it was a pre-existing bug in RPCChannel::Call().

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: MessageChannel::Call() should delete aMsg when the channel is not connected.

At some point, these methods should use move constructor or something, but this is okay for now. I skimmed through the other functions in this file and they didn't seem to have this particular problem.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a074577f1a6

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8678487 [details] [diff] [review]
MessageChannel::Call() should delete aMsg when the channel is not connected.

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

Nice catch.  (Also, if it had been a case of deliberate “takes ownership if successful” instead of just a mistake, I'd have suggested changing it.)

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a87f75e1316

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a87f75e1316