Fix shutdown order for ArCoreGl

It's illegal to destroy mojo callbacks that haven't been run while
their message pipe is still open. Since ArCoreGlThread's destructor
shuts down the task runner, this was causing DCHECK failures if
the task runner owned a closure with a pending GetFrameData
mojo callback.

To fix this, have ArCoreGl retain ownership of the closure containing
a mojo callback, and call it through a helper method.

Bug: 1065572
Change-Id: Ice2340906da470ff2b5b69554e9e7755dd040b3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2127420
Commit-Queue: Klaus Weidner <klausw@chromium.org>
Reviewed-by: Alexander Cooper <alcooper@chromium.org>
Reviewed-by: Piotr Bialecki <bialpio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754655}

chrome/browser/android/vr/arcore_device/arcore_gl.cc

@@ -115,6 +115,11 @@ ArCoreGl::~ArCoreGl() {
   DCHECK(IsOnGlThread());
   ar_image_transport_->DestroySharedBuffers(webxr_.get());
   ar_image_transport_.reset();
+
+  // Make sure mojo bindings are closed before proceeding with member
+  // destruction. Specifically, destroying pending_getframedata_
+  // must happen after closing bindings, see RunNextGetFrameData()
+  // comments.
   CloseBindingsIfOpen();
 }
 
@@ -510,11 +515,20 @@ void ArCoreGl::CopyCameraImageToFramebuffer() {
     }
     TRACE_COUNTER1("gpu", "ARCore update schedule (ms)",
                    delay.InMilliseconds());
+    // RunNextGetFrameData is needed since we must retain ownership of the mojo
+    // callback inside the pending_getframedata_ closure.
     gl_thread_task_runner_->PostDelayedTask(
-        FROM_HERE, std::move(pending_getframedata_), delay);
+        FROM_HERE, base::BindOnce(&ArCoreGl::RunNextGetFrameData, GetWeakPtr()),
+        delay);
   }
 }
 
+void ArCoreGl::RunNextGetFrameData() {
+  DVLOG(3) << __func__;
+  DCHECK(pending_getframedata_);
+  std::move(pending_getframedata_).Run();
+}
+
 void ArCoreGl::FinishFrame(int16_t frame_index) {
   TRACE_EVENT1("gpu", __func__, "frame", frame_index);
   DVLOG(3) << __func__;

chrome/browser/android/vr/arcore_device/arcore_gl.h

@@ -170,6 +170,11 @@ class ArCoreGl : public mojom::XRFrameDataProvider,
   void CopyCameraImageToFramebuffer();
   void OnTransportFrameAvailable(const gfx::Transform& uv_transform);
 
+  // Use a helper method to avoid storing the mojo getframedata callback
+  // in a closure owned by the task runner, that would lead to inconsistent
+  // state on session shutdown. See https://crbug.com/1065572.
+  void RunNextGetFrameData();
+
   base::OnceClosure session_shutdown_callback_;
 
   scoped_refptr<gl::GLSurface> surface_;
@@ -249,6 +254,9 @@ class ArCoreGl : public mojom::XRFrameDataProvider,
       this};
   mojo::Remote<device::mojom::XRPresentationClient> submit_client_;
 
+  // This closure saves arguments for the next GetFrameData call, including a
+  // mojo callback. Must remain owned by ArCoreGl, don't pass it off to the task
+  // runner directly. See RunNextGetFrameData() comments.
   base::OnceClosure pending_getframedata_;
 
   mojom::VRDisplayInfoPtr display_info_;

chrome/browser/android/vr/arcore_device/arcore_gl_thread.cc

@@ -18,9 +18,12 @@ ArCoreGlThread::ArCoreGlThread(
     : base::android::JavaHandlerThread("ArCoreGL"),
       ar_image_transport_factory_(std::move(ar_image_transport_factory)),
       mailbox_bridge_(std::move(mailbox_bridge)),
-      initialized_callback_(std::move(initialized_callback)) {}
+      initialized_callback_(std::move(initialized_callback)) {
+  DVLOG(3) << __func__;
+}
 
 ArCoreGlThread::~ArCoreGlThread() {
+  DVLOG(3) << __func__;
   Stop();
 }