android: Recreate sync compositor on RWHVA reuse

RenderWidgetHostViews can become "inactive", and then "active" again.
This can happen if the origin of a RWHV is still kept alive by another
active RWHV, after the RWHV becomes inactive. Then the inactive RWHV
can become active again if its WebContents navigates back to the
matching origin.

RWHVA destroys SynchronousCompositorHost when it becomes inactive in
UpdateNativeViewTree. However it does not recreate it when RWHVA is
active again later, breaking rendering in Android WebView. Fix this
and also clean up by de-duplicating code to create and destroy
SynchronousCompositorHost.

Bug: 1039586
Change-Id: I7772c9d341f44f5c822f3ac186be236e5d6abf02
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1995841
Commit-Queue: Bo <boliu@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732558}

content/browser/android/synchronous_compositor_browsertest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "base/macros.h"
+#include "content/browser/android/synchronous_compositor_host.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/public/browser/android/synchronous_compositor.h"
+#include "content/public/browser/android/synchronous_compositor_client.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
+#include "content/public/test/test_utils.h"
+#include "content/shell/browser/shell.h"
+#include "content/test/content_browser_test_utils_internal.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+
+namespace content {
+
+class TestSynchronousCompositorClient : public SynchronousCompositorClient {
+ public:
+  TestSynchronousCompositorClient() = default;
+  ~TestSynchronousCompositorClient() override = default;
+
+  // SynchronousCompositorClient overrides.
+  void DidInitializeCompositor(SynchronousCompositor* compositor,
+                               const viz::FrameSinkId& id) override {
+    DCHECK(compositor_map_.count(id) == 0);
+    compositor_map_[id] = compositor;
+  }
+
+  void DidDestroyCompositor(SynchronousCompositor* compositor,
+                            const viz::FrameSinkId& id) override {
+    DCHECK(compositor_map_.count(id));
+    compositor_map_.erase(id);
+  }
+  void UpdateRootLayerState(SynchronousCompositor* compositor,
+                            const gfx::Vector2dF& total_scroll_offset,
+                            const gfx::Vector2dF& max_scroll_offset,
+                            const gfx::SizeF& scrollable_size,
+                            float page_scale_factor,
+                            float min_page_scale_factor,
+                            float max_page_scale_factor) override {}
+  void DidOverscroll(SynchronousCompositor* compositor,
+                     const gfx::Vector2dF& accumulated_overscroll,
+                     const gfx::Vector2dF& latest_overscroll_delta,
+                     const gfx::Vector2dF& current_fling_velocity) override {}
+  void PostInvalidate(SynchronousCompositor* compositor) override {}
+  void DidUpdateContent(SynchronousCompositor* compositor) override {}
+  ui::TouchHandleDrawable* CreateDrawable() override { return nullptr; }
+  void CopyOutput(
+      SynchronousCompositor* compositor,
+      std::unique_ptr<viz::CopyOutputRequest> copy_request) override {}
+  void AddBeginFrameCompletionCallback(base::OnceClosure callback) override {}
+
+  SynchronousCompositor* GetCompositor(const viz::FrameSinkId& id) {
+    auto itr = compositor_map_.find(id);
+    if (itr == compositor_map_.end())
+      return nullptr;
+    return itr->second;
+  }
+
+ private:
+  std::map<viz::FrameSinkId, SynchronousCompositor*> compositor_map_;
+  DISALLOW_COPY_AND_ASSIGN(TestSynchronousCompositorClient);
+};
+
+class SynchronousCompositorBrowserTest : public ContentBrowserTest {
+ public:
+  SynchronousCompositorBrowserTest() = default;
+
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    ContentBrowserTest::SetUpCommandLine(command_line);
+    IsolateAllSitesForTesting(command_line);
+  }
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+    SetupCrossSiteRedirector(embedded_test_server());
+    ASSERT_TRUE(embedded_test_server()->Start());
+  }
+
+  TestSynchronousCompositorClient compositor_client_;
+};
+
+IN_PROC_BROWSER_TEST_F(SynchronousCompositorBrowserTest,
+                       RenderWidgetHostViewAndroidReuse) {
+  EXPECT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL("a.com", "/title1.html")));
+
+  // Open a popup and navigate it to a.com.
+  Shell* popup = OpenPopup(
+      shell(), embedded_test_server()->GetURL("a.com", "/title2.html"), "foo");
+  WebContentsImpl* popup_contents =
+      static_cast<WebContentsImpl*>(popup->web_contents());
+  SynchronousCompositor::SetClientForWebContents(popup_contents,
+                                                 &compositor_client_);
+  RenderFrameHostImpl* rfh = popup_contents->GetMainFrame();
+  RenderViewHostImpl* rvh = rfh->render_view_host();
+  viz::FrameSinkId id = rvh->GetWidget()->GetFrameSinkId();
+  {
+    SynchronousCompositorHost* compositor =
+        static_cast<SynchronousCompositorHost*>(
+            compositor_client_.GetCompositor(id));
+    EXPECT_TRUE(compositor);
+    EXPECT_TRUE(compositor->GetSynchronousCompositor());
+  }
+
+  // Navigate popup to b.com.  Because there's an opener, the RVH for a.com
+  // stays around in the inactive state.
+  EXPECT_TRUE(NavigateToURLInSameBrowsingInstance(
+      popup, embedded_test_server()->GetURL("b.com", "/title3.html")));
+  EXPECT_FALSE(rvh->is_active());
+  EXPECT_FALSE(compositor_client_.GetCompositor(id));
+
+  // Go back to a.com. This should make the rvh active again and reinitialize
+  // SynchronousCompositor.
+  TestNavigationObserver back_observer(popup_contents);
+  popup_contents->GetController().GoBack();
+  back_observer.Wait();
+  EXPECT_TRUE(rvh->is_active());
+  {
+    SynchronousCompositorHost* compositor =
+        static_cast<SynchronousCompositorHost*>(
+            compositor_client_.GetCompositor(id));
+    EXPECT_TRUE(compositor);
+    EXPECT_TRUE(compositor->GetSynchronousCompositor());
+  }
+}
+
+}  // namespace content

