Possible race condition in proxy object creation

[filipnavara]

Android framework version

net8.0-android

Affected platform version

VS 2022 17.12.4

Description

We spent the last few weeks trying to track down a mysterious bug where two instances of Android.App.Application .NET proxy objects are created. So far we were not able to reproduce the issue locally but it happens quite consistently for thousands of our customers.

Finally we were able to get stack traces from the constructor where the second instance is created.

The usual first instance gets created with this stack trace:

AndroidApp::.ctor
   at System.Environment.get_StackTrace()
   at MailClient.Mobile.Droid.AndroidApp..ctor(IntPtr handle, JniHandleOwnership ownership)
   at System.Reflection.RuntimeConstructorInfo.InternalInvoke(RuntimeConstructorInfo , Object , IntPtr* , Exception& )
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Constructor(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object , Span`1 , BindingFlags )
   at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object , BindingFlags , Binder , Object[] , CultureInfo )
   at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags , Binder , Object[] , CultureInfo )
   at System.Reflection.ConstructorInfo.Invoke(Object[] parameters)
   at Java.Interop.TypeManager.CreateProxy(Type , IntPtr , JniHandleOwnership )
   at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
   at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
   at Java.Lang.Object._GetObject[Application](IntPtr , JniHandleOwnership )
   at Java.Lang.Object.GetObject[Application](IntPtr handle, JniHandleOwnership transfer)
   at Java.Lang.Object.GetObject[Application](IntPtr jnienv, IntPtr handle, JniHandleOwnership transfer)
   at Android.App.Application.n_OnCreate(IntPtr jnienv, IntPtr native__this)
   at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PP_V(_JniMarshal_PP_V callback, IntPtr jnienv, IntPtr klazz)
-- Java --
	at crc647fae2f69c19dcd0d.AndroidApp.n_onCreate(Native Method)
	at crc647fae2f69c19dcd0d.AndroidApp.onCreate(AndroidApp.java:25)
	at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1316)
	at android.app.ActivityThread.handleBindApplication(ActivityThread.java:7848)
	at android.app.ActivityThread.-$$Nest$mhandleBindApplication(Unknown Source:0)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2486)
	at android.os.Handler.dispatchMessage(Handler.java:106)
	at android.os.Looper.loopOnce(Looper.java:230)
	at android.os.Looper.loop(Looper.java:319)
	at android.app.ActivityThread.main(ActivityThread.java:9063)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:588)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1103)

The second instance comes from a worker (subclass of AndroidX.Work.Worker):

   at System.Environment.get_StackTrace()
   at MailClient.Mobile.Droid.AndroidApp..ctor(IntPtr handle, JniHandleOwnership ownership)
   at System.Reflection.RuntimeConstructorInfo.InternalInvoke(RuntimeConstructorInfo , Object , IntPtr* , Exception& )
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Constructor(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object , Span`1 , BindingFlags )
   at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object , BindingFlags , Binder , Object[] , CultureInfo )
   at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags , Binder , Object[] , CultureInfo )
   at System.Reflection.ConstructorInfo.Invoke(Object[] parameters)
   at Java.Interop.TypeManager.CreateProxy(Type , IntPtr , JniHandleOwnership )
   at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
   at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
   at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayElementToManaged>b__70_9(Type type, IntPtr source, Int32 index)
   at Android.Runtime.JNIEnv.GetObjectArray(IntPtr , Type[] )
   at Java.Interop.TypeManager.n_Activate(IntPtr jnienv, IntPtr jclass, IntPtr typename_ptr, IntPtr signature_ptr, IntPtr jobject, IntPtr parameters_ptr)
   at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPLLLL_V(_JniMarshal_PPLLLL_V callback, IntPtr jnienv, IntPtr klazz, IntPtr p0, IntPtr p1, IntPtr p2, IntPtr p3)
-- Java --
	at mono.android.TypeManager.n_activate(Native Method)
	at mono.android.TypeManager.Activate(TypeManager.java:7)
	at crc647fae2f69c19dcd0d.SyncWorker.<init>(SyncWorker.java:23)
	at java.lang.reflect.Constructor.newInstance0(Native Method)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:343)
	at androidx.work.WorkerFactory.createWorkerWithDefaultFallback(WorkerFactory.java:95)
	at androidx.work.impl.WorkerWrapper.runWorker(WorkerWrapper.java:243)
	at androidx.work.impl.WorkerWrapper.run(WorkerWrapper.java:144)
	at androidx.work.impl.utils.SerialExecutorImpl$Task.run(SerialExecutorImpl.java:96)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
	at java.lang.Thread.run(Thread.java:1012)

Notably, the first instance exists and it's not collected on the .NET side (pinned through a static variable). This suggests that there's a race condition that could lead to creation of two proxy objects.

Steps to Reproduce

#9862 (comment)

Did you find any workaround?

No response

Relevant log output

[filipnavara]

