1175999 - OS X Shared Memory implementation leaks invisibly

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

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).

Comment 2

[Mike Conley (:mconley) (:⚙️)]

ni'ing myself for backout of bug 1161166 if we can't find a patch for the leak.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Deallocate mach SharedMemory properly. r=?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

jrmuizel has found a solution. We were deallocating the wrong amount of memory.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Unilaterally marking M7 because this is a BFD.

Comment 6

[Andrew McCreight [:mccr8]]

Could you also file a bug about adding some kind of memory reporter for this?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(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 8

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 9

[Nicholas Nethercote [inactive]]

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!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/656ea3bfbb4e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

New OSX nightly triggered at mconley's request.