firebase / quickstart-unity

Firebase Quickstart Samples for Unity
https://firebase.google.com/games
Apache License 2.0
834 stars 428 forks source link

Firebase.FirebaseApp.SetCheckDependenciesThread crash #510

Closed victords closed 4 years ago

victords commented 4 years ago

Please fill in the following fields:

Unity editor version: 2018.4.6f1 Firebase Unity SDK version: 6.6.0 Firebase plugins in use (Auth, Database, etc.): Analytics Additional SDKs you are using (Facebook, AdMob, etc.): Facebook, Vungle, Chartboost Platform you are using the Unity editor on (Mac, Windows, or Linux): Mac Platform you are targeting (iOS, Android, and/or desktop): Android Scripting Runtime (Mono, and/or IL2CPP): don't know

Please describe the issue here:

We are receiving a crash in this method, with the "Don't call other Firebase functions while CheckDependencies is running." message, but using the recommended pattern Firebase.FirebaseApp.CheckAndFixDependenciesAsync().ContinueWith(task => { ... }), and Firebase is only used in this single place.

Please answer the following, if applicable:

Have you been able to reproduce this issue with just the Firebase Unity quickstarts (this GitHub project)? No

What's the issue repro rate? (eg 100%, 1/5 etc) I don't know exactly, but it's very low (probably less than 1%).

stewartmiles commented 4 years ago

The exception error message Don't call other Firebase functions while CheckDependencies is running describes what you should do to avoid the exception. i.e you should not call other parts of Firebase while it's being initialized by CheckDependencies and instead wait until initialization is complete. I recommend checking your code to make sure that you're not calling Firebase methods from other parts of your application while Firebase is being initialized.

victords commented 4 years ago

Hi, thanks for the response. I already checked that. Here's the full initialization code:

        Firebase.FirebaseApp.CheckAndFixDependenciesAsync().ContinueWith(task => {
            var dependencyStatus = task.Result;
            if (dependencyStatus == Firebase.DependencyStatus.Available)
            {
                // Initializer maybe
                Firebase.Analytics.FirebaseAnalytics.SetAnalyticsCollectionEnabled(true);
                // Set the user ID.
                Firebase.Analytics.FirebaseAnalytics.SetUserId(_userProfile.PublicId.ToString());

                // Finish initializer
                Debug.Log("Firebase initialized");
            }
            else
            {
                Debug.LogError(String.Format("Dependency error: {0}", dependencyStatus)); // Firebase Unity SDK is not safe to use here.
            }
        });

Also I searched for firebase in the entire project and this is the only place where it is used... =/

stewartmiles commented 4 years ago

I'm afraid that's the only way this behavior can occur. If you decompile Firebase.App.dll you'll see that we have code like this...

  private static void ThrowIfCheckDependenciesRunning() {
    lock (CheckDependenciesThreadLock) {
      if (CheckDependenciesThread != CheckDependenciesNoThread &&
          CheckDependenciesThread !=
              System.Threading.Thread.CurrentThread.ManagedThreadId) {
        throw new System.InvalidOperationException(
            "Don't call Firebase functions before CheckDependencies has finished");
      }
    }
  }

  private static void SetCheckDependenciesThread(int threadId) {
    lock (CheckDependenciesThreadLock) {
      if (CheckDependenciesThread == CheckDependenciesNoThread ||
          CheckDependenciesThread == CheckDependenciesPendingThread ||
          CheckDependenciesThread ==
              System.Threading.Thread.CurrentThread.ManagedThreadId) {
        CheckDependenciesThread = threadId;
      } else {
        throw new System.InvalidOperationException(
            "Don't call other Firebase functions while CheckDependencies is running.");
      }
    }
  }

  public static System.Threading.Tasks.Task<DependencyStatus>
      CheckDependenciesAsync() {
    SetCheckDependenciesThread(CheckDependenciesPendingThread);
    Firebase.Platform.FirebaseHandler.CreatePartialOnMainThread(
        Firebase.Platform.FirebaseAppUtils.Instance);
    InitializeAppUtilCallbacks();
    return System.Threading.Tasks.Task.Run<DependencyStatus>(() => {
      // Set the thread id, so that this thread can access and create a
      // FirebaseApp.
      SetCheckDependenciesThread(
          System.Threading.Thread.CurrentThread.ManagedThreadId);
      DependencyStatus result = CheckDependencies();
      // Finished running CheckDependencies, so clear the thread for others.
      SetCheckDependenciesThread(CheckDependenciesNoThread);
      return result;
    });
  }

