Bug 1767128 - Rework and re-enable SurfaceControl rendering path on Android. r=agi,gfx-reviewers,aosmond

In bug 1762424 we introduced a rendering path on Android using the
SurfaceControl API, in order to work around a bug preventing recovery
from a GPU process crash. However, the initial implementation caused
this bug: repeatedly sending the same SurfaceControl objects over AIDL
to the GPU process resulted in them being leaked, eventually causing
severe display issues. Not only were we duplicating the SurfaceControl
for each widget, but each time a widget was resized too.

This patch reworks our usage of the SurfaceControl API to avoid ever
having to send them cross-process. Instead, we create a single child
SurfaceControl object for each SurfaceControl that is attached to a
widget. (Typically there will be a single one shared between all
widgets, changing only when the app is paused and resumed, which is
much fewer than one per widget per resize.)

In the parent process we obtain the Surfaces that will be rendered in
to from the child SurfaceControls, and only send those Surfaces to the
GPU process. Thankfully unlike SurfaceControls, sending Surfaces
cross-process does not cause leaks. When the GPU process dies we
simply destroy all of the child SurfaceControls, and recreate them
again on-demand.

Differential Revision: https://phabricator.services.mozilla.com/D147437

gfx/ipc/GPUProcessManager.cpp

@@ -56,6 +56,7 @@
 #include "nsPrintfCString.h"
 
 #if defined(MOZ_WIDGET_ANDROID)
+#  include "mozilla/java/SurfaceControlManagerWrappers.h"
 #  include "mozilla/widget/AndroidUiThread.h"
 #  include "mozilla/layers/UiCompositorControllerChild.h"
 #endif  // defined(MOZ_WIDGET_ANDROID)
@@ -825,6 +826,10 @@ void GPUProcessManager::HandleProcessLost() {
 
   DestroyRemoteCompositorSessions();
 
+#ifdef MOZ_WIDGET_ANDROID
+  java::SurfaceControlManager::GetInstance()->OnGpuProcessLoss();
+#endif
+
   // Re-launch the process if immediately if the GPU process is still enabled.
   // Except on Android if the app is in the background, where we want to wait
   // until the app is in the foreground again.

gfx/layers/ipc/UiCompositorControllerChild.cpp

@@ -332,16 +332,15 @@ void UiCompositorControllerChild::SetCompositorSurfaceManager(
 };
 
 void UiCompositorControllerChild::OnCompositorSurfaceChanged(
-    int32_t aWidgetId, java::sdk::Surface::Param aSurface,
-    java::sdk::SurfaceControl::Param aSurfaceControl) {
+    int32_t aWidgetId, java::sdk::Surface::Param aSurface) {
   // If mCompositorSurfaceManager is not set then there is no GPU process and
   // we do not need to do anything.
   if (mCompositorSurfaceManager == nullptr) {
     return;
   }
 
-  nsresult result = mCompositorSurfaceManager->OnSurfaceChanged(
-      aWidgetId, aSurface, aSurfaceControl);
+  nsresult result =
+      mCompositorSurfaceManager->OnSurfaceChanged(aWidgetId, aSurface);
 
   // If our remote binder has died then notify the GPU process manager.
   if (NS_FAILED(result)) {

gfx/layers/ipc/UiCompositorControllerChild.h

@@ -70,9 +70,8 @@ class UiCompositorControllerChild final
   // Note that this function does not actually use the PUiCompositorController
   // IPDL protocol, and instead uses Android's binder IPC mechanism via
   // mCompositorSurfaceManager. It can be called from any thread.
-  void OnCompositorSurfaceChanged(
-      int32_t aWidgetId, java::sdk::Surface::Param aSurface,
-      java::sdk::SurfaceControl::Param aSurfaceControl);
+  void OnCompositorSurfaceChanged(int32_t aWidgetId,
+                                  java::sdk::Surface::Param aSurface);
 #endif
 
  protected:

mobile/android/geckoview/src/main/aidl/org/mozilla/gecko/gfx/ICompositorSurfaceManager.aidl

@@ -5,8 +5,7 @@
 package org.mozilla.gecko.gfx;
 
 import android.view.Surface;
-import android.view.SurfaceControl;
 
 interface ICompositorSurfaceManager {
-    void onSurfaceChanged(int widgetId, in Surface surface, in SurfaceControl surfaceControl);
+    void onSurfaceChanged(int widgetId, in Surface surface);
 }

mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/CompositorSurfaceManager.java

@@ -7,7 +7,6 @@ package org.mozilla.gecko.gfx;
 
 import android.os.RemoteException;
 import android.view.Surface;
-import android.view.SurfaceControl;
 import org.mozilla.gecko.annotation.WrapForJNI;
 
 public final class CompositorSurfaceManager {
@@ -20,9 +19,8 @@ public final class CompositorSurfaceManager {
   }
 
   @WrapForJNI(exceptionMode = "nsresult")
-  public synchronized void onSurfaceChanged(
-      final int widgetId, final Surface surface, final SurfaceControl surfaceControl)
+  public synchronized void onSurfaceChanged(final int widgetId, final Surface surface)
       throws RemoteException {
-    mManager.onSurfaceChanged(widgetId, surface, surfaceControl);
+    mManager.onSurfaceChanged(widgetId, surface);
   }
 }

mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/SurfaceControlManager.java

@@ -0,0 +1,93 @@
+/* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.gecko.gfx;
+
+import android.os.Build;
+import android.view.Surface;
+import android.view.SurfaceControl;
+import androidx.annotation.RequiresApi;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.mozilla.gecko.annotation.WrapForJNI;
+
+// A helper class that creates Surfaces from SurfaceControl objects, for the widget to render in to.
+// Unlike the Surfaces provided to the widget directly from the application, these are suitable for
+// use in the GPU process as well as the main process.
+//
+// The reason we must not render directly in to the Surface provided by the application from the GPU
+// process is because of a bug on Android versions 12 and later: when the GPU process dies the
+// Surface is not detached from the dead process' EGL surface, and any subsequent attempts to
+// attach another EGL surface to the Surface will fail.
+//
+// The application is therefore required to provide the SurfaceControl object to a GeckoDisplay
+// whenever rendering in to a SurfaceView. The widget will then obtain a Surface from that
+// SurfaceControl using getChildSurface(). Internally, this creates another SurfaceControl as a
+// child of the provided SurfaceControl, then creates the Surface from that child. If the GPU
+// process dies we are able to simply destroy and recreate the child SurfaceControl objects, thereby
+// avoiding the bug.
+public class SurfaceControlManager {
+  private static final String LOGTAG = "SurfaceControlManager";
+
+  private static final SurfaceControlManager sInstance = new SurfaceControlManager();
+
+  private WeakHashMap<SurfaceControl, SurfaceControl> mChildSurfaceControls = new WeakHashMap<>();
+
+  @WrapForJNI
+  public static SurfaceControlManager getInstance() {
+    return sInstance;
+  }
+
+  // Returns a Surface of the requested size that will be composited in to the specified
+  // SurfaceControl.
+  @RequiresApi(api = Build.VERSION_CODES.Q)
+  @WrapForJNI(exceptionMode = "abort")
+  public synchronized Surface getChildSurface(
+      final SurfaceControl parent, final int width, final int height) {
+    SurfaceControl child = mChildSurfaceControls.get(parent);
+    if (child == null) {
+      // We must periodically check if any of the SurfaceControls we are managing have been
+      // destroyed, as we are unable to directly listen to their SurfaceViews' surfaceDestroyed
+      // callbacks, and they may not be attached to any compositor when they are destroyed meaning
+      // we cannot perform cleanup in response to the compositor being paused.
+      // Doing so here, when we encounter a new SurfaceControl instance, is a reasonable guess as to
+      // when a previous instance may have been released.
+      final Iterator<Map.Entry<SurfaceControl, SurfaceControl>> it =
+          mChildSurfaceControls.entrySet().iterator();
+      while (it.hasNext()) {
+        final Map.Entry<SurfaceControl, SurfaceControl> entry = it.next();
+        if (!entry.getKey().isValid()) {
+          it.remove();
+        }
+      }
+
+      child = new SurfaceControl.Builder().setParent(parent).setName("GeckoSurface").build();
+      mChildSurfaceControls.put(parent, child);
+    }
+
+    new SurfaceControl.Transaction()
+        .setVisibility(child, true)
+        .setBufferSize(child, width, height)
+        .apply();
+
+    return new Surface(child);
+  }
+
+  // Must be called whenever the GPU process has died. This destroys all the child SurfaceControls
+  // that have been created, meaning subsequent calls to getChildSurface() will create new ones.
+  @RequiresApi(api = Build.VERSION_CODES.Q)
+  @WrapForJNI(exceptionMode = "abort")
+  public synchronized void onGpuProcessLoss() {
+    for (final SurfaceControl child : mChildSurfaceControls.values()) {
+      // We could reparent the child SurfaceControl to null here to immediately remove it from the
+      // tree. However, this will result in a black screen while we wait for the new compositor to
+      // be created. It's preferable for the user to see the old content instead, so simply call
+      // release().
+      child.release();
+    }
+    mChildSurfaceControls.clear();
+  }
+}

mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoServiceGpuProcess.java

@@ -8,7 +8,6 @@ package org.mozilla.gecko.process;
 import android.os.Binder;
 import android.util.SparseArray;
 import android.view.Surface;
-import android.view.SurfaceControl;
 import org.mozilla.gecko.annotation.WrapForJNI;
 import org.mozilla.gecko.gfx.ICompositorSurfaceManager;
 import org.mozilla.gecko.gfx.ISurfaceAllocator;
@@ -46,16 +45,9 @@ public class GeckoServiceGpuProcess extends GeckoServiceChildProcess {
     }
 
     private final SparseArray<Surface> mSurfaces = new SparseArray<Surface>();
-    private final SparseArray<SurfaceControl> mSurfaceControls = new SparseArray<SurfaceControl>();
 
     @Override
-    public synchronized void onSurfaceChanged(
-        final int widgetId, final Surface surface, final SurfaceControl surfaceControl) {
-      if (surfaceControl != null) {
-        mSurfaceControls.put(widgetId, surfaceControl);
-      } else {
-        mSurfaceControls.remove(widgetId);
-      }
+    public synchronized void onSurfaceChanged(final int widgetId, final Surface surface) {
       if (surface != null) {
         mSurfaces.put(widgetId, surface);
       } else {
@@ -67,10 +59,5 @@ public class GeckoServiceGpuProcess extends GeckoServiceChildProcess {
     public synchronized Surface getCompositorSurface(final int widgetId) {
       return mSurfaces.get(widgetId);
     }
-
-    @WrapForJNI
-    public synchronized SurfaceControl getCompositorSurfaceControl(final int widgetId) {
-      return mSurfaceControls.get(widgetId);
-    }
   }
 }

widget/android/CompositorWidgetParent.cpp

@@ -47,13 +47,7 @@ void CompositorWidgetParent::OnCompositorSurfaceChanged() {
   java::GeckoServiceGpuProcess::RemoteCompositorSurfaceManager::LocalRef
       manager = java::GeckoServiceGpuProcess::RemoteCompositorSurfaceManager::
           GetInstance();
-  java::sdk::SurfaceControl::LocalRef surfaceControl =
-      manager->GetCompositorSurfaceControl(mWidgetId);
-  if (surfaceControl) {
-    mSurface = java::sdk::Surface::FromSurfaceControl(surfaceControl);
-  } else {
-    mSurface = manager->GetCompositorSurface(mWidgetId);
-  }
+  mSurface = manager->GetCompositorSurface(mWidgetId);
 }
 
 }  // namespace widget

widget/android/InProcessAndroidCompositorWidget.cpp

@@ -44,15 +44,8 @@ void InProcessAndroidCompositorWidget::ObserveVsync(VsyncObserver* aObserver) {
 nsIWidget* InProcessAndroidCompositorWidget::RealWidget() { return mWindow; }
 
 void InProcessAndroidCompositorWidget::OnCompositorSurfaceChanged() {
-  java::sdk::SurfaceControl::LocalRef surfaceControl =
-      java::sdk::SurfaceControl::Ref::From(static_cast<jobject>(
-          mWindow->GetNativeData(NS_JAVA_SURFACE_CONTROL)));
-  if (surfaceControl) {
-    mSurface = java::sdk::Surface::FromSurfaceControl(surfaceControl);
-  } else {
-    mSurface = java::sdk::Surface::Ref::From(
-        static_cast<jobject>(mWindow->GetNativeData(NS_JAVA_SURFACE)));
-  }
+  mSurface = java::sdk::Surface::Ref::From(
+      static_cast<jobject>(mWindow->GetNativeData(NS_JAVA_SURFACE)));
 }
 
 void InProcessAndroidCompositorWidget::NotifyClientSizeChanged(

widget/android/bindings/SurfaceTexture-classes.txt

@@ -2,4 +2,4 @@
 [android.view.Surface = exceptionMode:nsresult]
 <init>(Landroid/view/SurfaceControl;)V = stubName:FromSurfaceControl, exceptionMode:abort
 [android.view.SurfaceControl = exceptionMode:nsresult]
-[android.view.SurfaceControl$Transaction = exceptionMode:abort]
+isValid()Z = exceptionMode:abort

widget/android/moz.build

@@ -76,6 +76,7 @@ classes_with_WrapForJNI = [
     "SessionTextInput",
     "SpeechSynthesisService",
     "SurfaceAllocator",
+    "SurfaceControlManager",
     "SurfaceTextureListener",
     "TelemetryUtils",
     "WebAuthnTokenManager",

widget/android/nsWindow.cpp

@@ -88,6 +88,7 @@
 #include "mozilla/java/GeckoSystemStateListenerWrappers.h"
 #include "mozilla/java/PanZoomControllerNatives.h"
 #include "mozilla/java/SessionAccessibilityWrappers.h"
+#include "mozilla/java/SurfaceControlManagerWrappers.h"
 #include "mozilla/layers/APZEventState.h"
 #include "mozilla/layers/APZInputBridge.h"
 #include "mozilla/layers/APZThreadUtils.h"
@@ -899,6 +900,8 @@ class LayerViewSupport final
   Atomic<bool, ReleaseAcquire> mCompositorPaused;
   java::sdk::Surface::GlobalRef mSurface;
   java::sdk::SurfaceControl::GlobalRef mSurfaceControl;
+  int32_t mWidth;
+  int32_t mHeight;
   // Used to communicate with the gecko compositor from the UI thread.
   // Set in NotifyCompositorCreated and cleared in NotifyCompositorSessionLost.
   RefPtr<UiCompositorControllerChild> mUiCompositorControllerChild;
@@ -1019,19 +1022,27 @@ class LayerViewSupport final
     MOZ_ASSERT(AndroidBridge::IsJavaUiThread());
     mUiCompositorControllerChild = aUiCompositorControllerChild;
 
-    if (auto window{mWindow.Access()}) {
-      nsWindow* gkWindow = window->GetNsWindow();
-      if (gkWindow) {
-        mUiCompositorControllerChild->OnCompositorSurfaceChanged(
-            gkWindow->mWidgetId, mSurface, mSurfaceControl);
-      }
-    }
-
     if (mDefaultClearColor) {
       mUiCompositorControllerChild->SetDefaultClearColor(*mDefaultClearColor);
     }
 
     if (!mCompositorPaused) {
+      // If we are using SurfaceControl but mSurface is null, that means the
+      // previous surface was destroyed along with the the previous compositor,
+      // and we need to create a new one.
+      if (mSurfaceControl && !mSurface) {
+        mSurface = java::SurfaceControlManager::GetInstance()->GetChildSurface(
+            mSurfaceControl, mWidth, mHeight);
+      }
+
+      if (auto window{mWindow.Access()}) {
+        nsWindow* gkWindow = window->GetNsWindow();
+        if (gkWindow) {
+          mUiCompositorControllerChild->OnCompositorSurfaceChanged(
+              gkWindow->mWidgetId, mSurface);
+        }
+      }
+
       mUiCompositorControllerChild->Resume();
     }
   }
@@ -1041,6 +1052,12 @@ class LayerViewSupport final
     MOZ_ASSERT(AndroidBridge::IsJavaUiThread());
     mUiCompositorControllerChild = nullptr;
 
+    if (mSurfaceControl) {
+      // If we are using SurfaceControl then we must set the Surface to null
+      // here to ensure we create a new one when the new compositor is created.
+      mSurface = nullptr;
+    }
+
     if (auto window = mWindow.Access()) {
       while (!mCapturePixelsResults.empty()) {
         auto result =
@@ -1057,9 +1074,6 @@ class LayerViewSupport final
   }
 
   java::sdk::Surface::Param GetSurface() { return mSurface; }
-  java::sdk::SurfaceControl::Param GetSurfaceControl() {
-    return mSurfaceControl;
-  }
 
  private:
   already_AddRefed<DataSourceSurface> FlipScreenPixels(
@@ -1178,6 +1192,16 @@ class LayerViewSupport final
 
     if (mUiCompositorControllerChild) {
       mUiCompositorControllerChild->Pause();
+
+      mSurface = nullptr;
+      mSurfaceControl = nullptr;
+      if (auto window = mWindow.Access()) {
+        nsWindow* gkWindow = window->GetNsWindow();
+        if (gkWindow) {
+          mUiCompositorControllerChild->OnCompositorSurfaceChanged(
+              gkWindow->mWidgetId, nullptr);
+        }
+      }
     }
 
     if (auto lock{mWindow.Access()}) {
@@ -1210,20 +1234,19 @@ class LayerViewSupport final
       jni::Object::Param aSurfaceControl) {
     MOZ_ASSERT(AndroidBridge::IsJavaUiThread());
 
-    mSurface = java::sdk::Surface::GlobalRef::From(aSurface);
-    // Disable the SurfaceControl compositing path for now, until we have a
-    // solution to bug 1767128. This means users on Android 12 will be unable to
-    // recover from a GPU process crash (due to bug 1762025), and the parent
-    // process will crash as a result (which is no worse than not using the GPU
-    // process in the first place).
-    mSurfaceControl = nullptr;
-
+    mWidth = aWidth;
+    mHeight = aHeight;
+    mSurfaceControl =
+        java::sdk::SurfaceControl::GlobalRef::From(aSurfaceControl);
     if (mSurfaceControl) {
-      // Setting the SurfaceControl's buffer size here ensures child Surfaces
-      // created by the compositor have the correct size.
-      java::sdk::SurfaceControl::Transaction::LocalRef transaction =
-          java::sdk::SurfaceControl::Transaction::New();
-      transaction->SetBufferSize(mSurfaceControl, aWidth, aHeight)->Apply();
+      // When using SurfaceControl, we create a child Surface to render in to
+      // rather than rendering directly in to the Surface provided by the
+      // application. This allows us to work around a bug on some versions of
+      // Android when recovering from a GPU process crash.
+      mSurface = java::SurfaceControlManager::GetInstance()->GetChildSurface(
+          mSurfaceControl, mWidth, mHeight);
+    } else {
+      mSurface = java::sdk::Surface::GlobalRef::From(aSurface);
     }
 
     if (mUiCompositorControllerChild) {
@@ -1232,7 +1255,7 @@ class LayerViewSupport final
         if (gkWindow) {
           // Send new Surface to GPU process, if one exists.
           mUiCompositorControllerChild->OnCompositorSurfaceChanged(
-              gkWindow->mWidgetId, mSurface, mSurfaceControl);
+              gkWindow->mWidgetId, mSurface);
         }
       }
 
@@ -2493,13 +2516,6 @@ void* nsWindow::GetNativeData(uint32_t aDataType) {
         return lvs->GetSurface().Get();
       }
       return nullptr;
-
-    case NS_JAVA_SURFACE_CONTROL:
-      if (::mozilla::jni::NativeWeakPtr<LayerViewSupport>::Accessor lvs{
-              mLayerViewSupport.Access()}) {
-        return lvs->GetSurfaceControl().Get();
-      }
-      return nullptr;
   }
 
   return nullptr;

widget/nsIWidget.h

@@ -144,7 +144,6 @@ typedef void* nsNativeWidget;
 #endif
 #ifdef MOZ_WIDGET_ANDROID
 #  define NS_JAVA_SURFACE 100
-#  define NS_JAVA_SURFACE_CONTROL 101
 #endif
 
 #define MOZ_WIDGET_MAX_SIZE 16384