Closed Bug 1384814 Opened 3 years ago Closed 3 years ago

Consider removing critical address machinery from Mac implementation of MozStackWalk()

Categories

(Core :: mozglue, enhancement)

54 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed
firefox58 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(1 file)

The Mac implementation of MozStackWalk() has a bunch of machinery involving
a "critical address" which is unsafe to wind past.

It was created in bug 478195 in response to a deadlock that was occurring in
trace-malloc builds. (trace-malloc has since been replaced with DMD.) It
detects the presence of a new_sem_from_pool function.

Bug 696376 modified the part that identifies new_sem_from_pool, because dlsym
stopped working for that in Mac OS 10.6. Furthermore,
https://hg.mozilla.org/mozilla-central/rev/c9a74f4ee1f7 says "In 10.7 the call
to malloc is gone, so we don't have to worry about critical addresses on it
anymore."

This claim was corroborated by the following assertion in StackWalk.cpp that
indicates the critical address machinery was unused in 10.7 (Lion) and later.

> MOZ_ASSERT(OnLionOrLater() || gCriticalAddress.mAddr != nullptr);

Note that this assertion was removed in bug 1287937. That bug looks like it was
just getting rid of now-unnecessary Mac version checks, i.e. the assertion was
constant-folded away to true and thus removed without consideration of whether
the stuff used in the right disjunct was still needed.

The minimum Mac OS version we support is now 10.9, so there's a good chance we
don't need any of this machinery any more.
This triggered neither locally (10.12) nor on try (10.10).
Attachment #8890719 - Flags: review?(mh+mozilla)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8890719 [details] [diff] [review]
Add a diagnostic assertion to detect any use of the critical address machinery

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

s/problem/probe/ in the commit message. Note, I'm not even sure we have things hooked up to be able to call telemetry from mozglue.
Attachment #8890719 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2cf93fdcb4c1662470b75275b2bf36943ffe264
Bug 1384814 - Add a diagnostic assertion to detect any use of the critical address machinery. r=glandium.
https://hg.mozilla.org/mozilla-central/rev/f2cf93fdcb4c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Should this bug actually be kept open instead?  Or should a new bug be filed to actually do the removal?
Flags: needinfo?(n.nethercote)
Sorry, I forgot the "leave-open" keyword.
Status: RESOLVED → REOPENED
Flags: needinfo?(n.nethercote)
Keywords: leave-open
Resolution: FIXED → ---
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3729a7809983b9b69b1e3b79a0582345398ef93
Bug 1384814 - Remove critical address machinery from Mac implementation of MozStackWalk(). r=glandium.
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3729a780998
Remove critical address machinery from Mac implementation of MozStackWalk(). r=glandium.
https://hg.mozilla.org/mozilla-central/rev/c3729a780998
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.