Closed Bug 1217640 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Keywords: memory-leak, Whiteboard: [MemShrink])

Attachments

(2 files)

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.
Oops, that should be dom/plugins/test/mochitest/test_crashing.html.
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.
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.
Component: Plug-ins → IPC
Summary: test_crashing leaks → IPDL leaks messages when sending fails
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.
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.
Assignee: nobody → continuation
Summary: IPDL leaks messages when sending fails → MessageChannel::Call() leaks a message when the channel is not connected
I think this may have been introduced in the refactoring of bug 901789, but I can't really tell.
(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().
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
Attachment #8678487 - Flags: review?(jld)
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.)
Attachment #8678487 - Flags: review?(jld) → review+
https://hg.mozilla.org/mozilla-central/rev/4a87f75e1316
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.