Blueprint Assist crashes quite a bit and the root cause is unsafe pointer usage

[samaursa]

First off, thanks for the plugin. The formatter is quite incredible.

BlueprintAssist formatter crashes quite a bit (at least 2 times a day) when formatting Blueprints. The complexity of the Blueprint does not seem to matter. Going through the code, I noticed that there are a lot of raw pointers.

For example, in EdGraphFormatter.h we have NodePool, NodeTree, LastFormattedComments etc. that are all storing raw pointers to UObjects that may be GC'ed at any time. So far, all of the crashes are due to the pointers getting invalidated. Just checking the pointer for nullptr is not enough as the pointer may be garbage but still appear valid to C++.

All such pointers should be stored as TWeakObjectPtr which has a built-in IsValid which should be checked every time it is dereferenced. Without doing this, eventually there will be a crash.

I hope that this is fixed in an upcoming release. As it stands, I am no longer able to convince my team to continue using the plugin if they encounter multiple crashes throughout the day due to it.

[fpwong]

Hi @samaursa, thanks for looking into this. I have been noticing a nullptr related crash recently which have been causing me some headache. I think there is a good chance what you said has perhaps caused issues.

Generally speaking I have been quite lazy in avoiding raw ptrs. Normally I kept them in when I was sure that the scope of the class would be not persist i.e. the formatter is used in a single function and unless you have enabled the setting bEnableFasterFormatting, the formatter would not live outside of the function so in this way it shouldn't cause GC issues. Please correct me if I'm wrong in saying that.

But that is assuming that engine code does not try to do anything unexpected from something I have called. So while it is a large change, fixing this should allow me to rule out this as a potential source of issues. I'll see if I can get this into the next version but it might take some time.

[samaursa]

Normally I kept them in when I was sure that the scope of the class would be not persist i.e. the formatter is used in a single function and unless you have enabled the setting bEnableFasterFormatting, the formatter would not live outside of the function so in this way it shouldn't cause GC issues. Please correct me if I'm wrong in saying that.

If I am understanding correctly, you mean that the formatter is created and destroyed in the same frame. In which case you would be right that shouldn't have GC related issues. However, the data that's given to the formatter might already be invalid, causing the formatter to eventually crash. I see that FBAGraphHandler has a few variables that are using raw pointers.

But that is assuming that engine code does not try to do anything unexpected from something I have called.

Given the crashes and the callstacks I've seen, using TWeakObjectPtr as a replacement of the raw pointers would definitely resolve some of the crashes (perhaps majority of our crashes). This will force a check if (NodePtr.IsValid()) before any usage, making the code more reslient. Of course, it will also silently ignore errors that perhaps should not be ignored. However, I'd rather the tool's formatting fails instead of crashing the editor. Ensuresmsgf/logs can help.

So while it is a large change, fixing this should allow me to rule out this as a potential source of issues. I'll see if I can get this into the next version but it might take some time.

Thank you for looking into this! Looking forward to the next release with the fix.