Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make mapboxMap property optional in map view accessor #3377

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

krystofcelba
Copy link
Contributor

Description

Hi, I have recently encoutered this crash caused by some race condition when the JS code wants to access the zoom but the mapboxMap is not initialized yet. So I have made this fix. I simply changed the mapboxMap getter of RNMBXMapView to optional.

Checklist

  • I've read CONTRIBUTING.md
  • I updated the doc/other generated code with running yarn generate in the root folder
  • I have tested the new feature on /example app.
    • In V11 mode/ios
    • In New Architecture mode/ios
    • In V11 mode/android
    • In New Architecture mode/android
  • I added/updated a sample - if a new feature was implemented (/example)

@mfazekas
Copy link
Contributor

mfazekas commented Feb 14, 2024

@krystofcelba thanks much for looking into. My main concern is that with those fixes, we'll just skip some operations.
From one side this is better than crashing, from other side it's worse as debugging a camera change that doesn't happens if much more harder to debug than a crash.
We should at least ad error logs at places we don't expect MapboxMap to be nil.

Can you share a stack trace or anything regarding the original issue?

@Gp2mv3
Copy link
Contributor

Gp2mv3 commented May 3, 2024

I've the exact same issue on iOS with latest update.

@mfazekas
Copy link
Contributor

mfazekas commented May 4, 2024

@Gp2mv3 can you reproduce the issue? Can you share a stack trace?

@Gp2mv3
Copy link
Contributor

Gp2mv3 commented May 6, 2024

Yes I did, here is the stacktrace (before applying this PR):

