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

Status: RESOLVED FIXED

(Core :: mozglue, enhancement)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Add a diagnostic assertion to detect any use of the critical address machinery

This triggered neither locally (10.12) nor on try (10.10).

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

[Nicholas Nethercote [inactive]]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f2cf93fdcb4c

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should this bug actually be kept open instead?  Or should a new bug be filed to actually do the removal?

Comment 6

[Nicholas Nethercote [inactive]]

Sorry, I forgot the "leave-open" keyword.

Comment 7

[Nicholas Nethercote [inactive]]

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.

Comment 8

[Nicholas Nethercote [inactive]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3729a780998
Remove critical address machinery from Mac implementation of MozStackWalk(). r=glandium.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c3729a780998