Delete reply in MessageChannel::DispatchSyncMessage and DispatchInterruptMessage if channel isn't connected

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({memory-leak})

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed, firefox36 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

Assignee

Description

5 years ago
Bill noticed this when we were looking at the code associated with the stacks in bug 1036562.  Maybe this will fix it.
Assignee

Updated

5 years ago
Whiteboard: [MemShrink]
Assignee

Updated

5 years ago
Assignee: nobody → continuation
Assignee

Comment 1

5 years ago
MessageChannel::DispatchInterruptMessage has the same issue.
Assignee

Updated

5 years ago
Summary: Delete reply in MessageChannel::DispatchSyncMessage if channel isn't connected → Delete reply in MessageChannel::DispatchSyncMessage and DispatchInterruptMessage if channel isn't connected
Assignee

Comment 2

5 years ago
It looks like this bug is fairly old, pre-CPOWs. I converted this to nsAutoPtr<>, because that will be less error-prone. I filed bug 1090374 for making mLink an nsAutoPtr, which will fix the last instance of "delete" in MessageChannel.cpp.

try run for an older version of the patch that did not use autoptr: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=241113afe6c5

bc1 did not hit the leak in 20 or so retriggers, so I think there's a decent chance this is the fix.
Attachment #8512796 - Flags: review?(wmccloskey)
Attachment #8512796 - Flags: review?(wmccloskey) → review+
Assignee

Comment 3

5 years ago
tree's closed
Keywords: checkin-needed
Whiteboard: [MemShrink] → [MemShrink:P2]
https://hg.mozilla.org/mozilla-central/rev/d2c6c62f63bf
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee

Comment 6

5 years ago
Comment on attachment 8512796 [details] [diff] [review]
Delete reply in MessageChannel::DispatchSyncMessage and DispatchInterruptMessage if channel isn't connected.

Approval Request Comment
[Feature/regressing bug #]: old
[User impact if declined]: intermittent memory leaks on e10s and B2G
[Describe test coverage new/current, TBPL]: TBPL
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8512796 - Flags: approval-mozilla-aurora?
I believe that this affects Beta34 as well?
Flags: needinfo?(continuation)
Assignee

Comment 8

5 years ago
Yeah, it probably affects everything, but I wasn't sure how worthwhile it would be to put on beta given that it shouldn't affect non-e10s desktop.  Plus it doesn't seem to happen too much on TBPL outside of trunk. Is there some B2G branch that will be forking off beta?
Flags: needinfo?(continuation)
b2g34 takes merges from beta34 :)
Assignee

Comment 10

5 years ago
Comment on attachment 8512796 [details] [diff] [review]
Delete reply in MessageChannel::DispatchSyncMessage and DispatchInterruptMessage if channel isn't connected.

Ah, cool, sounds like a good reason to backport.

Approval Request Comment
[Feature/regressing bug #]: old
[User impact if declined]: B2G leaks
[Describe test coverage new/current, TBPL]: TBPL tests
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8512796 - Flags: approval-mozilla-beta?
Assignee

Updated

5 years ago
Keywords: mlk
Comment on attachment 8512796 [details] [diff] [review]
Delete reply in MessageChannel::DispatchSyncMessage and DispatchInterruptMessage if channel isn't connected.

Approving the low-risk backport because of b2g branching.
Attachment #8512796 - Flags: approval-mozilla-beta?
Attachment #8512796 - Flags: approval-mozilla-beta+
Attachment #8512796 - Flags: approval-mozilla-aurora?
Attachment #8512796 - Flags: approval-mozilla-aurora+
Is this something that might even be wanted for b2g32 (v2.0)?
Assignee

Comment 15

5 years ago
It doesn't seem to worthwhile to me.  We occasionally get a leak of 48 bytes on one test, so it doesn't seem to be all that frequent, though I suppose b2g could be different.
You need to log in before you can comment on or make changes to this bug.