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

Re-work views to avoid incorrect disposal #1180

Merged
merged 6 commits into from
Mar 24, 2020
Merged

Re-work views to avoid incorrect disposal #1180

merged 6 commits into from
Mar 24, 2020

Conversation

mattleibow
Copy link
Contributor

@mattleibow mattleibow commented Mar 19, 2020

Description of Change

In some instances, the managed canvas is being collected at the same time the new one is being created. This causes the the app to crash for some reason. The technically should not happen as the canvas is still valid - and we can see this when we set the canvas as a class field.

But, for now, just work around the issue by adding the canvas as a field.

In addition, do more granular construction of the GL objects, and then also reduce the amount of interop in the render loop.

Bugs Fixed

API Changes

None.

Behavioral Changes

Some fixes for crashes.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Updated documentation

@mattleibow mattleibow merged commit 73bfe55 into master Mar 24, 2020
@mattleibow mattleibow deleted the dev/fix-1121 branch March 24, 2020 16:10
@mattleibow
Copy link
Contributor Author

Based on preliminary feedback, this looks to resolve the issue, so I am going to merge and publish the preview.

@hajmajeboss
Copy link

OK. Will be available on NuGet?

@mattleibow
Copy link
Contributor Author

Yes. Just waiting on CI right now. If all goes well, then in 2 hours. I will tweet and update the issues.

@hajmajeboss
Copy link

OK, looking forward to it. Hope it's all fixed and stable, I just made a move to new Mapsui version which is dependent on SkiaSharp 1.68.1+, so it really came in convenient time :-D

@mattleibow
Copy link
Contributor Author

I think there is a new beta that is based on the PR nuget which has the fix. Have you tested that?
But, either way in an hour or so we shall have a new toy to ply with.

@hajmajeboss
Copy link

I tested preview43 and it still crashes, do you mean that?

@mattleibow
Copy link
Contributor Author

Actually, this doesn't matter. You should be able to try out the things right now. I pushed 1.68.2-pr.1180.6 to the preview feed: https://aka.ms/skiasharp-eap/index.json

@hajmajeboss
Copy link

Ah, thanks, will try.

@hajmajeboss
Copy link

Got a crash already, nothing in logcat, the app just halted with no message.

@hajmajeboss
Copy link

hajmajeboss commented Mar 24, 2020

Seems to be quite stable, got one crash in three hours. Good work there.

@mattleibow
Copy link
Contributor Author

Hmmmm. A crash is still bad. What was happening at the time? This was Android?

Also, this should be the exact same code, but just in case, try the new preview 45 on NuGet.org.

@hajmajeboss
Copy link

I don't know, just moving around the map when it crashed. MIUI gives me the crash report most of the time, including segfaults, none this time - like it was killed due lacking memory or something. No other crashes since then.

@mattleibow mattleibow mentioned this pull request Mar 29, 2020
4 tasks
@mattleibow
Copy link
Contributor Author

Hey folks, it has been a long time coming and thanks to all the folks with your repros, samples and stack traces I think we have finally managed to fix those pesky crashes - at least the ones we know about. I have just pushed 1.68.2-preview.50 to NuGet so let me know if this fixes all the crashes you have ever had 😄

@mattleibow mattleibow added this to the v1.68.2 milestone Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants