Bug 1702504 - Fix crash in [@ mozilla::widget::TouchResampler::NotifyFrame]. r=botond,geckoview-reviewers,m_kato

Give Observer a virtual destructor while at it.

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

widget/android/AndroidVsync.cpp

@@ -107,6 +107,7 @@ void AndroidVsync::UnregisterObserver(Observer* aObserver, ObserverType aType) {
   } else {
     impl->mRenderObservers.RemoveElement(aObserver);
   }
+  aObserver->Dispose();
   impl->UpdateObservingVsync();
 }
 

widget/android/AndroidVsync.h

@@ -35,6 +35,10 @@ class AndroidVsync final : public SupportsThreadSafeWeakPtr<AndroidVsync> {
    public:
     // Will be called on the Java UI thread.
     virtual void OnVsync(const TimeStamp& aTimeStamp) = 0;
+    // Called when the observer is unregistered, in case it wants to
+    // manage its own lifetime.
+    virtual void Dispose() {}
+    virtual ~Observer() = default;
   };
 
   // INPUT observers are called before RENDER observers.

widget/android/nsWindow.cpp

@@ -186,8 +186,7 @@ using WindowPtr = jni::NativeWeakPtr<GeckoViewSupport>;
  * it separate from GeckoViewSupport.
  */
 class NPZCSupport final
-    : public java::PanZoomController::NativeProvider::Natives<NPZCSupport>,
-      public AndroidVsync::Observer {
+    : public java::PanZoomController::NativeProvider::Natives<NPZCSupport> {
   WindowPtr mWindow;
   java::PanZoomController::NativeProvider::WeakRef mNPZC;
 
@@ -253,6 +252,38 @@ class NPZCSupport final
     bool IsUIEvent() const override { return true; }
   };
 
+  class MOZ_HEAP_CLASS Observer final : public AndroidVsync::Observer {
+   public:
+    static Observer* Create(jni::NativeWeakPtr<NPZCSupport>&& aNPZCSupport) {
+      return new Observer(std::move(aNPZCSupport));
+    }
+
+   private:
+    // Private constructor, part of a strategy to make sure
+    // we're only able to create these on the heap.
+    explicit Observer(jni::NativeWeakPtr<NPZCSupport>&& aNPZCSupport)
+        : mNPZCSupport(std::move(aNPZCSupport)) {}
+
+    void OnVsync(const TimeStamp& aTimeStamp) override {
+      auto accessor = mNPZCSupport.Access();
+
+      if (!accessor) {
+        return;
+      }
+
+      accessor->mTouchResampler.NotifyFrame(
+          aTimeStamp -
+          TimeDuration::FromMilliseconds(kTouchResampleVsyncAdjustMs));
+      accessor->ConsumeMotionEventsFromResampler();
+    }
+
+    void Dispose() override { delete this; }
+
+    jni::NativeWeakPtr<NPZCSupport> mNPZCSupport;
+  };
+
+  Observer* mObserver = nullptr;
+
   template <typename Lambda>
   void PostInputEvent(Lambda&& aLambda) {
     // Use priority queue for input events.
@@ -281,7 +312,7 @@ class NPZCSupport final
   ~NPZCSupport() {
     if (mListeningToVsync) {
       MOZ_RELEASE_ASSERT(mAndroidVsync);
-      mAndroidVsync->UnregisterObserver(this, AndroidVsync::INPUT);
+      mAndroidVsync->UnregisterObserver(mObserver, AndroidVsync::INPUT);
       mListeningToVsync = false;
     }
   }
@@ -769,19 +800,23 @@ class NPZCSupport final
   void RegisterOrUnregisterForVsync(bool aNeedVsync) {
     MOZ_RELEASE_ASSERT(mAndroidVsync);
     if (aNeedVsync && !mListeningToVsync) {
-      mAndroidVsync->RegisterObserver(this, AndroidVsync::INPUT);
+      MOZ_ASSERT(!mObserver);
+      auto win = mWindow.Access();
+      if (!win) {
+        return;
+      }
+      nsWindow* gkWindow = win->GetNsWindow();
+      jni::NativeWeakPtr<NPZCSupport> weakPtrToThis =
+          gkWindow->GetNPZCSupportWeakPtr();
+      mObserver = Observer::Create(std::move(weakPtrToThis));
+      mAndroidVsync->RegisterObserver(mObserver, AndroidVsync::INPUT);
     } else if (!aNeedVsync && mListeningToVsync) {
-      mAndroidVsync->UnregisterObserver(this, AndroidVsync::INPUT);
+      mAndroidVsync->UnregisterObserver(mObserver, AndroidVsync::INPUT);
+      mObserver = nullptr;
     }
     mListeningToVsync = aNeedVsync;
   }
 
-  void OnVsync(const TimeStamp& aTimeStamp) override {
-    mTouchResampler.NotifyFrame(aTimeStamp - TimeDuration::FromMilliseconds(
-                                                 kTouchResampleVsyncAdjustMs));
-    ConsumeMotionEventsFromResampler();
-  }
-
   void ConsumeMotionEventsFromResampler() {
     auto outgoing = mTouchResampler.ConsumeOutgoingEvents();
     while (!outgoing.empty()) {
@@ -888,7 +923,8 @@ class LayerViewSupport final
   int32_t mWidth;
   int32_t mHeight;
   // Used to communicate with the gecko compositor from the UI thread.
-  // Set in NotifyCompositorCreated and cleared in NotifyCompositorSessionLost.
+  // Set in NotifyCompositorCreated and cleared in
+  // NotifyCompositorSessionLost.
   RefPtr<UiCompositorControllerChild> mUiCompositorControllerChild;
 
   Maybe<uint32_t> mDefaultClearColor;
@@ -1013,8 +1049,8 @@ class LayerViewSupport final
 
     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.
+      // 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);
@@ -1039,7 +1075,8 @@ class LayerViewSupport final
 
     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.
+      // here to ensure we create a new one when the new compositor is
+      // created.
       mSurface = nullptr;
     }
 
@@ -1467,8 +1504,8 @@ class LayerViewSupport final
         } else {
           result->CompleteExceptionally(
               java::sdk::IllegalStateException::New(
-                  "Failed to create flipped snapshot surface (probably out of "
-                  "memory)")
+                  "Failed to create flipped snapshot surface (probably out "
+                  "of memory)")
                   .Cast<jni::Throwable>());
         }
       } else {
@@ -2927,6 +2964,10 @@ void nsWindow::UpdateSafeAreaInsets(const ScreenIntMargin& aSafeAreaInsets) {
   }
 }
 
+jni::NativeWeakPtr<NPZCSupport> nsWindow::GetNPZCSupportWeakPtr() {
+  return mNPZCSupport;
+}
+
 already_AddRefed<nsIWidget> nsIWidget::CreateTopLevelWindow() {
   nsCOMPtr<nsIWidget> window = new nsWindow();
   return window.forget();

widget/android/nsWindow.h

@@ -239,6 +239,9 @@ class nsWindow final : public nsBaseWidget {
   virtual mozilla::ScreenIntMargin GetSafeAreaInsets() const override;
   void UpdateSafeAreaInsets(const mozilla::ScreenIntMargin& aSafeAreaInsets);
 
+  mozilla::jni::NativeWeakPtr<mozilla::widget::NPZCSupport>
+  GetNPZCSupportWeakPtr();
+
  protected:
   void BringToFront();
   nsWindow* FindTopLevel();