fix: geolocation crashes electron on macOS #29913

omarkilani · 2021-06-28T02:27:13Z

Description of Change

Geolocation was crashing in Release builds with a backtrace that seemed to implicate WASM SIMD.

$ lldb --debug ./Electron.app/Contents/MacOS/Electron 
(lldb) target create "./Electron.app/Contents/MacOS/Electron"
Current executable set to '/Users/omar/Downloads/electron-v13/Electron.app/Contents/MacOS/Electron' (x86_64).
(lldb) r https://www.google.com/
Process 67072 launched: '/Users/omar/Downloads/electron-v13/Electron.app/Contents/MacOS/Electron' (x86_64)
2021-06-27 10:10:30.780668-0700 Electron[67072:11102508] +[MTLIOAccelDevice registerDevices]: Zero Metal services found
Process 67072 stopped
* thread #39, name = 'Geolocation', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
    frame #0: 0x0000000106a8e3e4 Electron Framework`___lldb_unnamed_symbol4$$Electron Framework + 53326868
Electron Framework`v8::internal::compiler::GetI32WasmCallDescriptorForSimd:
->  0x106a8e3e4 <+40987940>: movq   0x8(%rdi), %rax
    0x106a8e3e8 <+40987944>: testq  %rax, %rax
    0x106a8e3eb <+40987947>: je     0x106a8e3f1               ; <+40987953>
    0x106a8e3ed <+40987949>: lock   
Target 0: (Electron) stopped.
(lldb) bt
* thread #39, name = 'Geolocation', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
  * frame #0: 0x0000000106a8e3e4 Electron Framework`___lldb_unnamed_symbol4$$Electron Framework + 53326868
    frame #1: 0x00000001037af356 Electron Framework`node::SetTracingController(v8::TracingController*) + 1008182
    frame #2: 0x000000010480ce19 Electron Framework`v8::internal::compiler::GetI32WasmCallDescriptorForSimd(v8::internal::Zone*, v8::internal::compiler::CallDescriptor*) + 4806489
    frame #3: 0x000000010480a952 Electron Framework`v8::internal::compiler::GetI32WasmCallDescriptorForSimd(v8::internal::Zone*, v8::internal::compiler::CallDescriptor*) + 4797074
    frame #4: 0x000000010185ee95 Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 7115717
    frame #5: 0x000000010480a636 Electron Framework`v8::internal::compiler::GetI32WasmCallDescriptorForSimd(v8::internal::Zone*, v8::internal::compiler::CallDescriptor*) + 4796278
    frame #6: 0x0000000101aed0fc Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 9795116
    frame #7: 0x0000000101b053d6 Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 9894150
    frame #8: 0x0000000101b3d3e0 Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 10123536
    frame #9: 0x0000000101b399c2 Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 10108658
    frame #10: 0x0000000101b3cbff Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 10121519
    frame #11: 0x00007fff204626dc CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #12: 0x00007fff20462644 CoreFoundation`__CFRunLoopDoSource0 + 180
    frame #13: 0x00007fff204623ba CoreFoundation`__CFRunLoopDoSources0 + 242
    frame #14: 0x00007fff20460dc8 CoreFoundation`__CFRunLoopRun + 897
    frame #15: 0x00007fff20460380 CoreFoundation`CFRunLoopRunSpecific + 567
    frame #16: 0x00007fff21118647 Foundation`-[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 212
    frame #17: 0x0000000101b3d9d9 Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 10125065
    frame #18: 0x0000000101b3c6db Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 10120203
    frame #19: 0x0000000101b067af Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 9899231
    frame #20: 0x0000000101ad8e0c Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 9712444
    frame #21: 0x0000000101b231c8 Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 10016504
    frame #22: 0x0000000101b233c7 Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 10017015
    frame #23: 0x0000000101b380d8 Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 10102280
    frame #24: 0x00007fff203698fc libsystem_pthread.dylib`_pthread_start + 224
    frame #25: 0x00007fff20365443 libsystem_pthread.dylib`thread_start + 15
(lldb)

