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

RESOLVED FIXED in Firefox 44

Status

()

Core
IPC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({mlk})

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Oops, that should be dom/plugins/test/mochitest/test_crashing.html.
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
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
(Assignee)

Comment 4

2 years ago
Created attachment 8677782 [details]
allocation stacks of leaked messages
(Assignee)

Updated

2 years ago
Summary: test_crashing leaks → IPDL leaks messages when sending fails
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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
(Assignee)

Comment 7

2 years ago
I think this may have been introduced in the refactoring of bug 901789, but I can't really tell.
(Assignee)

Comment 8

2 years ago
(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().
(Assignee)

Comment 9

2 years ago
Created attachment 8678487 [details] [diff] [review]
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
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+

Comment 11

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a87f75e1316
https://hg.mozilla.org/mozilla-central/rev/4a87f75e1316
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.