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

[BUG] SIGABRT: Object reference not set to an instance of an object #1308

Closed
arashcoder opened this issue May 25, 2020 · 20 comments · Fixed by #1784
Closed

[BUG] SIGABRT: Object reference not set to an instance of an object #1308

arashcoder opened this issue May 25, 2020 · 20 comments · Fixed by #1784
Milestone

Comments

@arashcoder
Copy link

arashcoder commented May 25, 2020

Description

Hi! I'm using Microcharts library, which uses SkiaSharp under the hood. Occasionally on iOS, I get null reference exception. Here's all the crash log I get:

Code

SKCGSurfaceFactory.CreateSurface (CoreGraphics.CGRect contentsBounds, System.nfloat scale, SkiaSharp.SKImageInfo& info)
SKCanvasView.Draw (CoreGraphics.CGRect rect)
(wrapper managed-to-native) 
UIKit.UIApplication.UIApplicationMain(int,string[],intptr,intptr)
UIApplication.Main (System.String[] args, System.IntPtr principal, System.IntPtr delegate)
UIApplication.Main (System.String[] args, System.String principalClassName, System.String delegateClassName)
Application.Main (System.String[] args)

Expected Behavior

It shouldn't crash.

Actual Behavior

It crashes occasionally.

Basic Information

  • Version with issue: 1.68.0
  • Last known good version:
  • IDE: Visual Studio for Mac
  • Platform Target Frameworks:
    • iOS: 13.4
  • Target Devices: iPhone 8 Plus, iPhone XR, iPad (5th gen, Wi-Fi)

Any ideas why this occurs?
Thanks in advance.

@mattleibow
Copy link
Contributor

Does it still crash if you update to the latest v1.68.3?

@arashcoder
Copy link
Author

Thanks for the suggestion. I will try that, but since I can't reproduce that crash I can't be sure.

@arashcoder
Copy link
Author

@mattleibow I updated to v1.68.3 and I still get that crash occasionally.
Any ideas?
Thanks.

@mattleibow
Copy link
Contributor

Could you paste the full stack trace? Very weird.

@arashcoder
Copy link
Author

That's the whole stack trace that I get in AppCenter.

@mattleibow
Copy link
Contributor

This is very weird because there is not any way to get nulls...
https://github.com/mono/SkiaSharp/blob/v1.68.3/source/SkiaSharp.Views/SkiaSharp.Views.Apple/SKCGSurfaceFactory.cs#L26-L59

The only possible way to get a null is with this line:

bitmapData = NSMutableData.FromLength(info.BytesSize);

That should not return null, so this whole thing looks off.

Do you have any other info that may possibly be related? Is this a big graph, small graph, animated? What was happening before/during updates. Maybe you are accessing something from a background thread and there is a hidden exception.

Are you able to share the actual text message form the exception?

@arashcoder
Copy link
Author

Thanks for the explanations. Indeed it's strange.
This is the whole log that I get from this exception. I don't think I'm doing anything special with the graphs, and it's working almost all the time. The problem is that I can't reproduce this, so I don't have a full stack trace.
Also could this line cause the crash?
Info = info = CreateInfo((int)contentsBounds.Width, (int)contentsBounds.Height);

@mattleibow
Copy link
Contributor

mattleibow commented Jun 11, 2020

That is working with structs, which cannot be null. How rare is this exception? Maybe iOS just decides that it will return null when it feels like it.

@yungkakin
Copy link

I am also encountering this issue. Not easy to reproduce but it indeed happens on iOS. (Never happen on Android in my case)
The SkiaSharp version I am using is 2.80.2.

@arashcoder
Copy link
Author

So could someone please have a look at this?

@yungkakin
Copy link

It seems no one is going to handle this issue. Is SkiaSharp really stable on iOS?

@richirisu
Copy link
Contributor

Hello, I am new to this conversation. I can confirm that this issue does indeed occur from time to time. It is more likely to occur in situations of low memory, e.g. when using SkiaSharp to render full-screen. Indeed, NSMutableData.FromLength is not supposed to return null by default, but it is possible when there is not enough (continuous) memory available to allocate in the first place. Then null will be returned.

Of course, I am unaware about how to proceed best in case there is not enough memory to allocate for the drawing surface. I mean, continuing with a blank surface is not a proper solution either.

By the way, I get the same crash log as arashcoder has in his first post.

@mattleibow
Copy link
Contributor

I'll have another look as this seems like it should not happen. We had a memory block that we just freed, so there should be some memory.
Also, this only runs when the view bounds changes as we reuse the data object between paints.

Is there some sort of view changes happening when it crashes?

What I could do is check for null and throw a better exception and see if that appears in the stacktrace so we know for sure?

@richirisu
Copy link
Contributor

richirisu commented Feb 17, 2021

No pressure, and thanks for looking further into it.

In my case the Xamarin.iOS application opens a Xamarin.Forms Page whose entire content is a single SKCanvasView. The crash occurs at creation time, more precisely, when the page becomes visible for the first time. It does not even get to any custom drawing code. As such, I suppose no bitmap data has been allocated yet. With the currently biggest iPad resolution available a chunk of 22 MB is necessary that must fit in one continuous space in memory. And depending on current memory usage this is sometimes a problem.