And because Chrome 91 enabled WASM SIMD by default and the crash occurred between 13.0.0-beta13 and 13.0.0-beta14 (66a2218#diff-42a9b04f836798e53829385d847f016f600b92336c532d9c7f7a18acc4937d26L118), it seemed to be the culprit and sort of lead me down the wrong path.

However, when dSYMs were available and loaded into lldb, it was able to produce a real backtrace:

$ lldb --debug ./Electron.app/Contents/MacOS/Electron 
(lldb) target create "./Electron.app/Contents/MacOS/Electron"
Current executable set to '/Users/omar/Downloads/electron-v13/Electron.app/Contents/MacOS/Electron' (x86_64).
(lldb) add-dsym Electron.dSYM
symbol file '/Users/omar/Downloads/electron-v13/Electron.dSYM/Contents/Resources/DWARF/Electron' has been added to '/Users/omar/Downloads/electron-v13/Electron.app/Contents/MacOS/Electron'
(lldb) add-dsym Electron\ Framework.dSYM
symbol file '/Users/omar/Downloads/electron-v13/Electron Framework.dSYM/Contents/Resources/DWARF/Electron Framework' has been added to '/Users/omar/Downloads/electron-v13/Electron.app/Contents/Frameworks/Electron Framework.framework/Electron Framework'
(lldb) r https://www.google.com/
Process 69270 launched: '/Users/omar/Downloads/electron-v13/Electron.app/Contents/MacOS/Electron' (x86_64)
2021-06-27 10:12:53.149980-0700 Electron[69270:11119528] +[MTLIOAccelDevice registerDevices]: Zero Metal services found
Electron Framework was compiled with optimization - stepping may behave oddly; variables may not be available.
Process 69270 stopped
* thread #39, name = 'Geolocation', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
    frame #0: 0x0000000106a8e3e4 Electron Framework`device::GeolocationSystemPermissionManager::GetObserverList() [inlined] scoped_refptr<base::ObserverListThreadSafe<device::GeolocationSystemPermissionManager::GeolocationPermissionObserver> >::scoped_refptr(r=0x0000000000000008) at scoped_refptr.h:197:59 [opt]
Target 0: (Electron) stopped.
(lldb) bt
* thread #39, name = 'Geolocation', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
  * frame #0: 0x0000000106a8e3e4 Electron Framework`device::GeolocationSystemPermissionManager::GetObserverList() [inlined] scoped_refptr<base::ObserverListThreadSafe<device::GeolocationSystemPermissionManager::GeolocationPermissionObserver> >::scoped_refptr(r=0x0000000000000008) at scoped_refptr.h:197:59 [opt]
    frame #1: 0x0000000106a8e3e4 Electron Framework`device::GeolocationSystemPermissionManager::GetObserverList(this=0x0000000000000000) at geolocation_system_permission_mac.mm:97 [opt]
    frame #2: 0x000000010480ce19 Electron Framework`device::NetworkLocationProvider::NetworkLocationProvider(this=0x0000000109977ce0, url_loader_factory=<unavailable>, geolocation_system_permission_manager=0x0000000000000000, main_task_runner=const scoped_refptr<base::SingleThreadTaskRunner> @ 0x0000700016c35960, api_key=<unavailable>, position_cache=<unavailable>) at network_location_provider.cc:64:46 [opt]
    frame #3: 0x000000010480a952 Electron Framework`device::LocationArbitrator::NewNetworkLocationProvider(scoped_refptr<network::SharedURLLoaderFactory>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) [inlined] std::__1::__unique_if<device::NetworkLocationProvider>::__unique_single std::__1::make_unique<device::NetworkLocationProvider, scoped_refptr<network::SharedURLLoaderFactory>, device::GeolocationSystemPermissionManager*&, scoped_refptr<base::SingleThreadTaskRunner>&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, device::PositionCache*>(__args=0x0000000109980ca0, __args=0x0000000109980ca8) at memory:2006:32 [opt]
    frame #4: 0x000000010480a922 Electron Framework`device::LocationArbitrator::NewNetworkLocationProvider(this=0x0000000109980c90, url_loader_factory=scoped_refptr<network::SharedURLLoaderFactory> @ scalar, api_key="") at location_arbitrator.cc:157 [opt]
    frame #5: 0x000000010185ee95 Electron Framework`device::LocationArbitrator::RegisterProviders(this=0x0000000109980c90) at location_arbitrator.cc:122:22 [opt]
    frame #6: 0x000000010480a636 Electron Framework`device::LocationArbitrator::StartProvider(this=0x0000000109980c90, enable_high_accuracy=<unavailable>) at location_arbitrator.cc:65:5 [opt]
    frame #7: 0x0000000101aed0fc Electron Framework`base::TaskAnnotator::RunTask(char const*, base::PendingTask*) [inlined] base::OnceCallback<void ()>::Run() && at callback.h:101:12 [opt]
    frame #8: 0x0000000101aed0da Electron Framework`base::TaskAnnotator::RunTask(this=<unavailable>, trace_event_name=<unavailable>, pending_task=0x000000011109a000) at task_annotator.cc:173 [opt]
    frame #9: 0x0000000101b053d6 Electron Framework`non-virtual thunk to base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork() at thread_controller_with_message_pump_impl.cc:351:25 [opt]
    frame #10: 0x0000000101b04f8b Electron Framework`non-virtual thunk to base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork() [inlined] base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork(this=0x0000000109eb4980) at thread_controller_with_message_pump_impl.cc:264 [opt]
    frame #11: 0x0000000101b04ee6 Electron Framework`non-virtual thunk to base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork() at thread_controller_with_message_pump_impl.cc:0 [opt]
    frame #12: 0x0000000101b3d3e0 Electron Framework`invocation function for block in base::MessagePumpCFRunLoopBase::RunWorkSource(void*) [inlined] base::MessagePumpCFRunLoopBase::RunWork(this=0x0000000109971e20) at message_pump_mac.mm:384:54 [opt]
    frame #13: 0x0000000101b3d3b2 Electron Framework`invocation function for block in base::MessagePumpCFRunLoopBase::RunWorkSource(.block_descriptor=<unavailable>) at message_pump_mac.mm:361 [opt]
    frame #14: 0x0000000101b399c2 Electron Framework`v8::internal::SetupIsolateDelegate::SetupHeap(v8::internal::Heap*) + 10108658
    frame #15: 0x0000000101b3cbff Electron Framework`base::MessagePumpCFRunLoopBase::RunWorkSource(info=<unavailable>) at message_pump_mac.mm:360:3 [opt]
    frame #16: 0x00007fff204626dc CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #17: 0x00007fff20462644 CoreFoundation`__CFRunLoopDoSource0 + 180
    frame #18: 0x00007fff204623ba CoreFoundation`__CFRunLoopDoSources0 + 242
    frame #19: 0x00007fff20460dc8 CoreFoundation`__CFRunLoopRun + 897
    frame #20: 0x00007fff20460380 CoreFoundation`CFRunLoopRunSpecific + 567
    frame #21: 0x00007fff21118647 Foundation`-[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 212
    frame #22: 0x0000000101b3d9d9 Electron Framework`base::MessagePumpNSRunLoop::DoRun(this=0x0000000109971e20, delegate=<unavailable>) at message_pump_mac.mm:630:5 [opt]
    frame #23: 0x0000000101b3c6db Electron Framework`base::MessagePumpCFRunLoopBase::Run(this=0x0000000109971e20, delegate=0x0000000109eb4980) at message_pump_mac.mm:157:3 [opt]
    frame #24: 0x0000000101b067af Electron Framework`base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run(this=0x0000000109eb4980, application_tasks_allowed=true, timeout=<unavailable>) at thread_controller_with_message_pump_impl.cc:460:12 [opt]
    frame #25: 0x0000000101ad8e0c Electron Framework`base::RunLoop::Run(this=0x0000700016c36f30, location=<unavailable>) at run_loop.cc:133:14 [opt]
    frame #26: 0x0000000101b231c8 Electron Framework`base::Thread::Run(this=<unavailable>, run_loop=<unavailable>) at thread.cc:312:13 [opt]
    frame #27: 0x0000000101b233c7 Electron Framework`base::Thread::ThreadMain(this=0x0000000109ead3c0) at thread.cc:383:3 [opt]
    frame #28: 0x0000000101b380d8 Electron Framework`base::(anonymous namespace)::ThreadFunc(params=<unavailable>) at platform_thread_posix.cc:87:13 [opt]
    frame #29: 0x00007fff203698fc libsystem_pthread.dylib`_pthread_start + 224
    frame #30: 0x00007fff20365443 libsystem_pthread.dylib`thread_start + 15
(lldb)

One of the participants in #29343, @TheCleric, happened to stumble upon a Chrome commit that seemed related.

Chrome had merged a change to NetworkLocationProvider:

https://chromium.googlesource.com/chromium/src/+/36d366175fee2d4f0fd0a8ccf53338984da9b531%5E%21/

That had caused a crash in headless Chrome:

https://bugs.chromium.org/p/chromium/issues/detail?id=1195664&q=OS%3DMac%20component%3ABlink%3EGeolocation&can=1

For which a fix was committed:

https://chromium.googlesource.com/chromium/src/+/39cabc596fccd3e79b71cd8ddd0f3348cc2975d9%5E%21/

The reason it affected Electron past 13.0.0-beta13 was because 13.0.0-beta14 bumped Chromium to 91.0.4448.0 , while the changed was landed in Chromium 91.0.4437.0:

https://chromiumdash.appspot.com/commit/36d366175fee2d4f0fd0a8ccf53338984da9b531

This PR / commit implements an Electron specific version of this fix, modified for changes to GeolocationManager in Chrome 93.

I've tested the fix for cases where no system permissions have been granted, where system permissions have been granted with no Google API key, and where system permissions have been granted with a Google API key. Seems to work like it used to.

Closes #29343

Checklist

PR description included and stakeholders cc'd
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed crashes on macOS when Geolocation was used.

trop · 2021-06-28T02:28:36Z

@omarkilani has manually backported this PR to "14-x-y", please check out #29914

trop · 2021-06-28T02:29:26Z

@omarkilani has manually backported this PR to "13-x-y", please check out #29912

deepak1556 · 2021-06-28T04:27:21Z

Thanks for the PR, couple of changes:

Can you move the creation of device::GeolocationManager to Electron::BrowserMainParts::PreMainMessageLoopRun. Based on https://chromium-review.googlesource.com/c/chromium/src/+/2728410 it looks the lifetime management in the browser process is not tied with the utilization in the device service but the comment in https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/content_browser_client.h;l=829-833 speaks differently. Also the change will match what chrome and headless are doing today.
Since the default backend for gelocation service on mac relies on the system location service, it looks like codesigned application will need com.apple.security.personal-information.location entitlement for it to work correctly. This behavior change should be documented.

omarkilani · 2021-06-28T04:41:40Z

@deepak1556 yup, was making that main parts change already.

Just wasn’t sure if the goal was to minimize the number of files touched or what the project rules were for calling out to main parts from browser client since nothing was doing that already.

It takes me like 4-5 hours to build on one branch so… I’ll try to get that done soon.

I think 2 only applies to the main and 14-x-y branches. Is that your view?

My focus was on the 13-x-y branch and that’s sort of what I tested comprehensively with. I did need to codesign with my own Developer ID for it to work, but didn’t need to add that entitlement.

Do you want the main parts bit to match headless on all the branches?

deepak1556 · 2021-06-28T05:30:38Z

Do you want the main parts bit to match headless on all the branches?

Yes please match the change on all the branches.

It takes me like 4-5 hours to build on one branch so… I’ll try to get that done soon.

Did you setup electron with https://github.com/electron/build-tools ? It should provide read only compile cache that should speed up builds.

I think 2 only applies to the main and 14-x-y branches. Is that your view?

The backend was enabled by default first in chromium 87 https://chromium-review.googlesource.com/c/chromium/src/+/2411302 , so 13-x-y and 12-x-y should also be affected.

I did need to codesign with my own Developer ID for it to work, but didn’t need to add that entitlement.

Just to confirm, did you codesign for hardened runtime ?

omarkilani · 2021-06-28T05:46:52Z

Do you want the main parts bit to match headless on all the branches?

Yes please match the change on all the branches.

Okay.

It takes me like 4-5 hours to build on one branch so… I’ll try to get that done soon.

Did you setup electron with https://github.com/electron/build-tools ? It should provide read only compile cache that should speed up builds.

Ah, that's handy. Thanks! I've never needed to compile Electron before so... there's a lot to learn. :)

Actually I ended up hacking ninja directly to hash the file contents and ignore the mtime because I accidentally ran gn instead of ninja after 5 hours of building and... it decided to rebuild the entire thing again.

I think 2 only applies to the main and 14-x-y branches. Is that your view?

The backend was enabled by default first in chromium 87 https://chromium-review.googlesource.com/c/chromium/src/+/2411302 , so 13-x-y and 12-x-y should also be affected.

Okay.

I did need to codesign with my own Developer ID for it to work, but didn’t need to add that entitlement.

Just to confirm, did you codesign for hardened runtime ?

Nope. We don't publish on MAS so it's not something I regularly test.

I tested my build of what's over here: https://app.circleci.com/pipelines/github/electron/electron/41700/workflows/bbe3b3bc-c4fa-45c3-9551-c369e899cbde/jobs/927113/artifacts

And used it with electron-builder to test our own app's Geolocation functionality with a Google API key. Seemed to work exactly like the 12.x releases.

I also tested it against all the various test cases in the linked issue.

omarkilani · 2021-06-28T08:59:13Z

Okay, I think I'm done on all 3 branches code-wise. It's as close to headless as possible.

deepak1556

LGTM with minor change.

shell/browser/electron_browser_client.cc

welcome · 2021-06-29T03:27:00Z

Congrats on merging your first pull request! 🎉🎉🎉

release-clerk · 2021-06-29T03:27:00Z

Release Notes Persisted

Fixed crashes on macOS when Geolocation was used.

…#29913)

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 28, 2021

omarkilani mentioned this pull request Jun 28, 2021

fix: geolocation crashes electron on macOS #29914

Merged

trop bot added the in-flight/14-x-y label Jun 28, 2021

omarkilani mentioned this pull request Jun 28, 2021

fix: geolocation crashes electron on macOS #29912

Merged

trop bot added the in-flight/13-x-y label Jun 28, 2021

codebytere changed the title ~~fix: geolocation crashes electron on macOS (#29343)~~ fix: geolocation crashes electron on macOS Jun 28, 2021

deepak1556 reviewed Jun 28, 2021

View reviewed changes

shell/browser/electron_browser_client.cc Outdated Show resolved Hide resolved

deepak1556 added the semver/patch backwards-compatible bug fixes label Jun 28, 2021

fix: geolocation crashes electron on macOS (#29343)

83c142d

deepak1556 approved these changes Jun 28, 2021

View reviewed changes

deepak1556 requested a review from MarshallOfSound June 28, 2021 14:55

MarshallOfSound approved these changes Jun 28, 2021

View reviewed changes

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 29, 2021

deepak1556 merged commit 1b4ce6c into electron:main Jun 29, 2021

trop bot added merged/13-x-y and removed in-flight/13-x-y labels Jun 29, 2021

omarkilani deleted the main branch June 30, 2021 02:39

BlackHole1 pushed a commit to BlackHole1/electron that referenced this pull request Aug 30, 2021

fix: geolocation crashes electron on macOS (electron#29343) (electron…

aa26f4e

…#29913)

schavaLogi mentioned this pull request Jan 25, 2022

[Bug]: Electron App Displayed under Location Service on Mac #32606

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: geolocation crashes electron on macOS #29913

fix: geolocation crashes electron on macOS #29913

omarkilani commented Jun 28, 2021 •

edited

trop bot commented Jun 28, 2021

trop bot commented Jun 28, 2021

deepak1556 commented Jun 28, 2021

omarkilani commented Jun 28, 2021 •

edited

deepak1556 commented Jun 28, 2021

omarkilani commented Jun 28, 2021 •

edited

omarkilani commented Jun 28, 2021

deepak1556 left a comment

welcome bot commented Jun 29, 2021

release-clerk bot commented Jun 29, 2021

fix: geolocation crashes electron on macOS #29913

fix: geolocation crashes electron on macOS #29913

Conversation

omarkilani commented Jun 28, 2021 • edited

Description of Change

Checklist

Release Notes

trop bot commented Jun 28, 2021

trop bot commented Jun 28, 2021

deepak1556 commented Jun 28, 2021

omarkilani commented Jun 28, 2021 • edited

deepak1556 commented Jun 28, 2021

omarkilani commented Jun 28, 2021 • edited

omarkilani commented Jun 28, 2021

deepak1556 left a comment

Choose a reason for hiding this comment

welcome bot commented Jun 29, 2021

release-clerk bot commented Jun 29, 2021

omarkilani commented Jun 28, 2021 •

edited

omarkilani commented Jun 28, 2021 •

edited

omarkilani commented Jun 28, 2021 •

edited