GameAnalytics / GA-SDK-DEFOLD

Repository for GameAnalytics Defold SDK
MIT License
14 stars 7 forks source link

GA blocking Defold's ability to create proper crash reports of its engine? #22

Closed subsoap closed 4 years ago

subsoap commented 4 years ago

You may want to talk to Defold engine folks about this as it is somewhat serious if that is the case. People who use GA extension need to be able to get proper Defold crash reports generated.

the1schwartz commented 4 years ago

Ok thanks for the feedback. I have released a new version (v2.2.7) where by default the GA error reporting has been disable. It can still be enabled via the game.project file.

mathiaswking commented 4 years ago

Hi @the1schwartz ! I'm wondering how you've implemented this. I think the analytics of the callstacks is a great feature, and I see no reason why they both shouldn't work? The way we've implemented our crash log handling in Defold, is that we set our own signal handlers. We do this at engine init, before any third party libraries should have started.

My guess is that your library then set your own signal handlers (correct?).

My thinking is that you should be able to store the old handlers, and after calling you handler, also call the previous handler.

the1schwartz commented 4 years ago

Yeah I guess that makes sense @mathiaswking. I have made some changes in the developer branch of our C++ SDK repo that you can have a look at before I release a new version of it to see if that would work on how the SDK call the previous handler:

https://github.com/GameAnalytics/GA-SDK-CPP/blob/develop/source/gameanalytics/GAUncaughtExceptionHandler.cpp#L39-L87

https://github.com/GameAnalytics/GA-SDK-CPP/blob/develop/source/gameanalytics/GAUncaughtExceptionHandler.cpp#L89-L228

mathiaswking commented 4 years ago

I think it looks good. Hard to spot any direct errors :)

Two things caught my attention however:

std::exit

I'm concerned about the use of std::_Exit() I can't decide if a library should call exit(), or leave it for the application. In this case it might justifiable since it's unlikely the app will recover from this error.

reverse order

If we have the "exit" pattern as an example, if the app calls exit() in the signal handler, will the rest of your code be called? I think the app should have the last say in the order of things during shutdown? E.g. releasing resources etc.

the1schwartz commented 4 years ago

Ok thanks for the feedback. I have also been in doubt if the "exit" should be there or not. Examples around the internet all say different things. So I will remove the exit call and then have the call to the previous handler after GA's handler code instead of having it before, am I understanding you correctly?

mathiaswking commented 4 years ago

Yes, correct.

the1schwartz commented 4 years ago

Ok thanks. I have just released a new version now (v2.2.8). Let me know if it is still causing problems.