719427 - Fix malloc logging tools on OS X

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Fix malloc logging tools on OS X

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Where are you seeing the assert fail?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

When using MallocStackLogging=1. Instruments also uses malloc_logger to track allocations.

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 589858 [details] [diff] [review]
Fix malloc logging tools on OS X

The r+ is conditional on the detection still working with Instruments on. That is, the assert

  MOZ_ASSERT(OnLionOrLater() || gCriticalAddress.mAddr != NULL);

should not fire on 10.6. If it fails, we have to disable this feature when we detect instruments :-( (or further refine the hack, which is probably not worth it).

Two nits
* Add a comment before

malloc_logger_t *old_malloc_logger = malloc_logger;

that instruments is the one setting the logger before us. Someone reading the code might conclude that old_malloc_logger is always NULL since nothing *in the code* sets it.

* Add a comment explaining why we do

  malloc_logger = NULL;
...
  malloc_logger = old_malloc_logger;

I assume it is because you don't want instruments to see a free for a malloc it missed?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Fix malloc logging tools on OS X v2

This version adds the comments, removes the null assignment, and I verified that gCriticalAddress is set on 10.6 when malloc_logger doesn't start as NULL.

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 597083 [details] [diff] [review]
Fix malloc logging tools on OS X v2

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

lgtm

Comment 6

[Josh Matthews [:jdm]]

Jeff, can this land?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Yes.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

This was basically fixed by bug 737959

Comment 9

[Jeff Muizelaar [:jrmuizel]]

I checked in some left over comment changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe032073e04

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fbe032073e04