Fix malloc logging tools on OS X

RESOLVED FIXED in mozilla14

Status

()

Core
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla14
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 589858 [details] [diff] [review]
Fix malloc logging tools on OS X
Attachment #589858 - Flags: review?(respindola)
Where are you seeing the assert fail?
(Assignee)

Comment 2

6 years ago
When using MallocStackLogging=1. Instruments also uses malloc_logger to track allocations.
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?
Attachment #589858 - Flags: review?(respindola) → review+
(Assignee)

Comment 4

5 years ago
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.
Attachment #589858 - Attachment is obsolete: true
Attachment #597083 - Flags: review?(respindola)
Comment on attachment 597083 [details] [diff] [review]
Fix malloc logging tools on OS X v2

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

lgtm
Attachment #597083 - Flags: review?(respindola) → review+

Updated

5 years ago
Assignee: nobody → jmuizelaar

Updated

5 years ago
Status: NEW → ASSIGNED

Comment 6

5 years ago
Jeff, can this land?
(Assignee)

Comment 7

5 years ago
Yes.
(Assignee)

Comment 8

5 years ago
This was basically fixed by bug 737959
(Assignee)

Comment 9

5 years ago
I checked in some left over comment changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe032073e04
https://hg.mozilla.org/mozilla-central/rev/fbe032073e04
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.