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

Fixed crash when style is not fully loaded #265

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

Conversation

michalgwo
Copy link
Contributor

Fixes #252 on Android. This crash doesn't seem to happen on the JS target. I don't know if the problem also occurs on iOS, I was looking at the documentation and couldn't find isFullyLoaded equivalent, so it may be an Android-specific crash, but it would be worth to test it also on iOS.

I'm not so familiar with the codebase here, so I'm unsure if skipping the execution of functions or returning a null/empty list is a good solution, so I would appreciate feedback on this.

Based on my investigation of the code, it looks like all the internal executions of the style's functions happen after the style is already loaded, so it shouldn't skip any execution that should happen on a loaded style.

The crash happens on changing the style trying to execute functions of the dismissed style, so it makes sense to skip it, as it will be executed again after a new style is fully loaded.

The cases I can think of when this crash could happen:

  • after the previous style is dismissed, but before a new style is loaded
  • after a new style started loading, but the initialization code from the previous style didn't finish executing yet

@sargunv
Copy link
Owner

sargunv commented Feb 1, 2025

thanks for looking into this! with this fix, instead of crashing, we just won't add the images, layers, etc.

Do you know if the style being operated on at the time of the crash is the outgoing style or the incoming style? If it's outgoing this is fine, but if it's incoming this would be a bug.

It might be better to make a change elsewhere in the stack to ensure we don't make style changes on outgoing styles (there's already an attempt at this by setting a no-op style implementation, but I guess it's missing something)

@michalgwo
Copy link
Contributor Author

I took a different approach now, it seems to be a better solution. Previously, the crash still happened in some cases, but now I can't reproduce it anymore. It's implemented in common code, so it will affect all platforms, not only Android, and now we can be sure that isLoaded is false only on the outgoing style. The drawback of this approach is that we need to check if style isLoaded before every call of style's functions, otherwise, it can throw IllegalStateException when changing styles.

@sargunv
Copy link
Owner

sargunv commented Feb 2, 2025

Could you see if the bug can be replicated with the changes in #269? It's an approach somewhere in the middle of your two approaches here.

It's working fine on my machine, but I haven't been able to replicate the crash at all so I'm working blind here 😅

@sargunv sargunv added this to the v0.7.0 milestone Feb 2, 2025
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.

Crash when changing styles quickly on Android
2 participants