As mentioned before, it does not happen all the time and it is more likely to occur on a device that has many applications installed / opened. The aforementioned page can be closed and opened over and over again, thus, allocating a new SKCanvasView each time. In general, this is not a problem. Further, the few crash reports I got all have in common that they occurred roughly a week after the application was launched. Memory leaks and memory defragmentation might increase the chances, as it seems.

In respect to the stack trace, I doubt throwing another exception in return is going to help much. Of course, it would slightly be more expressive than a null reference exception. However, if an exception is not handled in the Draw method of an UIView, the application will crash consequently.

https://github.com/mono/SkiaSharp/blob/v1.68.3/source/SkiaSharp.Views/SkiaSharp.Views.AppleiOS/SKCanvasView.cs#L81-L106

As far as I understand, OnPaintSurface is not called if the surface size is empty. So maybe it is a viable workaround to simply render a blank canvas in case of an exception, or as an alternative, to render the message of the exception directly to the canvas (not a fan of the latter though but I have seen many design tools doing exactly that). I have to admit, it’s not very elegant though. At least the result would be that the application won’t crash right away, just the canvas view being blank.

Still, I think, there should be a means to let the implementer know that the bitmap data could not be allocated properly.

Performance-wise, I am not sure how the costs are for having a try-catch-block in the draw method.

richirisu added a commit to richirisu/SkiaSharpTest that referenced this issue Jun 7, 2021
@richirisu
Copy link
Contributor

I created a test application in order to reproduce the error. In the test application, a new page is being pushed to the navigation page by tapping the screen. The content view of each page is a new instance of a canvas view filled with a random background color, simple as that. The title bar shows the current number of stacked pages, as well as the currently used amount of memory (wich is the same as the profiling tool would display).

Among others, depending on the device, on the screen size and on how many applications are currently being cached, the test application will crash at some point and throw the above exception - exactly at the code line which I had mentioned in my other post. In my case at around 150 pages and 2200 MB. Keep tapping the screen ;)

Note, that you must test this on an actual device, because the simulator has sufficient memory available.

I also took the freedom to provide a fix for this issue. The application does not crash anymore if the memory is low. This is, an empty surface (height = width = 0) will be returned and consequently the rendering will be skipped. In the test application the canvas view will stay white/transparent in this case.

I think by returning an empty surface it is in accordance with what is done at https://github.com/mono/SkiaSharp/blob/v2.80.2/source/SkiaSharp.Views/SkiaSharp.Views.Apple/SKCGSurfaceFactory.cs#L35-L40 where also a new but empty SKSurface object is being returned.

Please, have a look at the following links, one without and one with the bugfix. SkiaSharp is being referenced as a submodule.

https://github.com/richirisu/SkiaSharpTest/releases/tag/without-bugfix
https://github.com/richirisu/SkiaSharpTest/releases/tag/with-bugfix

SkiSharp submodule with bugfix:

richirisu@c70ab35

If it's okay, I can also create a pull request. Hope it helps!

@rhult
Copy link

rhult commented Aug 12, 2021

Just adding that I'm seeing this bug consistently on iPad and the analysis by @richirisu above makes sense to me.

@thisisthekap
Copy link

@mattleibow Does the fix suggested by @richirisu wound decent to you?

image

@mattleibow
Copy link
Contributor

mattleibow commented Aug 19, 2021

I'll try get this in with the patch that I have to do.

My question is that this should not be happening and will only happen if the view is not collected. The fix looks sensible, nothing could be allocated, so just stop trying. However, I don't like the fact that we run out of memory across page changes. We probably need to somehow release memory when we navigate away.

But for now, I'll get your fixes in. Would you like to create a PR? Thanks a lot for the research and repro cases.

If you would like to make a PR (get your name in the books!), then do so against the patch/v2.80.4 branch.

@richirisu
Copy link
Contributor

I get your point, however, I think that in most cases it's less a problem of allocating a multitude of canvas views that causes the application to run into low memory but a low availability of free memory to work with in the first place, for instance, too many other applications in the background or the own application doing too much work in some manner or another.

In our case, for instance, there is only a single canvas view that covers the entire screen size, and it happens so that on creation of the view the surface cannot be instantiated due to low memory. All previously allocated canvas views have already been garbage collected beforehand.

The fix I am suggesting is not a solution regarding a proper memory management, that is correct. At least, it offers an additional guarding condition for the problem on the surface factory level, so that you may very well come up with a proper memory management solution in the future.

Another advantage I see is that when the surface of the canvas view is empty/blank after the creation of canvas view itself, it is then possible to test for that circumstance and to present an alert to the user that there was a problem, not enough memory etc. Until now, what happened, was that the application had simply crashed.

@mattleibow
Copy link
Contributor

Fixed in dbcd625

@mattleibow mattleibow added this to the v2.80.4 milestone May 22, 2022
@mattleibow mattleibow moved this to Done in SkiaSharp Backlog May 22, 2022
@mattleibow mattleibow linked a pull request May 22, 2022 that will close this issue
4 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants