1419004 - Crash in @0x0 | mozilla::plugins::PluginUtilsOSX::SetProcessName

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Tyler Downer [He/Him]]

This bug was filed from the Socorro interface and is
report bp-e566e5c0-fd12-4cf1-b296-d8e960171120.

Reported by a SUMO user at https://support.mozilla.org/en-US/questions/1188509
=============================================================

Top 8 frames of crashing thread:

0  @0x0 
1 XUL mozilla::plugins::PluginUtilsOSX::SetProcessName dom/plugins/ipc/PluginUtilsOSX.mm:280
2 XUL mozilla::dom::ContentChild::SetProcessName dom/ipc/ContentChild.cpp:717
3 XUL mozilla::dom::ContentChild::Init dom/ipc/ContentChild.cpp:682
4 XUL mozilla::dom::ContentProcess::Init dom/ipc/ContentProcess.cpp:252
5 XUL XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:673
6 plugin-container main ipc/contentproc/plugin-container.cpp:63
7 plugin-container start 

=============================================================

Comment 1

[Marcia Knous [:marcia]]

Moving to a better component. Feel free to re-triage. 10.13 crash only so far. 45 crashes in 58b4.

No useful comments from what I can see.

Comment 2

[[:philipp]]

some comments refer to applying a macos system update just before those crashes started happening

Comment 3

[kennytm]

This affects the current nightly (59.0a1, build ID: 20171208220141) as well.

Comment 4

[kennytm]

The cause of the crash is that getting the `_LSGetCurrentApplicationASN` function pointer via `CFBundleGetFunctionPointerForName` will always return NULL on 10.13.2, and `PluginUtilsOSX::SetProcessName` never checked whether the return value is NULL before calling it.  

https://hg.mozilla.org/releases/mozilla-release/file/43107b7a72e4853be9db4ff69869993363474b6c/dom/plugins/ipc/PluginUtilsOSX.mm#l280


    CFBundleRef launchServices = ::CFBundleGetBundleWithIdentifier(
                                          CFSTR("com.apple.LaunchServices"));
    if (!launchServices) {
      NS_WARNING("Failed to set process name: Could not open LaunchServices bundle");
      return false;
    }

    if (!sApplicationASN) {
      sApplicationASN = ::CFBundleGetFunctionPointerForName(launchServices, 
                                            CFSTR("_LSGetCurrentApplicationASN"));
    }
    LSGetASNType getASNFunc = reinterpret_cast<LSGetASNType>
                                          (sApplicationASN);

    ...

    CFTypeRef currentAsn = getASNFunc();   // getASNFunc == NULL here





The NULL return issue is reproducible with a simple ObjC program. Interestingly `dlsym` (without passing the RTLD_NOLOAD flag) does return a valid function pointer, which suggests `CFBundleGetBundleWithIdentifier` somehow is unable to properly load the framework.


	// clang++ -fmodules -g 1.m -framework Foundation
	
	@import Foundation;
	#include <dlfcn.h>

	int main() {
		@autoreleasepool {
			CFBundleRef bundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.LaunchServices"));
			CFShow(bundle);
			
			void* ptr = CFBundleGetFunctionPointerForName(bundle, CFSTR("_LSGetCurrentApplicationASN"));
			printf("FPTR>> %p\n", ptr); // should be 0x0
			ptr = CFBundleGetDataPointerForName(bundle, CFSTR("_LSGetCurrentApplicationASN"));
			printf("DPTR>> %p\n", ptr); // should be 0x0

			void* dll = dlopen("/System/Library/Frameworks/CoreServices.framework/Versions/A/Frameworks/LaunchServices.framework/LaunchServices", 0);
			printf("DLL??? %p\n", dll); // should be nonzero
			void* x = dlsym(dll, "_LSGetCurrentApplicationASN");
			printf("DLSYM> %p\n", x);   // should be nonzero (!)

			CFShow(((CFTypeRef(*)())x)()); // should not crash.

			dlclose(dll);
		}
	}



Anyway, since there is proper NULL check when creating the `launchServices`, I'm able to manually patch `Firefox.app/Contents/MacOS/XUL` by replacing the string "com.apple.LaunchServices" by "xom.apple.LaunchServices" as a workaround.

