rive-app / rive-ios

iOS runtime for Rive
MIT License
496 stars 58 forks source link

Fix riveModel force-unwrap crash #246

Closed FelixHerrmann closed 1 year ago

FelixHerrmann commented 1 year ago

The RiveView class has a weak reference of the riveModel which is defined as implicitly unwrapped; the RiveViewModel stores both (the view and model) references strongly.

In certain circumstance the riveModel instance get's de-initialized before the riveView. This causes a crash in the RiveView.advance(delta:) method when the property get's called and is nil:

Screenshot 2023-02-20 at 18 30 58

I'm not 100% sure when this actually could happen, but I can constantly reproduce it by trying to present a view controller on a view controller that already presents one (you can see the log message in the screenshot above):

let riveViewController = RiveViewController()
present(riveViewController, animated: true)

DispatchQueue.main.asyncAfter(deadline: .now() + 2) {
    let riveViewController2 = RiveViewController()
    self.present(riveViewController2, animated: true)
}

Here is an example project: Rive Crash.zip

It is generally a bad practice to do implicit/force unwraps so I fixed that and handled the optionals in all places!

zplata commented 1 year ago

Thanks for contributing @FelixHerrmann ! We're going to take a look at this and test out on our end

FelixHerrmann commented 1 year ago

Hey @zplata, any updates? It's been a while.

zplata commented 1 year ago

@FelixHerrmann Sorry for the delay here! Getting a review on this internally, and assuming all goes well, it should merge into this repo in the next day or so here.

FelixHerrmann commented 1 year ago

No problem, appreciate the update!

zplata commented 1 year ago

@FelixHerrmann hey there! So a few things:

  1. Thanks again for your contribution here! This should be released in 3.1.8 if you want to try it out
  2. HUGE apologies here on our part, but our internal repo that pushes down to this downstream public runtime repo didn't appropriately attribute credit to you as the commit author as it did internally when this change landed here. We do some scripting here to push the commit from internal -> public, but we need to tweak it to keep the commit author contributor while maintaining our push model. We'll work on adding that for future contributions.
FelixHerrmann commented 1 year ago

Awesome, already saw it, thank you for the update!

To 2: No problem at all, the important thing is to have this fix available. How do we want to proceed with this PR? Should I just close it?

zplata commented 1 year ago

Yup, feel free to close this, thanks!

FelixHerrmann commented 1 year ago

Was contributed via internal repo