Megabit / Blazorise

Blazorise is a component library built on top of Blazor with support for CSS frameworks like Bootstrap, Tailwind, Bulma, AntDesign, and Material.
https://blazorise.com/
Other
3.29k stars 532 forks source link

[Bug]: NullReferenceException in Blazorise Datagrid #4995

Closed njannink closed 1 year ago

njannink commented 1 year ago

Blazorise Version

1.2.5

What Blazorise provider are you running on?

Material

Link to minimal reproduction, or a simple code snippet

While reviewing our logs I see the following NullReferenceException logged:

System.NullReferenceException: Object reference not set to an instance of an object.
   at Blazorise.DataGrid.DataGrid`1.IsUserAgentMacintoshOS()
   at Blazorise.DataGrid.DataGrid`1.OnAfterRenderAsync(Boolean firstRender)
   at Microsoft.AspNetCore.Components.RenderTree.Renderer.GetErrorHandledTask(Task taskToHandle, ComponentState owningComponentState)

I don't know the rootcause or repro scenario, but possibly a code review could point out the issue.

David-Moreira commented 1 year ago

Hello @njannink Hmm the stack trace shows it comes already from an error handling of some sorts? Microsoft.AspNetCore.Components.RenderTree.Renderer.GetErrorHandledTask(Task taskToHandle, ComponentState owningComponentState)

The IsUserMacintoshOs has been on the Datagrid after render for quite a few versions already. If there was a general problem with it. We would have a lot of reports of Datagrid not working.

As such can you provide more details or are you able to reproduce the issue?

Edit : sorry, on mobile, you already said you don't have a repro. Do you have any other details that can be helpful in troubleshooting this issue?

github-actions[bot] commented 1 year ago

Hello @njannink, thank you for your submission. The issue was labeled "Status: Repro Missing", as you have not provided a way to reproduce the issue quickly. Most problems already solve themselves when isolated, but we would like you to provide us with a reproducible code to make it easier to investigate a possible bug.

njannink commented 1 year ago

The call in JSUtilitiesModule

public virtual ValueTask<string> GetUserAgent()
        => InvokeSafeAsync<string>( "getUserAgent" );

This method can return null/default in case of a race condition on the dispose/cleanup. Also the underlying javascript call navigator.userAgent can return null so I guess the safest would be to check for null in the IsUserAgentMacintoshOS method in Datagrid ur return "unknow" as default for the GetUserAgent call.

Note: We recently switched to server-side Blazor so these unhandled exception at navigate away, timeouts etc that in the past you would not see because they happen in the client they now become visible in our server logs.

David-Moreira commented 1 year ago

If I remember correctly the InvokeSafeAsync we have is in fact to avoid those kind of race conditions, where something happens and JS was running, disposing, navigating away, etc... But maybe it's not bulletproof...

Also null errors in js don't propagate as a null object reference to c# as far as I know.

So there might be something else there we might be missing, but we'll take a look, thanks for the tips.

David-Moreira commented 1 year ago

Oh I just went and checked the code. I see what you mean. In case of something gone wrong, it returns the default. And the contains check that we have here might throw a null object reference then. You're right.

Adding a null check might be enough. image

It most certainly is not breaking anything for your users, just silently failing on the server, still not great to have exceptions getting thrown for no reason.

Again thanks for the help / tips.

mtbayley commented 1 year ago

Also noticing this exception. It has been a problem for a while now. Just getting around to reporting it.