Comment 5

[Marcia Knous [:marcia]]

(In reply to [:philipp] from comment #2)
> some comments refer to applying a macos system update just before those
> crashes started happening

Yes definitely almost all of the comments mention updating to 10.13.2 and having the crash happen. This crash is at the top of the Mac specific crash stats in 57.0.2.

Comment 6

[Stephen A Pohl [:spohl]]

Attachment: Patch

Let's fix the crash by adding a null check and figure out how to do this properly on 10.13.2 separately.

Comment 7

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e66e48637c6578350a43e157988b2a9787a3ab4a
Bug 1419004: Fix crash when setting plugin process names on macOS 10.13.2. r=mstange

Comment 8

[Stephen A Pohl [:spohl]]

(In reply to kennytm from comment #3)
> This affects the current nightly (59.0a1, build ID: 20171208220141) as well.

Could you confirm when these crashes occur? Based on the call stack I would expect one crash per content process that we launch. Is this the case?

Comment 9

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c237e10a08aa185f17cbed0d7eeedd9173a2cfe3
Backout e66e48637c65 (bug 1419004) for compile error. r=me

Comment 10

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f590085517b410e835dd082cdc89788bc9c57217
Bug 1419004: Fix crash when setting plugin process names on macOS 10.13.2. r=mstange

Comment 11

[Stephen A Pohl [:spohl]]

Attachment: Patch

Patch as landed. Carrying over r+.

Comment 12

[kennytm]

(In reply to Stephen A Pohl [:spohl] from comment #8)
> (In reply to kennytm from comment #3)
> > This affects the current nightly (59.0a1, build ID: 20171208220141) as well.
> 
> Could you confirm when these crashes occur? Based on the call stack I would
> expect one crash per content process that we launch. Is this the case?

There is one crash every time a new tab is opened, yes.

Comment 13

[Cosmin Sabou [:CosminS]]

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

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Is that something we would like to fix in 58?

Comment 15

[Stephen A Pohl [:spohl]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> Is that something we would like to fix in 58?

Yes, but we should confirm the fix in Nightly first.

kennytm, could you try the latest Nightly[1] and verify that the crash is fixed? Thanks!

[1] https://archive.mozilla.org/pub/firefox/nightly/2017/12/2017-12-14-10-03-15-mozilla-central/firefox-59.0a1.en-US.mac.dmg

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Hmm, I don't see any crash in 59 at all.

Comment 17

[Stephen A Pohl [:spohl]]

Interesting. kennytm, it would be great if you could verify that the crash occurs in a build before this patch landed[1] and confirm that it is now fixed in current nightly[2]. Thank you!

[1] https://archive.mozilla.org/pub/firefox/nightly/2017/12/2017-12-01-10-01-15-mozilla-central/firefox-59.0a1.en-US.mac.dmg
[2] https://archive.mozilla.org/pub/firefox/nightly/2017/12/2017-12-14-10-03-15-mozilla-central/firefox-59.0a1.en-US.mac.dmg

Comment 18

[kennytm]

Verified. 

The 12-01 build crashed with bp-c8e19b78-6795-4f7a-829c-6cf530171214 as soon as the browser opened, as expected.
The 12-14 build works normally.

Comment 20

[Stephen A Pohl [:spohl]]

Comment on attachment 8936538 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: macOS 10.13
[User impact if declined]: Some users on macOS 10.13 will encounter continuous crashes every time a content process is launched.
[Is this code covered by automated tests?]: no (this affects a subset of 10.13 installs)
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Simple null check that prevents a crash.
[String changes made/needed]: none

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8936538 [details] [diff] [review]
Patch

Fix a crash. Beta58+.

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8936538 [details] [diff] [review]
Patch

Fix a crash. Beta58+.

Comment 23

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

https://hg.mozilla.org/releases/mozilla-beta/rev/99e8736d4a20

Comment 25

[Ryan VanderMeulen [:RyanVM]]

We know there are ESR52 users on OSX 10.13 (universities and such). Is this something we should consider backporting there too? Hard to get a sense of how frequent the crash is there given the great Crash Data Purge of 2017 :). Looks like a safe-enough patch anyway.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

It doesn't match the esr criteria...