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

Return an empty surface in case the allocation has failed #1784

Merged
merged 3 commits into from
Sep 6, 2021

Conversation

richirisu
Copy link
Contributor

@richirisu richirisu commented Aug 23, 2021

Description of Change

Return an empty surface in case the allocation has failed

Bugs Fixed

API Changes

None

Behavioral Changes

Prior to this change the application would crash with an unrecoverable exception. After the change the application does not crash anymore but the surface of the canvas view will remain blank, i.e. nothing will be drawn on the surface in case there is not sufficient memory to allocate.

PR Checklist

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

@mattleibow
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mattleibow
Copy link
Contributor

Thanks for this PR.

I was playing around with the test app from the issue and think it lasts even longer if I dispose the data when it is removed from the window. It still crashes, but much later. Without the fix, it crashed after about 380 navigations on mu iPhone 6s. With the fixe and the release, it lasted to about 500. It still crashes, but this was the app.

It started producing blank pages for a bit (basically ran out of memory to allocate the surface), and then crashed after a while - but not from our code. Seems the OS got tired of allocating view controllers. I suppose 500 is a bit extreme for an app to just keep navigating down while preserving the back stack. Some overhead might also be that Xamarin.Forms may not be disposing the actual UIViews.

At least now, we are making sure to release the data when it is removed from the view hierarchy.

I think this is better? We don't need to keep all the data in memory because we can totally re-allocate. And, the OS still has the last image in the graphics context. Our drawing is basically a second buffer. SkiaSharp draws to that, then blits it to the screen. If we dispose our buffer, the OS still has the copy.

@richirisu
Copy link
Contributor Author

Thanks for having looked into it this thoroughly.

I think releasing the pixel data when the canvas view is being removed from the view hierarchy is a good move. After all, it can be recreated. Back in the good old days of iOS 1, it was even required to free up all data that could be recreated when switching view controllers. Of course, my test application is just an extreme example, and since NSData objects share the memory with all the rest, there can be a multitude of reasons for running out of memory.

The only thing I am not really sure about is that in most cases willMoveToWindow will only be called when the view controller is being pushed and when it is being popped again, but not when another view controller is being pushed on top. So all intermediate view controllers will still keep their views alive. But I might be mistaken.

@mattleibow
Copy link
Contributor

willMoveToWindow will only be called when the view controller is being pushed and when it is being popped again, but not when another view controller is being pushed on top

I tested this and it seems that when we push another VC on top, it removes the previous one from the window. I did not actually expect that as I assumed it would still live on. However, it seems as soon as a view is no longer in the view hierarchy, it is removed from the window.

Maybe this was sort of a hint but not explicitly stated:

newWindow
The window object that will be at the root of the receiver's new view hierarchy. This parameter may be nil.

@richirisu
Copy link
Contributor Author

I could confirm in Xcode that you are right. Using the “Debug View Hierarchy” feature, only the top-most view controller is attached. All intermediate view controllers are detached for the time being.

I could also confirm that for all view controllers that are being detached willMoveToWindow will be called with ’null’ (which is in accordance with the specification as the view is being detached from the window). However, willMoveToSuperview will not be called with ‘null’ because the view is still attached to its superview. Only for the view controller's root view willMoveToSuperview will be called with ‘null’ because the root view is indeed detached from its parent view (which is a view belonging to the navigation controller). Note, that you can assign any custom view as the root view by overriding the loadView method of a view controller.

I think I had confused it with the search view controller. Displaying the search view controller does not cause the actual view controller underneath to be removed from the view hierarchy.

@mattleibow mattleibow merged commit 8b060e6 into mono:patch/v2.80.4 Sep 6, 2021
@mattleibow mattleibow added this to the v2.80.4 milestone May 22, 2022
@mattleibow
Copy link
Contributor

Fixes #1308

@mattleibow mattleibow linked an issue May 22, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BUG] SIGABRT: Object reference not set to an instance of an object
2 participants