Then we use ThrowIfCheckDependenciesRunning in all code paths to initialize FirebaseApp to guard against other threads calling into Firebase APIs while this is happening.

Looking at your code, it seems fine though ContinueWith will may run the closure on a different thread to the main thread if you're using .NET 4.x.

Any chance the stack trace points at the problematic code path?

victords commented 4 years ago

Hi. The stack trace doesn't help much as this initialization happens inside a coroutine.

Firebase.FirebaseApp.SetCheckDependenciesThread (Firebase.FirebaseApp) Firebase.FirebaseApp.CheckDependenciesAsync (Firebase.FirebaseApp) Firebase.FirebaseApp.CheckAndFixDependenciesAsync (Firebase.FirebaseApp) App+cIterator8.MoveNext (App+cIterator8) Framework.CoroutineInternal.MoveNext (Framework.CoroutineInternal) Framework.CoroutineInternal1[TResult].MoveNext (Framework.CoroutineInternal1[TResult]) UnityEngine.SetupCoroutine.InvokeMoveNext (UnityEngine.SetupCoroutine) Framework.Task1[TResult].get_Result (Framework.Task1[TResult]) App+cIterator7.MoveNext (App+cIterator7) Framework.CoroutineInternal.MoveNext (Framework.CoroutineInternal) UnityEngine.SetupCoroutine.InvokeMoveNext (UnityEngine.SetupCoroutine) Framework.Task.Dispose (Framework.Task) Framework.Task.Dispose (Framework.Task) Framework.Task.Finish (Framework.Task) Framework.CoroutineInternal.MoveNext (Framework.CoroutineInternal) UnityEngine.SetupCoroutine.InvokeMoveNext (UnityEngine.SetupCoroutine)

I checked the code and there are 3 levels of nested coroutines before reaching the initialization... Do you think this could be a problem?

victords commented 4 years ago

By the way, looking at the decompiled code, it seems like if a thread calls "CheckDependenciesAsync" after a different thread has executed SetCheckDependenciesThread(System.Threading.Thread.CurrentThread.ManagedThreadId) but before executing SetCheckDependenciesThread(CheckDependenciesNoThread), it would trigger this error... Am I missing something?

stewartmiles commented 4 years ago

The code should be doing the following:

  public static System.Threading.Tasks.Task<DependencyStatus>
      CheckDependenciesAsync() {
    SetCheckDependenciesThread(CheckDependenciesPendingThread);  // <-- Thread 1: indicate that thread that will check dependencies is going to be set
    Firebase.Platform.FirebaseHandler.CreatePartialOnMainThread(
        Firebase.Platform.FirebaseAppUtils.Instance);
    InitializeAppUtilCallbacks();
    return System.Threading.Tasks.Task.Run<DependencyStatus>(() => {
      // Set the thread id, so that this thread can access and create a
      // FirebaseApp.
      SetCheckDependenciesThread(
          System.Threading.Thread.CurrentThread.ManagedThreadId); // <-- Thread 2: lock Firebase operations to this thread.
      DependencyStatus result = CheckDependencies();
      // Finished running CheckDependencies, so clear the thread for others.
      SetCheckDependenciesThread(CheckDependenciesNoThread);  // <-- Thread 2: all Firebase method calls from any thread
      return result;
    });
  }

This should mostly as Task.Run is going to spawn a unique thread for running the operation, preventing all other method calls on other threads during that period. Looking at this code again - it has been a long time - it looks like there is a race when spawning the thread which means that multiple concurrent calls to CheckDependenciesAsync() could end up causing some odd behavior.

google-oss-bot commented 4 years ago

Hey @victords. We need more information to resolve this issue but there hasn't been an update in 7 days. I'm marking the issue as stale and if there are no new updates in the next 3 days I will close it automatically.

If you have more information that will help us get to the bottom of this, just add a comment!

google-oss-bot commented 4 years ago

Since there haven't been any recent updates here, I am going to close this issue.

@victords if you're still experiencing this problem and want to continue the discussion just leave a comment here and we are happy to re-open this.