chromiumembedded / cef

Chromium Embedded Framework (CEF). A simple framework for embedding Chromium-based browsers in other applications.
https://bitbucket.org/chromiumembedded/cef/
Other
3.28k stars 456 forks source link

Mac: Build with use_allocator_shim=false #3061

Open magreenblatt opened 3 years ago

magreenblatt commented 3 years ago

Original report by Patrick Heyer (Bitbucket: PatTheMav, GitHub: PatTheMav).


Affected OS: macOS Big Sur (11.0.2)
Affected versions: 3770, 4183, 4280

In OBS Studio we faced the issue with OBS crashing for users on macOS Big Sur with browser sources (which use CEF) added to their currently active scene and audio monitoring enabled.

We were able to trace the crash to our code creating an AudioQueue before CefInitialize is called and using debug symbols found that the crash occurred inside malloc_zone_functions_mac.cc (which from what I understand patches CFAllocator to protect against OOM situations - see also here: https://bugs.chromium.org/p/chromium/issues/detail?id=45650).

Next we added the following code to cefsimple_mac.mm right at the top of the main function and before CefScopedLibraryLoader library_loader:

const char *uid = "AppleHDAEngineOutput:1F,3,0,1,2:0";
CFStringRef cf_uid = CFStringCreateWithBytes(
  NULL, (const UInt8 *)uid, strlen(uid),
  kCFStringEncodingUTF8, false);
AudioQueueRef queue;
AudioStreamBasicDescription desc = {
  .mSampleRate = (Float64)48000,
  .mFormatID = kAudioFormatLinearPCM,
  .mFormatFlags = kAudioFormatFlagIsFloat |
      kAudioFormatFlagIsPacked,
  .mBytesPerPacket = sizeof(float) * 2,
  .mFramesPerPacket = 1,
  .mBytesPerFrame = sizeof(float) * 2,
  .mChannelsPerFrame = 2,
  .mBitsPerChannel = sizeof(float) * 8};
AudioQueueNewOutput(&desc, audio_noop, NULL, NULL, NULL, 0, &queue);
AudioQueueSetProperty(queue, kAudioQueueProperty_CurrentDevice, &cf_uid, sizeof(cf_uid));
CFRelease(cf_uid);
AudioQueueStart(queue, NULL);

// audio_noop is a const method anywhere in cefsimple_mac.mm

static void audio_noop(void *data, AudioQueueRef aq, AudioQueueBufferRef buf)
{
  return;
}

This code mimics the part of OBS which we isolated to be necessary to run for CEF to crash and crashes cefsimple as well.

I’ve attached the crashlog of cefsimple as well as a trace from OBS with debug symbols (pointing towards malloc_zone_functions_mac).

magreenblatt commented 3 years ago

This sounds like a Chromium issue, or possibly a Big Sur issue. You can try an official CEF build with use_allocator_shim=false added to GN_DEFINES in case that helps. See here for background.

magreenblatt commented 3 years ago

You might also try locally reverting this commit from the Chromium issue that you linked.

magreenblatt commented 3 years ago

Original comment by Florin Florin (Bitbucket: florin0x01, GitHub: florin0x01).


According to docs, Mac/iOS use_allocator: none, we always use the system's allocator implementation.

So use_allocator_shim =false is default on Mac?

magreenblatt commented 3 years ago

So use_allocator_shim =false is default on Mac?

That’s not my reading of the docs or code.

magreenblatt commented 3 years ago

Original comment by Patrick Heyer (Bitbucket: PatTheMav, GitHub: PatTheMav).


Thanks for the tip, we’ll re-build CEF without the shim and check if it fixes our issue. OTOH I could imagine that using different kinds of macOS Framework code that in one form or another makes use of the CFAllocator could introduce a situation where Chromium cannot install its shim.

Such a situation might only occur in embedding scenarios, but I’ll report it on their issue tracker to see if they think it’s something they’d like to fix on their end or if it’s an important implementation detail for CEF only.

Here’s the issue report: https://bugs.chromium.org/p/chromium/issues/detail?id=1157462

magreenblatt commented 3 years ago

Original comment by Czarek Tomczak (Bitbucket: Czarek, GitHub: Czarek).


I would be in favor for removing shim allocator in default CEF builds. We already removed malloc allocator in Linux builds in the past.

magreenblatt commented 3 years ago

Original comment by Avi Drissman (Bitbucket: Avi Drissman).


I commented on the crbug but wanted to drop a line here. This isn’t actually a CFAllocator issue; if you look at the crash you see that it’s a malloc shim issue instead.

The crash is weird; it’s crashing in base::allocator::StoreZoneFunctions()::$_0::operator()()but that’s weird; does CEF modify malloc_zone_functions_mac.cc? If not, what could $_0 be?

Please follow up on the crbug re the CHECK statement that is the only thing I can think of that might be that.

Also, is there any way to repro this without CEF?

magreenblatt commented 3 years ago

Original comment by Avi Drissman (Bitbucket: Avi Drissman).


Ooh. Also, StoreZoneFunctions is calling CHECK. Are your logging streams set up?

magreenblatt commented 3 years ago

Original comment by Patrick Heyer (Bitbucket: PatTheMav, GitHub: PatTheMav).


@{557058:2f2a2aee-b500-4023-9734-037e9897c3ab} We can confirm that setting `use_allocator_shim=false` indeed fixed the issue (which is to be expected).

As for the suggestion to remove the “CHECK on line 27” I’d have to delegate this question to other CEF maintainers or members on my team as I only implement CEF, I don’t have the capabilities to build it myself.

magreenblatt commented 3 years ago

Original comment by Avi Drissman (Bitbucket: Avi Drissman).


To be clear, it’s not that I’m suggesting the removal of the CHECK, but rather asking if the problem goes away if we comment it out, to see if that’s the cause.

magreenblatt commented 3 years ago

Original comment by Patrick Heyer (Bitbucket: PatTheMav, GitHub: PatTheMav).


Yeah no worries, that’s how I understood it. We look at re-building CEF with that to check soon-ish (as for now disabling the shim fixes our crash). We’ve had other volunteers dig into this and as expected there is all kinds of code that influences the state of malloc zones before CEF can “do its thing”.

magreenblatt commented 3 years ago

See also issue #3095 for Linux.

magreenblatt commented 2 years ago

This appears to require the combination of use_allocator_shim=false use_allocator=none

Note some potential loss of functionality when using this configuration, related to https://bugs.chromium.org/p/chromium/issues/detail?id=1121427

magreenblatt commented 2 years ago

As an alternative to disabling the shim, the client application would need to always initialize it before performing other work as suggested in Chromium issue #1157462. To facilitate that CEF would need to expose (or call) the partition_alloc::EarlyMallocZoneRegistration() function as shown for content_shell here.

magreenblatt commented 2 years ago

Mac: Use system allocator instead of PartitionAlloc (fixes issue #3061)

This change disables PartitionAlloc and the related allocator shim. Using the system allocator makes it easier to integrate with client applications which may perform allocations before initializing CEF.

→ <<cset 2f5e1b621e1f (bb)>>

magreenblatt commented 2 years ago

The motivation for making this change in M98 specifically is to avoid new “Trying to load the allocator multiple times. This is *not* supported.“ console errors due to Chromium changes here.

magreenblatt commented 3 years ago

Original changes by Patrick Heyer (Bitbucket: PatTheMav, GitHub: PatTheMav).


magreenblatt commented 3 years ago

Original changes by Patrick Heyer (Bitbucket: PatTheMav, GitHub: PatTheMav).


magreenblatt commented 3 years ago
magreenblatt commented 3 years ago
magreenblatt commented 2 years ago
magreenblatt commented 5 months ago

This change is temporarily reverted in M123 and M124 as a workaround for https://issues.chromium.org/issues/326898585

magreenblatt commented 4 months ago

The allocator shim will remain enabled by default in non-Official builds so that we can benefit from BackupRefPtr checks. See #3239.