Closed
Bug 1384814
Opened 7 years ago
Closed 7 years ago
Consider removing critical address machinery from Mac implementation of MozStackWalk()
Categories
(Core :: mozglue, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(1 file)
1.42 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
This triggered neither locally (10.12) nor on try (10.10).
Attachment #8890719 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f2cf93fdcb4c
Status: ASSIGNED → RESOLVED
Closed: 7 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•7 years ago
|
||
Sorry, I forgot the "leave-open" keyword.
Status: RESOLVED → REOPENED
Flags: needinfo?(n.nethercote)
Keywords: leave-open
Resolution: FIXED → ---
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Assignee | ||
Comment 8•7 years ago
|
||
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.
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c3729a780998
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•