If it's expected that a second proxy object can be instantiated (*), then feel free to delegate the issue to dotnet/maui because they depend on the behaviour that this doesn't happen by saving the instance to a static variable:
https://github.com/dotnet/maui/blob/196a1d651c2d0abd60054d6a95866e9b2a14f66f/src/Core/src/Platform/Android/MauiApplication.cs#L19-L22

This in turn is accessed all over that place. With the example stack traces above, once the Application.OnCreate virtual method is called the instance saved to the static variable in constructor has been overwritten.

(*) I very much doubt it's intentional; if there were data associated with the object it would create a lot of issues.

[jonathanpeppers]

Do you have a small example of your AndroidX.Work.Worker and where do you start it?

Typical operation would be:

1. Application is created
2. ContentProvider starts the runtime
3. Worker starts

But I wonder if no. 3 can start happening in parallel with 2.

[filipnavara]

Do you have a small example of your AndroidX.Work.Worker and where do you start it?

The source code for the worker itself is likely not interesting - https://gist.github.com/filipnavara/c8e196bb1bd4471a00174708366ef6ff.

What is likely more interesting is how the worker is scheduled:

var syncWorkRequest = (PeriodicWorkRequest.Builder.From<SyncWorker>(TimeSpan.FromMinutes(15))
  .SetConstraints(new Constraints.Builder().SetRequiredNetworkType(NetworkType.Connected).Build())
  .SetInitialDelay(15, TimeUnit.Minutes) as PeriodicWorkRequest.Builder).Build();
WorkManager.GetInstance(context).EnqueueUniquePeriodicWork("sync", ExistingPeriodicWorkPolicy.Update, syncWorkRequest);

The traces we get from customers suggest that it's often happening after clicking on a notification. It's likely that the "required network type" constraint may play a role because some devices tend to do a "soft sleep" mode for network connections when locked. In this scenario it's more likely that the worker will get triggered alongside creation of the activity to handle the notification.

That said, it's just a theory. We don't have repro on any of our own devices and we have tried for weeks. Weirdly it happens to a large percent of customers in the wild regularly enough.

I don't see any locking in Java.Lang.Object.GetObject that would prevent two calls getting past the Peek* phase and creating separate instances. Launching the worker and the activity itself definitely happens from separate threads.

[filipnavara]

We managed to reproduce it on our devices for the first time 🎊

The trick is to mark the application as "Restricted" in the Battery settings. This prevents it from running in the background. We then run it and put it to background. Then we send a notification which wakes up the app and sets up the worker to run every 15 minutes. However, the app is forbidden to run in background, so it doesn't immediately trigger. After 15 minutes we start the app from notification and the issue reproduces because the system starts the worker alongside the app initialization.

This scenario can likely be simplified. The notifications are not essential part, the battery saving setting is.

Turns out that the crash logged from my device is 2 days old, so still no reliable repro 😔

[filipnavara]

I have a somewhat reliable repro:
AndroidWorker.zip

• Run the app from Visual Studio with attached debugger
• Press the "Run worker" button
• Kill the app (eg. stop in VS)
• Wait a minute
• Place a breakpoint in TestContentProvider.OnCreate (on the return true line) and Worker..ctor
• Run the app

Both breakpoints should be hit, first TestContentProvider.OnCreate, then Worker..ctor. Once you are in Worker..ctor observe in the watch window that MainApplication.constructorCallCount == 2 (ie. the constructor was called twice). Sometimes it happens even on the first breakpoint.

It's not the most reliable repro ever but I managed to hit it on two separate devices and Windows Subsystem for Android. Restarting the app when on the Worker..ctor breakpoint usually keeps the worker scheduled and the next run will exhibit the bug too.

[jonpryor]

I'm not sure that this is "reasonably" fixable.

Among the many guidelines around writing thread safe code are two of interest here:

1. The lock should be held for as short as time as possible, and
2. Code within the lock should not call "arbitrary" code.

See e.g. The COM static store, part 3: Avoiding creation of an expensive temporary when setting a singleton:

be extremely mindful about what you do while holding a lock. Don’t call out to foreign code while holding a lock.

Attempting to throw a lock around TypeManager.CreateInstance() immediately violates both of those guidelines:

• It's a lot of code to protect by a lock, and
• It invokes arbitrary code! TypeManager.CreateInstance() calls TypeManager.CreateProxy() which looks up and invokes the T(IntPtr, JniHandleOwnership) constructor, which is user-written code and can thus do anything, including calls to Object.GetObject<T>(). (Hello deadlock?)

I do not believe it is possible to prevent the creation of two AndroidApp instances, not without introducing the possibility of deadlock. Even in the "simpler" thread-safe The COM static store, part 2: Race conditions in setting a singleton, part of the approach involves the possibility of creating two of a thing, knowing that you'd throw one away. ("This algorithm assumes that creating a Thing and throwing it away is relatively harmless.")

Given that this scenario is difficult to address, what underlying issue are you trying to fix/workaround? What happens when two Application instances are created that made you care about this issue in the first place? Maybe we can improve or address that?