content/browser/android/synchronous_compositor_host.h

@@ -10,6 +10,7 @@
 
 #include <memory>
 
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
@@ -17,6 +18,7 @@
 #include "components/viz/common/frame_sinks/begin_frame_source.h"
 #include "components/viz/common/quads/compositor_frame.h"
 #include "components/viz/common/surfaces/frame_sink_id.h"
+#include "content/common/content_export.h"
 #include "content/common/input/synchronous_compositor.mojom.h"
 #include "content/public/browser/android/synchronous_compositor.h"
 #include "content/public/common/input_event_ack_state.h"
@@ -38,7 +40,8 @@ class SynchronousCompositorClient;
 class SynchronousCompositorSyncCallBridge;
 struct SyncCompositorCommonRendererParams;
 
-class SynchronousCompositorHost : public SynchronousCompositor,
+class CONTENT_EXPORT SynchronousCompositorHost
+    : public SynchronousCompositor,
       public mojom::SynchronousCompositorHost,
       public viz::BeginFrameObserver {
  public:
@@ -107,6 +110,8 @@ class SynchronousCompositorHost : public SynchronousCompositor,
   struct SharedMemoryWithSize;
   friend class ScopedSetZeroMemory;
   friend class SynchronousCompositorBase;
+  FRIEND_TEST_ALL_PREFIXES(SynchronousCompositorBrowserTest,
+                           RenderWidgetHostViewAndroidReuse);
 
   SynchronousCompositorHost(RenderWidgetHostViewAndroid* rwhva,
                             const viz::FrameSinkId& frame_sink_id,

content/browser/renderer_host/render_widget_host_view_android.cc

@@ -1024,7 +1024,7 @@ void RenderWidgetHostViewAndroid::OnInterstitialPageAttached() {
 }
 
 void RenderWidgetHostViewAndroid::OnInterstitialPageGoingAway() {
-  sync_compositor_.reset();
+  ResetSynchronousCompositor();
 }
 
 std::unique_ptr<SyntheticGestureTarget>
@@ -1106,10 +1106,23 @@ void RenderWidgetHostViewAndroid::FrameTokenChangedForSynchronousCompositor(
 void RenderWidgetHostViewAndroid::SetSynchronousCompositorClient(
       SynchronousCompositorClient* client) {
   synchronous_compositor_client_ = client;
+  MaybeCreateSynchronousCompositor();
+}
+
+void RenderWidgetHostViewAndroid::MaybeCreateSynchronousCompositor() {
   if (!sync_compositor_ && synchronous_compositor_client_) {
     sync_compositor_ =
         SynchronousCompositorHost::Create(this, host()->GetFrameSinkId());
     view_.SetCopyOutputCallback(sync_compositor_->GetCopyViewCallback());
+    if (render_widget_initialized_)
+      sync_compositor_->InitMojo();
+  }
+}
+
+void RenderWidgetHostViewAndroid::ResetSynchronousCompositor() {
+  if (sync_compositor_) {
+    view_.SetCopyOutputCallback(ui::ViewAndroid::CopyViewCallback());
+    sync_compositor_.reset();
   }
 }
 
@@ -1900,9 +1913,13 @@ void RenderWidgetHostViewAndroid::UpdateNativeViewTree(
   }
 
   if (!has_view_tree) {
-    sync_compositor_.reset();
+    ResetSynchronousCompositor();
     return;
   }
+  // Parent native view can become null and then later non-null again, if
+  // WebContents swaps away from this, and then later back to it. Need to
+  // ensure SynchronousCompositor is recreated in this case.
+  MaybeCreateSynchronousCompositor();
 
   if (is_showing_ && view_.GetWindowAndroid())
     StartObservingRootWindow();
@@ -1950,6 +1967,7 @@ RenderWidgetHostViewAndroid::GetLocalSurfaceIdAllocation() const {
 }
 
 void RenderWidgetHostViewAndroid::OnRenderWidgetInit() {
+  render_widget_initialized_ = true;
   if (sync_compositor_)
     sync_compositor_->InitMojo();
 }

content/browser/renderer_host/render_widget_host_view_android.h

@@ -413,6 +413,9 @@ class CONTENT_EXPORT RenderWidgetHostViewAndroid
       const gfx::Size& dst_size_in_pixel,
       base::OnceCallback<void(const SkBitmap&)> callback);
 
+  void MaybeCreateSynchronousCompositor();
+  void ResetSynchronousCompositor();
+
   void EvictDelegatedContent();
   void OnLostResources();
 
@@ -531,6 +534,8 @@ class CONTENT_EXPORT RenderWidgetHostViewAndroid
   // If true, then the next allocated surface should be embedded.
   bool navigation_while_hidden_ = false;
 
+  bool render_widget_initialized_ = false;
+
   // Tracks whether we are in SynchronousCopyContents to avoid repeated calls
   // into DevTools capture logic.
   // TODO(ericrk): Make this more robust.

content/test/BUILD.gn

@@ -829,6 +829,7 @@ test("content_browsertests") {
     "../browser/accessibility/site_per_process_accessibility_browsertest.cc",
     "../browser/accessibility/snapshot_ax_tree_browsertest.cc",
     "../browser/accessibility/touch_accessibility_aura_browsertest.cc",
+    "../browser/android/synchronous_compositor_browsertest.cc",
     "../browser/appcache/appcache_browsertest.cc",
     "../browser/back_forward_cache_browsertest.cc",
     "../browser/background_sync/background_sync_base_browsertest.cc",