Closed Bug 719427 Opened 13 years ago Closed 13 years ago

Fix malloc logging tools on OS X

Categories

(Core :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Fix malloc logging tools on OS X (obsolete) — Splinter Review
      No description provided.
Attachment #589858 - Flags: review?(respindola)
Where are you seeing the assert fail?
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+
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+
Assignee: nobody → jmuizelaar
Status: NEW → ASSIGNED
Jeff, can this land?
Yes.
This was basically fixed by bug 737959
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
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: