1396361 - Crash in <name omitted> | -[NSApplication _doUnhideWithoutActivationMaybeFakingIt:]

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-9d037533-3112-436d-b5d9-fdfd60170903.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	libmozglue.dylib 	<name omitted> 	memory/build/zone.c:159
1 	AppKit 	-[NSApplication _doUnhideWithoutActivationMaybeFakingIt:] 	
2 	AppKit 	-[NSApplication sendEvent:] 	
3 	XUL 	-[GeckoNSApplication sendEvent:] 	widget/cocoa/nsAppShell.mm:113
4 	AppKit 	-[NSApplication run] 	
5 	XUL 	nsAppShell::Run() 	widget/cocoa/nsAppShell.mm:683
6 	XUL 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp:287
7 	XUL 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp:4615
8 	XUL 	XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4772
9 	XUL 	XRE_main(int, char**, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4867
10 	firefox 	main 	browser/app/nsBrowserApp.cpp:236
11 	firefox 	start

these crash reports are popping up in the 56 cycle on macos installations with MOZ_RELEASE_ASSERT(zone) that got added in bug 1356701.

Comment 1

[Mike Hommey [:glandium]]

The crashing code looks like this:

static void
other_zone_free(malloc_zone_t* original_zone, void* ptr)
{
  // Sometimes, system libraries call malloc_zone_* functions with the wrong
  // zone (e.g. CoreFoundation does). In that case, we need to find the real
  // one. We can't call libSystem's free directly because we're exporting
  // free from libmozglue and we'd pick that one, so we manually find the
  // right zone and free with it.
  malloc_zone_t* zone = malloc_zone_from_ptr(ptr);
  // The system allocator crashes voluntarily by default when a pointer can't
  // be traced back to a zone. Do the same.
  MOZ_RELEASE_ASSERT(zone);
  MOZ_RELEASE_ASSERT(zone != original_zone);
  return malloc_zone_free(zone, ptr);
}

So somehow, this function is given a pointer that doesn't belong to any zone, such that malloc_zone_from_ptr returns NULL. And as mentioned in the comments, the system allocator crashes voluntarily in the same situation, per the definition of free() in https://opensource.apple.com/source/libmalloc/libmalloc-116.50.8/src/malloc.c.auto.html (malloc_debug_flags defaults to MALLOC_ABORT_ON_CORRUPTION per set_flags_from_environment).

Now, the notable difference between this code and the code from system free in libmalloc is that system free returns early when the pointer is NULL. It's plausible that malloc_zone_from_ptr would return NULL for a NULL pointer, and would lead to this crash.

Following the disassembly for the crashing code, it appears the rbx register contains the pointer passed into this function, and the crash reports do contain the value for rbx, which is 0, validating the hypothesis above.

What this means is that not only some system libraries do call malloc_zone_free with the wrong zone, but they even call it for NULL pointers...

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1396361 - Avoid crashing when some system library calls malloc_zone_free(zone, NULL).

Some system libraries call malloc_zone_free directly instead of free,
and sometimes they do that with the wrong zone. When that happens, we
circle back, trying to find the right zone, and call malloc_zone_free
with the right one, but when we can't find one, we crash, which matches
what the system free() would do. Except in one case where the pointer
we're being passed is NULL, in which case we can't trace it back to any
zone, but shouldn't crash (system free() explicitly doesn't crash in
that case).

Review commit: https://reviewboard.mozilla.org/r/175826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175826/

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8904079 [details]
Bug 1396361 - Avoid crashing when some system library calls malloc_zone_free(zone, NULL).

https://reviewboard.mozilla.org/r/175826/#review180810

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/488a3f27fb35
Avoid crashing when some system library calls malloc_zone_free(zone, NULL). r=njn

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8904079 [details]
Bug 1396361 - Avoid crashing when some system library calls malloc_zone_free(zone, NULL).

Approval Request Comment
[Feature/Bug causing the regression]: bug 1356701
[User impact if declined]: random crashes due to some system libraries doing unexpected things
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: n/a
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a trivial change that essentially skips the (voluntarily) crashing code in a safe case (freeing a null pointer, so there's nothing to free in the first place)
[String changes made/needed]: n/a

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/488a3f27fb35

Comment 7

[Gerry Chang [:gchang]]

Comment on attachment 8904079 [details]
Bug 1396361 - Avoid crashing when some system library calls malloc_zone_free(zone, NULL).

Fix a crash. Beta56+.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e0216397b51a