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

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

54 Branch
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed, firefox58 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
This triggered neither locally (10.12) nor on try (10.10).
Attachment #8890719 - Flags: review?(mh+mozilla)
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 3

2 years ago
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.

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f2cf93fdcb4c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
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)
(Assignee)

Comment 6

2 years ago
Sorry, I forgot the "leave-open" keyword.
Status: RESOLVED → REOPENED
Flags: needinfo?(n.nethercote)
Keywords: leave-open
Resolution: FIXED → ---
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3729a7809983b9b69b1e3b79a0582345398ef93
Bug 1384814 - Remove critical address machinery from Mac implementation of MozStackWalk(). r=glandium.

Comment 9

2 years ago
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
Last Resolved: 2 years ago2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.