Closed Bug 1384814 Opened 3 years ago Closed 3 years ago
Consider removing critical address machinery from Mac implementation of Moz
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.
Should this bug actually be kept open instead? Or should a new bug be filed to actually do the removal?
Sorry, I forgot the "leave-open" keyword.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
No sign of this diagnostic assertion triggering: https://crash-stats.mozilla.com/search/?moz_crash_reason=~gCriticalAddress&date=%3E%3D2017-07-03T02%3A39%3A04.000Z&date=%3C2017-10-03T02%3A39%3A04.000Z&page=1&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature I think it's safe to remove this code now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3729a7809983b9b69b1e3b79a0582345398ef93 Bug 1384814 - Remove critical address machinery from Mac implementation of MozStackWalk(). r=glandium.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c3729a780998 Remove critical address machinery from Mac implementation of MozStackWalk(). r=glandium.
You need to log in before you can comment on or make changes to this bug.