Fix use-after-free of WebContentsAccessibility.

This happens when the WebContents being destroyed. If the
InterstitialPage is showing we only clear the reference
to the native WebContentsAccessibility from InterstitialPage's
BrowserAccessibilityManager, use-after-free will be happened
when the |WebContents->GetMainFrame|'s BrowserAccessibilityManager
destroyed.

Change RawPtr of WebContentsAccessibilityAndroid to WeakPtr.

Bug:1056882

Change-Id: Id1efea84aef6c7437a178f7006bc09035949bbcb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2063789
Commit-Queue: Bo <boliu@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746202}

content/browser/accessibility/browser_accessibility_manager_android.cc

@@ -24,14 +24,14 @@ BrowserAccessibilityManager* BrowserAccessibilityManager::Create(
 
 BrowserAccessibilityManagerAndroid::BrowserAccessibilityManagerAndroid(
     const ui::AXTreeUpdate& initial_tree,
-    WebContentsAccessibilityAndroid* web_contents_accessibility,
+    base::WeakPtr<WebContentsAccessibilityAndroid> web_contents_accessibility,
     BrowserAccessibilityDelegate* delegate,
     BrowserAccessibilityFactory* factory)
     : BrowserAccessibilityManager(delegate, factory),
-      web_contents_accessibility_(web_contents_accessibility),
+      web_contents_accessibility_(std::move(web_contents_accessibility)),
       prune_tree_for_screen_reader_(true) {
-  if (web_contents_accessibility)
-    web_contents_accessibility->set_root_manager(this);
+  if (web_contents_accessibility_)
+    web_contents_accessibility_->set_root_manager(this);
   Initialize(initial_tree);
 }
 
@@ -438,7 +438,7 @@ WebContentsAccessibilityAndroid*
 BrowserAccessibilityManagerAndroid::GetWebContentsAXFromRootManager() {
   BrowserAccessibility* parent_node = GetParentNodeFromParentTree();
   if (!parent_node)
-    return web_contents_accessibility_;
+    return web_contents_accessibility_.get();
 
   auto* parent_manager =
       static_cast<BrowserAccessibilityManagerAndroid*>(parent_node->manager());

content/browser/accessibility/browser_accessibility_manager_android.h

@@ -5,6 +5,8 @@
 #ifndef CONTENT_BROWSER_ACCESSIBILITY_BROWSER_ACCESSIBILITY_MANAGER_ANDROID_H_
 #define CONTENT_BROWSER_ACCESSIBILITY_BROWSER_ACCESSIBILITY_MANAGER_ANDROID_H_
 
+#include <utility>
+
 #include "content/browser/accessibility/browser_accessibility_manager.h"
 
 namespace ui {
@@ -39,7 +41,7 @@ class CONTENT_EXPORT BrowserAccessibilityManagerAndroid
  public:
   BrowserAccessibilityManagerAndroid(
       const ui::AXTreeUpdate& initial_tree,
-      WebContentsAccessibilityAndroid* web_contents_accessibility,
+      base::WeakPtr<WebContentsAccessibilityAndroid> web_contents_accessibility,
       BrowserAccessibilityDelegate* delegate,
       BrowserAccessibilityFactory* factory = new BrowserAccessibilityFactory());
 
@@ -59,8 +61,9 @@ class CONTENT_EXPORT BrowserAccessibilityManagerAndroid
   }
   bool prune_tree_for_screen_reader() { return prune_tree_for_screen_reader_; }
 
-  void set_web_contents_accessibility(WebContentsAccessibilityAndroid* wcax) {
-    web_contents_accessibility_ = wcax;
+  void set_web_contents_accessibility(
+      base::WeakPtr<WebContentsAccessibilityAndroid> wcax) {
+    web_contents_accessibility_ = std::move(wcax);
   }
 
   bool ShouldRespectDisplayedPasswordText();
@@ -118,10 +121,10 @@ class CONTENT_EXPORT BrowserAccessibilityManagerAndroid
   // Handle a hover event from the renderer process.
   void HandleHoverEvent(BrowserAccessibility* node);
 
-  // Pointer to WebContentsAccessibility for reaching Java layer.
+  // A weak reference to WebContentsAccessibility for reaching Java layer.
   // Only the root manager has the reference. Should be accessed through
   // |GetWebContentsAXFromRootManager| rather than directly.
-  WebContentsAccessibilityAndroid* web_contents_accessibility_;
+  base::WeakPtr<WebContentsAccessibilityAndroid> web_contents_accessibility_;
 
   // See docs for set_prune_tree_for_screen_reader, above.
   bool prune_tree_for_screen_reader_;

content/browser/accessibility/web_contents_accessibility_android.cc

@@ -400,7 +400,7 @@ void WebContentsAccessibilityAndroid::Enable(JNIEnv* env,
   // web contents.
   if (manager) {
     set_root_manager(manager);
-    manager->set_web_contents_accessibility(this);
+    manager->set_web_contents_accessibility(GetWeakPtr());
     return;
   }
 
@@ -1402,6 +1402,11 @@ void WebContentsAccessibilityAndroid::CollectStats() {
   CAPABILITY_TYPE_HISTOGRAM(capabilities_mask, CAN_PERFORM_GESTURES);
 }
 
+base::WeakPtr<WebContentsAccessibilityAndroid>
+WebContentsAccessibilityAndroid::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 jlong JNI_WebContentsAccessibilityImpl_Init(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,

content/browser/accessibility/web_contents_accessibility_android.h

@@ -9,6 +9,7 @@
 
 #include "base/android/jni_weak_ref.h"
 #include "base/android/scoped_java_ref.h"
+#include "base/memory/weak_ptr.h"
 
 namespace ui {
 class MotionEventAndroid;
@@ -257,6 +258,8 @@ class CONTENT_EXPORT WebContentsAccessibilityAndroid
   void HandleHover(int32_t unique_id);
   void HandleNavigate();
 
+  base::WeakPtr<WebContentsAccessibilityAndroid> GetWeakPtr();
+
  private:
   BrowserAccessibilityAndroid* GetAXFromUniqueID(int32_t unique_id);
 
@@ -281,6 +284,8 @@ class CONTENT_EXPORT WebContentsAccessibilityAndroid
   class Connector;
   Connector* connector_;
 
+  base::WeakPtrFactory<WebContentsAccessibilityAndroid> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(WebContentsAccessibilityAndroid);
 };
 

content/browser/renderer_host/render_widget_host_view_android.cc

@@ -1265,8 +1265,7 @@ void RenderWidgetHostViewAndroid::SynchronousCopyContents(
 
 WebContentsAccessibilityAndroid*
 RenderWidgetHostViewAndroid::GetWebContentsAccessibilityAndroid() const {
-  return static_cast<WebContentsAccessibilityAndroid*>(
-      web_contents_accessibility_);
+  return web_contents_accessibility_;
 }
 
 void RenderWidgetHostViewAndroid::UpdateTouchSelectionController(
@@ -1646,9 +1645,10 @@ BrowserAccessibilityManager*
 RenderWidgetHostViewAndroid::CreateBrowserAccessibilityManager(
     BrowserAccessibilityDelegate* delegate,
     bool for_root_frame) {
+  auto* wcax = GetWebContentsAccessibilityAndroid();
   return new BrowserAccessibilityManagerAndroid(
       BrowserAccessibilityManagerAndroid::GetEmptyDocument(),
-      for_root_frame && host() ? GetWebContentsAccessibilityAndroid() : nullptr,
+      for_root_frame && host() && wcax ? wcax->GetWeakPtr() : nullptr,
       delegate);
 }
 

content/public/android/javatests/src/org/chromium/content/browser/accessibility/WebContentsAccessibilityTest.java

@@ -32,12 +32,16 @@ import org.chromium.base.test.BaseJUnit4ClassRunner;
 import org.chromium.base.test.util.MinAndroidSdkLevel;
 import org.chromium.base.test.util.RetryOnFailure;
 import org.chromium.base.test.util.UrlUtils;
+import org.chromium.content_public.browser.WebContents;
+import org.chromium.content_public.browser.WebContentsObserver;
+import org.chromium.content_public.browser.test.InterstitialPageDelegateAndroid;
 import org.chromium.content_public.browser.test.util.Criteria;
 import org.chromium.content_public.browser.test.util.CriteriaHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.content_shell_apk.ContentShellActivityTestRule;
 
 import java.lang.reflect.Method;
+import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 
 /**
@@ -74,6 +78,44 @@ public class WebContentsAccessibilityTest {
     @Rule
     public ContentShellActivityTestRule mActivityTestRule = new ContentShellActivityTestRule();
 
+    private static class TestWebContentsObserver extends WebContentsObserver {
+        private boolean mInterstitialShowing;
+
+        public TestWebContentsObserver(WebContents webContents) {
+            super(webContents);
+        }
+
+        public boolean isInterstitialShowing() throws ExecutionException {
+            return TestThreadUtils
+                    .runOnUiThreadBlocking(new Callable<Boolean>() {
+                        @Override
+                        public Boolean call() {
+                            return mInterstitialShowing;
+                        }
+                    })
+                    .booleanValue();
+        }
+
+        @Override
+        public void didAttachInterstitialPage() {
+            mInterstitialShowing = true;
+        }
+
+        @Override
+        public void didDetachInterstitialPage() {
+            mInterstitialShowing = false;
+        }
+    }
+
+    private void waitForInterstitial(final boolean shouldBeShown) {
+        CriteriaHelper.pollUiThread(Criteria.equals(shouldBeShown, new Callable<Boolean>() {
+            @Override
+            public Boolean call() {
+                return mActivityTestRule.getWebContents().isShowingInterstitialPage();
+            }
+        }));
+    }
+
     /*
      * Enable accessibility and wait until WebContentsAccessibility.getAccessibilityNodeProvider()
      * returns something not null.
@@ -823,4 +865,50 @@ public class WebContentsAccessibilityTest {
         Assert.assertTrue(result[1].left < result[2].left);
         Assert.assertTrue(result[2].left < result[3].left);
     }
+
+    /*
+     * Run this Test under asan to check if the references to the native
+     * WebContentsAccessibility had been cleared after the native
+     * WebContentsAccessibility is destroyed.
+     */
+    @Test
+    @MediumTest
+    public void testReferencesWithInterstitialPage() throws ExecutionException {
+        final String url =
+                UrlUtils.encodeHtmlDataUri("<html><head></head><body>test</body></html>");
+        mActivityTestRule.launchContentShellWithUrl(url);
+        mActivityTestRule.waitForActiveShellToBeDoneLoading();
+        enableAccessibilityAndWaitForNodeProvider();
+        final String htmlContent = "<html>"
+                + "<head>"
+                + "</head>"
+                + "<body>"
+                + "  <h1>This is a interstitial page</h1>"
+                + "</body>"
+                + "</html>";
+        final InterstitialPageDelegateAndroid delegate =
+                new InterstitialPageDelegateAndroid(htmlContent);
+        WebContentsAccessibilityTest.TestWebContentsObserver observer =
+                TestThreadUtils.runOnUiThreadBlocking(
+                        new Callable<WebContentsAccessibilityTest.TestWebContentsObserver>() {
+                            @Override
+                            public WebContentsAccessibilityTest.TestWebContentsObserver call() {
+                                delegate.showInterstitialPage(
+                                        url, mActivityTestRule.getWebContents());
+                                return new WebContentsAccessibilityTest.TestWebContentsObserver(
+                                        mActivityTestRule.getWebContents());
+                            }
+                        });
+
+        waitForInterstitial(true);
+        Assert.assertTrue("WebContentsObserver not notified of interstitial showing",
+                observer.isInterstitialShowing());
+        TestThreadUtils.runOnUiThreadBlocking(new Runnable() {
+            @Override
+            public void run() {
+                // Test passes if destory doesn't crash.
+                mActivityTestRule.getWebContents().destroy();
+            }
+        });
+    }
 }