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

FR: Run Logged Exception Handling Off Thread #860

Open
allan-o3h opened this issue Nov 7, 2020 · 2 comments
Open

FR: Run Logged Exception Handling Off Thread #860

allan-o3h opened this issue Nov 7, 2020 · 2 comments

Comments

@allan-o3h
Copy link

Feature proposal

  • Firebase Component: Crashlytics

When the Firebase.Crashlytics.LoggedException class is created it runs a lot of Regex matches on the stacktrace. This can take several milliseconds, 15ms on an iPhone 8 in an example I profiled.

image

Firebase.Crashlytics.ExceptionHandler registers a listener to Application.logMessageReceived on app startup and creates an instance of that LoggedException for every logged exception on the main thread. This can create a frame spike in our application.

In our application we also forward logged error messages to Firebase.Crashlytics.Crashlytics.LogException by listening to Application.logMessageReceived, but we push those messages off the main thread and trigger the logging from there. That appears to work without issues, so I'm assuming all the supporting infrastructure is thread safe.

Ideally there would be an option to either:

  1. Not have the ExceptionHandler register the logMessageReceived handler so we could manually push in the exceptions off thread
  2. Have the ExceptionHandler process the exceptions off the main thread.
@samedson
Copy link

Hey thank you for the feedback. We don't have a good way to do this right now because customers usually want a guarantee that crash reports are sent in order with their logs / custom keys. If this becomes async we've seen cases where logs and custom keys are out of order.

But the 15ms can be really bad for a game, so we'll take a look at this feature request and figure out whether to take it on.

@allan-o3h
Copy link
Author

For reference this is how I've worked around this, given the Unity source isn't available.

Add a post build script to modify the code generated by the plugin to prevent it from adding listeners to handle errors / crashes.

    /// <summary>
    /// Modify the generated cpp code from the firebase crashlytics library to prevent it from handling logged errors or unhandled exceptions
    ///
    /// This is to workaround https://github.com/firebase/quickstart-unity/issues/860
    ///
    /// Instead of letting the library handle this processing on the main thread, our implementation will handle the same data and log it in another thread
    /// </summary>
    public class DisableNativeCrashlyticsListeners : IPostprocessBuildWithReport
    {
        public int callbackOrder => 1;

        public void OnPostprocessBuild(BuildReport report)
        {
            if (report.summary.platform != BuildTarget.iOS)
            {
                return;
            }

            Debug.Log("Disabling the internal crash and error listeners in the Crashlytics library");

            // open the native generated cpp file
            string filePath = Path.Combine(report.summary.outputPath, "Classes", "Native", "Firebase.Crashlytics.cpp");
            string[] lines = File.ReadAllLines(filePath);

            // search for the function to register the exceptionHandler
            int lineToModify = -1;
            for (int i = 0; i < lines.Length; i  )
            {
                if (lines[i] == "// System.Void Firebase.Crashlytics.ExceptionHandler::Register()")
                {
                    // skip over the function declaration and look for the function itself
                    if (!lines[i   1].Contains(";"))
                    {
                        lineToModify = i   2;
                        break;
                    }
                }
            }

            // return immediately, which prevents it from adding listeners
            Debug.Log("Firebase.Crashlytics.cpp Line To Modify: "   lineToModify);
            if (lineToModify > 0)
            {
                string line = lines[lineToModify];
                Debug.Log("Line Before: "   line);
                line  = " return;";
                lines[lineToModify] = line;
                Debug.Log("Line After: "   line);
            }
            else
            {
                Debug.LogError("Unable to find the line to modify in the Firebase.Crashlytics.cpp file");
            }

            // save results
            File.WriteAllLines(filePath, lines);
        }
    }

This isn't going to be a stable solution and is subject to breaking with library changes, but it works well enough.

Then you need to add your own listeners and process them off the main thread:


// listen for Unity log messages and crashes
Application.logMessageReceivedThreaded  = OnLogMessageReceived;
AppDomain.CurrentDomain.UnhandledException  = OnUnhandledException;

// use reflection to get a reference to the static function to let use create the exception crashlytics will flag as a crash
Type classType = Type.GetType("Firebase.Crashlytics.LoggedException, Firebase.Crashlytics, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null");
CreateUnhandledException = classType?.GetMethod("FromException", BindingFlags.Static | BindingFlags.Public);

private static void OnUnhandledException(object sender, UnhandledExceptionEventArgs eArgs)
{
    // move processing off thread
    Task.Run(() =>
    {
        // matching the exact behaviour of the internal crashlytics
        Crashlytics.LogException((Exception)CreateUnhandledException.Invoke(null, new object[] { (Exception)eArgs.ExceptionObject }));
    });
}

private static void OnLogMessageReceived(string logString, string stackTrace, LogType logType)
{
    // only forward errors and exceptions (you could make this warnings or whatever here)
    if (logType == LogType.Error || logType == LogType.Exception)
    {
        // Log as a HandledException
        Task.Run(() =>
        {
             Crashlytics.LogException(new HandledException(logString, stackTrace));
        });
    }
}

// custom exception to match the stack trace of the logged exception
public class HandledException : Exception
{
    public HandledException(string message, string stackTrace) : base(message)
    {
         StackTrace = stackTrace;
    }

    public override string StackTrace { get; }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants