Last Comment Bug 719427 - Fix malloc logging tools on OS X
: Fix malloc logging tools on OS X
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Jeff Muizelaar [:jrmuizel]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-19 07:08 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-03-28 14:20 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix malloc logging tools on OS X (704 bytes, patch)
2012-01-19 07:08 PST, Jeff Muizelaar [:jrmuizel]
respindola: review+
Details | Diff | Splinter Review
Fix malloc logging tools on OS X v2 (1.17 KB, patch)
2012-02-14 10:43 PST, Jeff Muizelaar [:jrmuizel]
respindola: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-01-19 07:08:19 PST
Created attachment 589858 [details] [diff] [review]
Fix malloc logging tools on OS X
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-01-19 07:10:47 PST
Where are you seeing the assert fail?
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-01-19 07:15:08 PST
When using MallocStackLogging=1. Instruments also uses malloc_logger to track allocations.
Comment 3 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-01-19 07:28:45 PST
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] 2012-02-14 10:43:58 PST
Created attachment 597083 [details] [diff] [review]
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) 2012-02-14 10:57:21 PST
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] 2012-03-26 11:24:29 PDT
Jeff, can this land?
Comment 7 Jeff Muizelaar [:jrmuizel] 2012-03-26 11:36:06 PDT
Yes.
Comment 8 Jeff Muizelaar [:jrmuizel] 2012-03-27 10:49:23 PDT
This was basically fixed by bug 737959
Comment 9 Jeff Muizelaar [:jrmuizel] 2012-03-27 10:52:30 PDT
I checked in some left over comment changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe032073e04
Comment 10 Ed Morley [:emorley] 2012-03-28 14:20:56 PDT
https://hg.mozilla.org/mozilla-central/rev/fbe032073e04

Note You need to log in before you can comment on or make changes to this bug.