Closed Bug 1089833 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

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

Attachments

(1 file)

Bill noticed this when we were looking at the code associated with the stacks in bug 1036562.  Maybe this will fix it.
Whiteboard: [MemShrink]
Assignee: nobody → continuation
MessageChannel::DispatchInterruptMessage has the same issue.
Summary: Delete reply in MessageChannel::DispatchSyncMessage if channel isn't connected → Delete reply in MessageChannel::DispatchSyncMessage and DispatchInterruptMessage if channel isn't connected
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+
tree's closed
Keywords: checkin-needed
Whiteboard: [MemShrink] → [MemShrink:P2]
https://hg.mozilla.org/mozilla-central/rev/d2c6c62f63bf
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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)
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 :)
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?
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)?
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.