OS X Shared Memory implementation leaks invisibly

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm7+, firefox41 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

STR:

1) Get a copy of Nightly from June 18, 2015. That has the patches for bug 1161166 in it.
2) Open the Nightly on OS X, and open several tabs.
3) Open the Activity Monitor and find the Nightly main process. Order by name so that it doesn't go out of view. Make note of how much memory the process is consuming.
4) Wait for the memory allocations to settle after start-up and tab opening.
5) Start switching between tabs.

ER:

Switching between tabs should not cause a memory leak.

AR:

We seem to leak about 25 MB per tab switch.

What's worse, this is not reflected at all in about:memory, outside of resident memory.
I suspect we want to get this backed out before rolling the next Nightly.

Unless someone comes up with a compelling reason not to, I will back this out by the end of the work day (Eastern time).
Whiteboard: [MemShrink]
Blocks: 1161166
ni'ing myself for backout of bug 1161166 if we can't find a patch for the leak.
Flags: needinfo?(mconley)
Assignee: nobody → mconley
Flags: needinfo?(mconley)
jrmuizel has found a solution. We were deallocating the wrong amount of memory.
Attachment #8624364 - Flags: review?(blassey.bugs)
Unilaterally marking M7 because this is a BFD.
tracking-e10s: --- → m7+
Could you also file a bug about adding some kind of memory reporter for this?
(In reply to Andrew McCreight [:mccr8] from comment #6)
> Could you also file a bug about adding some kind of memory reporter for this?

The existing shared memory reporter already tries to track this. In this case it wouldn't have caught it because internally we thought we had deallocated everything but had fail to properly communicate that to the OS
Comment on attachment 8624364 [details] [diff] [review]
Deallocate mach SharedMemory properly. r=?

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

Yup, this is right http://mxr.mozilla.org/mozilla-central/source/ipc/glue/SharedMemoryBasic_mach.cpp#509
Attachment #8624364 - Flags: review?(blassey.bugs) → review+
Comment on attachment 8624364 [details] [diff] [review]
Deallocate mach SharedMemory properly. r=?

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

::: ipc/glue/SharedMemoryBasic_mach.cpp
@@ +600,5 @@
>    if (!mMemory) {
>      return;
>    }
>    vm_address_t address = toVMAddress(mMemory);
> +  kern_return_t kr = vm_deallocate(mach_task_self(), address, round_page(mMappedSize));

Crikey!
https://hg.mozilla.org/mozilla-central/rev/656ea3bfbb4e

New OSX nightly triggered at mconley's request.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.