* thread #1, queue = 'com.apple.main-thread', stop reason = Fatal error: Unexpectedly found nil while implicitly unwrapping an Optional value
  frame #0: 0x0000000185084a2c libswiftCore.dylib`_swift_runtime_on_report
  frame #1: 0x000000018512da94 libswiftCore.dylib`_swift_stdlib_reportFatalErrorInFile + 208
  frame #2: 0x0000000184d03fa4 libswiftCore.dylib`closure #1 (Swift.UnsafeBufferPointer<Swift.UInt8>) -> () in closure #1 (Swift.UnsafeBufferPointer<Swift.UInt8>) -> () in closure #1 (Swift.UnsafeBufferPointer<Swift.UInt8>) -> () in Swift._assertionFailure(_: Swift.StaticString, _: Swift.StaticString, file: Swift.StaticString, line: Swift.UInt, flags: Swift.UInt32) -> Swift.Never + 292
  frame #3: 0x0000000184d03e3c libswiftCore.dylib`closure #1 (Swift.UnsafeBufferPointer<Swift.UInt8>) -> () in closure #1 (Swift.UnsafeBufferPointer<Swift.UInt8>) -> () in Swift._assertionFailure(_: Swift.StaticString, _: Swift.StaticString, file: Swift.StaticString, line: Swift.UInt, flags: Swift.UInt32) -> Swift.Never + 332
  frame #4: 0x0000000184d037c8 libswiftCore.dylib`Swift._assertionFailure(_: Swift.StaticString, _: Swift.StaticString, file: Swift.StaticString, line: Swift.UInt, flags: Swift.UInt32) -> Swift.Never + 184
 * frame #5: 0x0000000103f95d30 Seety`RNMBXMapView.mapboxMap.getter(self=0x0000000127463be0) at RNMBXMapView.swift:210:11
  frame #6: 0x0000000103fc7cb8 Seety`static RNMBXMapViewManager.withMapboxMap(view=0x0000000127463be0, name="getZoom", rejecter=0x0000000103fcdcec Seety`reabstraction thunk helper from @escaping @callee_unowned @convention(block) (@unowned Swift.Optional<__C.NSString>, @unowned Swift.Optional<__C.NSString>, @unowned Swift.Optional<__C.NSError>) -> () to @escaping @callee_guaranteed (@guaranteed Swift.Optional<Swift.String>, @guaranteed Swift.Optional<Swift.String>, @guaranteed Swift.Optional<Swift.Error>) -> ()partial apply forwarder with unmangled suffix ".57" at <compiler-generated>, fn=0x0000000103fc99b8 Seety`partial apply forwarder for closure #1 (MapboxMaps.MapboxMap) -> () in static rnmapbox_maps.RNMBXMapViewManager.getZoom(_: rnmapbox_maps.RNMBXMapView, resolver: (Swift.Optional<Any>) -> (), rejecter: (Swift.Optional<Swift.String>, Swift.Optional<Swift.String>, Swift.Optional<Swift.Error>) -> ()) -> () at <compiler-generated>, self=@thick rnmapbox_maps.RNMBXMapViewManager.Type) at RNMBXMapViewManager.swift:35:36
  frame #7: 0x0000000103fc9834 Seety`static RNMBXMapViewManager.getZoom(view=0x0000000127463be0, resolver=0x0000000103fcdca8 Seety`reabstraction thunk helper from @escaping @callee_unowned @convention(block) (@unowned Swift.Optional<Swift.AnyObject>) -> () to @escaping @callee_guaranteed (@in_guaranteed Swift.Optional<Any>) -> ()partial apply forwarder with unmangled suffix ".53" at <compiler-generated>, rejecter=0x0000000103fcdcec Seety`reabstraction thunk helper from @escaping @callee_unowned @convention(block) (@unowned Swift.Optional<__C.NSString>, @unowned Swift.Optional<__C.NSString>, @unowned Swift.Optional<__C.NSError>) -> () to @escaping @callee_guaranteed (@guaranteed Swift.Optional<Swift.String>, @guaranteed Swift.Optional<Swift.String>, @guaranteed Swift.Optional<Swift.Error>) -> ()partial apply forwarder with unmangled suffix ".57" at <compiler-generated>, self=@thick rnmapbox_maps.RNMBXMapViewManager.Type) at RNMBXMapViewManager.swift:127:13
  frame #8: 0x0000000103fc9a98 Seety`@objc static RNMBXMapViewManager.getZoom(_:resolver:rejecter:) at <compiler-generated>:0
  frame #9: 0x0000000103efc9bc Seety`__45-[RNMBXMapViewModule getZoom:resolve:reject:]_block_invoke(.block_descriptor=0x0000000282145350, view=0x0000000127463be0) at RNMBXMapViewModule.mm:97:9
  frame #10: 0x0000000103efb778 Seety`__58-[RNMBXMapViewModule withMapView:block:reject:methodName:]_block_invoke(.block_descriptor=0x00000002835ecec0, uiManager=0x00000002804fb100, viewRegistry=0x0000000282f9a220) at RNMBXMapViewModule.mm:42:12
  frame #11: 0x000000010364e6ac Seety`__44-[RCTUIManager flushUIBlocksWithCompletion:]_block_invoke(.block_descriptor=0x00000002821b4720) at RCTUIManager.m:1162:9
  frame #12: 0x000000010364e97c Seety`__44-[RCTUIManager flushUIBlocksWithCompletion:]_block_invoke.194(.block_descriptor=0x0000000283aaf240) at RCTUIManager.m:1182:5
  frame #13: 0x0000000103656408 Seety`__RCTExecuteOnMainQueue_block_invoke(.block_descriptor=0x00000002821b7c00) at RCTUtils.m:279:7
  frame #14: 0x000000010739c520 libdispatch.dylib`_dispatch_call_block_and_release + 32
  frame #15: 0x000000010739e038 libdispatch.dylib`_dispatch_client_callout + 20
  frame #16: 0x00000001073ae798 libdispatch.dylib`_dispatch_main_queue_drain + 1196
  frame #17: 0x00000001073ae2dc libdispatch.dylib`_dispatch_main_queue_callback_4CF + 44
  frame #18: 0x000000018b033c28 CoreFoundation`__CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 16
  frame #19: 0x000000018b015560 CoreFoundation`__CFRunLoopRun + 1992
  frame #20: 0x000000018b01a3ec CoreFoundation`CFRunLoopRunSpecific + 612
  frame #21: 0x00000001c653035c GraphicsServices`GSEventRunModal + 164
  frame #22: 0x000000018d3a6f58 UIKitCore`-[UIApplication _run] + 888
  frame #23: 0x000000018d3a6bbc UIKitCore`UIApplicationMain + 340
  frame #24: 0x0000000102e6888c Seety`main(argc=2, argv=0x000000016cf9f7b8) at main.m:18:12
  frame #25: 0x00000001aa54cdec dyld`start + 2220

It's indeed in the accessor of mapboxMap for the getZoom() that the crashes occurs.

Hope it helps !

@mfazekas
Copy link
Contributor

mfazekas commented May 7, 2024

@Gp2mv3 thanks much awesome, this should be addressed in #3477
The difference between #3477 and this PR is that the former will execute the getZoom, getCenter, etc callbacks as soon as the mapView initialisation finished. While this PR just will not do some stuff until mapView has initialised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants