WebUI: prevent WebContent to hold an invalid pointer.

A ConstrainedWebDialogBase creates and own a WebContent. For various
reasons, it stores a pointer to itself inside the WebContent. The
problem was that the lifetime of the WebContent may exceed the one of
ConstrainedWebDialogBase when |release_contents_on_close_| is set to
false. The pointer becomes invalid.

BUG=704327

Review-Url: https://codereview.chromium.org/2798583002
Cr-Commit-Position: refs/heads/master@{#463318}

chrome/browser/ui/cocoa/constrained_web_dialog_delegate_mac.mm

@@ -118,8 +118,8 @@ class ConstrainedWebDialogDelegateViewMac :
   void OnDialogCloseFromWebUI() override {
     return impl_->OnDialogCloseFromWebUI();
   }
-  void ReleaseWebContentsOnDialogClose() override {
-    return impl_->ReleaseWebContentsOnDialogClose();
+  std::unique_ptr<content::WebContents> ReleaseWebContents() override {
+    return impl_->ReleaseWebContents();
   }
   gfx::NativeWindow GetNativeDialog() override { return window_; }
   WebContents* GetWebContents() override { return impl_->GetWebContents(); }

chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc

@@ -175,8 +175,8 @@ class ConstrainedWebDialogDelegateViewViews
   void OnDialogCloseFromWebUI() override {
     return impl_->OnDialogCloseFromWebUI();
   }
-  void ReleaseWebContentsOnDialogClose() override {
-    return impl_->ReleaseWebContentsOnDialogClose();
+  std::unique_ptr<content::WebContents> ReleaseWebContents() override {
+    return impl_->ReleaseWebContents();
   }
   gfx::NativeWindow GetNativeDialog() override {
     return impl_->GetNativeDialog();

chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc

@@ -29,12 +29,13 @@ ConstrainedWebDialogDelegateBase::ConstrainedWebDialogDelegateBase(
     : WebDialogWebContentsDelegate(browser_context,
                                    new ChromeWebContentsHandler),
       web_dialog_delegate_(delegate),
-      closed_via_webui_(false),
-      release_contents_on_close_(false) {
+      closed_via_webui_(false) {
   CHECK(delegate);
-  web_contents_.reset(
-      WebContents::Create(WebContents::CreateParams(browser_context)));
-  zoom::ZoomController::CreateForWebContents(web_contents_.get());
+  web_contents_ =
+      WebContents::Create(WebContents::CreateParams(browser_context));
+  web_contents_holder_.reset(web_contents_);
+  WebContentsObserver::Observe(web_contents_);
+  zoom::ZoomController::CreateForWebContents(web_contents_);
   if (tab_delegate) {
     override_tab_delegate_.reset(tab_delegate);
     web_contents_->SetDelegate(tab_delegate);
@@ -44,12 +45,12 @@ ConstrainedWebDialogDelegateBase::ConstrainedWebDialogDelegateBase(
   content::RendererPreferences* prefs =
       web_contents_->GetMutableRendererPrefs();
   renderer_preferences_util::UpdateFromSystemSettings(
-      prefs, Profile::FromBrowserContext(browser_context), web_contents_.get());
+      prefs, Profile::FromBrowserContext(browser_context), web_contents_);
 
   web_contents_->GetRenderViewHost()->SyncRendererPrefs();
 
   // Set |this| as a delegate so the ConstrainedWebDialogUI can retrieve it.
-  ConstrainedWebDialogUI::SetConstrainedDelegate(web_contents_.get(), this);
+  ConstrainedWebDialogUI::SetConstrainedDelegate(web_contents_, this);
 
   web_contents_->GetController().LoadURL(delegate->GetDialogContentURL(),
                                          content::Referrer(),
@@ -58,8 +59,12 @@ ConstrainedWebDialogDelegateBase::ConstrainedWebDialogDelegateBase(
 }
 
 ConstrainedWebDialogDelegateBase::~ConstrainedWebDialogDelegateBase() {
-  if (release_contents_on_close_)
-    ignore_result(web_contents_.release());
+  if (web_contents_) {
+    // Remove reference to |this| in the WebContent since it will becomes
+    // invalid and the lifetime of the WebContent may exceed the one of this
+    // object.
+    ConstrainedWebDialogUI::ClearConstrainedDelegate(web_contents_);
+  }
 }
 
 const WebDialogDelegate*
@@ -74,15 +79,16 @@ WebDialogDelegate*
 
 void ConstrainedWebDialogDelegateBase::OnDialogCloseFromWebUI() {
   closed_via_webui_ = true;
-  CloseContents(web_contents_.get());
+  CloseContents(web_contents_);
 }
 
 bool ConstrainedWebDialogDelegateBase::closed_via_webui() const {
   return closed_via_webui_;
 }
 
-void ConstrainedWebDialogDelegateBase::ReleaseWebContentsOnDialogClose() {
-  release_contents_on_close_ = true;
+std::unique_ptr<content::WebContents>
+ConstrainedWebDialogDelegateBase::ReleaseWebContents() {
+  return std::move(web_contents_holder_);
 }
 
 gfx::NativeWindow ConstrainedWebDialogDelegateBase::GetNativeDialog() {
@@ -91,7 +97,7 @@ gfx::NativeWindow ConstrainedWebDialogDelegateBase::GetNativeDialog() {
 }
 
 WebContents* ConstrainedWebDialogDelegateBase::GetWebContents() {
-  return web_contents_.get();
+  return web_contents_;
 }
 
 void ConstrainedWebDialogDelegateBase::HandleKeyboardEvent(
@@ -114,6 +120,10 @@ gfx::Size ConstrainedWebDialogDelegateBase::GetPreferredSize() const {
   return gfx::Size();
 }
 
+void ConstrainedWebDialogDelegateBase::WebContentsDestroyed() {
+  web_contents_ = nullptr;
+}
+
 void ConstrainedWebDialogDelegateBase::ResizeToGivenSize(
     const gfx::Size size) {
   NOTREACHED();

chrome/browser/ui/webui/constrained_web_dialog_delegate_base.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "chrome/browser/ui/webui/constrained_web_dialog_ui.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "ui/web_dialogs/web_dialog_ui.h"
 #include "ui/web_dialogs/web_dialog_web_contents_delegate.h"
 
@@ -23,6 +24,7 @@ class WebDialogDelegate;
 // Platform-agnostic base implementation of ConstrainedWebDialogDelegate.
 class ConstrainedWebDialogDelegateBase
     : public ConstrainedWebDialogDelegate,
+      public content::WebContentsObserver,
       public ui::WebDialogWebContentsDelegate {
  public:
   // |browser_context| and |delegate| must outlive |this| instance, whereas
@@ -38,13 +40,16 @@ class ConstrainedWebDialogDelegateBase
   const ui::WebDialogDelegate* GetWebDialogDelegate() const override;
   ui::WebDialogDelegate* GetWebDialogDelegate() override;
   void OnDialogCloseFromWebUI() override;
-  void ReleaseWebContentsOnDialogClose() override;
+  std::unique_ptr<content::WebContents> ReleaseWebContents() override;
   content::WebContents* GetWebContents() override;
   gfx::NativeWindow GetNativeDialog() override;
   gfx::Size GetMinimumSize() const override;
   gfx::Size GetMaximumSize() const override;
   gfx::Size GetPreferredSize() const override;
 
+  // WebContentsObserver interface
+  void WebContentsDestroyed() override;
+
   // WebDialogWebContentsDelegate interface.
   void HandleKeyboardEvent(
       content::WebContents* source,
@@ -57,15 +62,15 @@ class ConstrainedWebDialogDelegateBase
   std::unique_ptr<ui::WebDialogDelegate> web_dialog_delegate_;
 
   // Holds the HTML to display in the constrained dialog.
-  std::unique_ptr<content::WebContents> web_contents_;
+  std::unique_ptr<content::WebContents> web_contents_holder_;
+
+  // Pointer to |web_contents_| that remains valid until it is destroyed.
+  content::WebContents* web_contents_;
 
   // Was the dialog closed from WebUI (in which case |web_dialog_delegate_|'s
   // OnDialogClosed() method has already been called)?
   bool closed_via_webui_;
 
-  // If true, release |web_contents_| on close instead of destroying it.
-  bool release_contents_on_close_;
-
   std::unique_ptr<WebDialogWebContentsDelegate> override_tab_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(ConstrainedWebDialogDelegateBase);

chrome/browser/ui/webui/constrained_web_dialog_ui.cc

@@ -108,6 +108,12 @@ void ConstrainedWebDialogUI::SetConstrainedDelegate(
                             new ConstrainedWebDialogDelegateUserData(delegate));
 }
 
+// static
+void ConstrainedWebDialogUI::ClearConstrainedDelegate(
+    content::WebContents* web_contents) {
+  web_contents->RemoveUserData(&kConstrainedWebDialogDelegateUserDataKey);
+}
+
 ConstrainedWebDialogDelegate* ConstrainedWebDialogUI::GetConstrainedDelegate() {
   ConstrainedWebDialogDelegateUserData* user_data =
       static_cast<ConstrainedWebDialogDelegateUserData*>(

chrome/browser/ui/webui/constrained_web_dialog_ui.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_UI_WEBUI_CONSTRAINED_WEB_DIALOG_UI_H_
 #define CHROME_BROWSER_UI_WEBUI_CONSTRAINED_WEB_DIALOG_UI_H_
 
+#include <memory>
+
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "content/public/browser/web_ui_controller.h"
@@ -32,10 +34,9 @@ class ConstrainedWebDialogDelegate {
   // message from WebUI.
   virtual void OnDialogCloseFromWebUI() = 0;
 
-  // If called, on dialog closure, the dialog will release its WebContents
-  // instead of destroying it. After which point, the caller will own the
-  // released WebContents.
-  virtual void ReleaseWebContentsOnDialogClose() = 0;
+  // If called, the dialog will release the ownership of its WebContents.
+  // The dialog will continue to use it until it is destroyed.
+  virtual std::unique_ptr<content::WebContents> ReleaseWebContents() = 0;
 
   // Returns the WebContents owned by the constrained window.
   virtual content::WebContents* GetWebContents() = 0;
@@ -74,6 +75,7 @@ class ConstrainedWebDialogUI : public content::WebUIController {
   // Sets the delegate on the WebContents.
   static void SetConstrainedDelegate(content::WebContents* web_contents,
                                      ConstrainedWebDialogDelegate* delegate);
+  static void ClearConstrainedDelegate(content::WebContents* web_contents);
 
  protected:
   // Returns the ConstrainedWebDialogDelegate saved with the WebContents.

chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc

@@ -120,9 +120,8 @@ IN_PROC_BROWSER_TEST_F(ConstrainedWebDialogBrowserTest, BasicTest) {
   EXPECT_TRUE(IsShowingWebContentsModalDialog(web_contents));
 }
 
-// Tests that ReleaseWebContentsOnDialogClose() works.
-IN_PROC_BROWSER_TEST_F(ConstrainedWebDialogBrowserTest,
-                       ReleaseWebContentsOnDialogClose) {
+// Tests that ReleaseWebContents() works.
+IN_PROC_BROWSER_TEST_F(ConstrainedWebDialogBrowserTest, ReleaseWebContents) {
   // The delegate deletes itself.
   WebDialogDelegate* delegate = new ui::test::TestWebDialogDelegate(
       GURL(chrome::kChromeUIConstrainedHTMLTestURL));
@@ -133,17 +132,18 @@ IN_PROC_BROWSER_TEST_F(ConstrainedWebDialogBrowserTest,
   ConstrainedWebDialogDelegate* dialog_delegate =
       ShowConstrainedWebDialog(browser()->profile(), delegate, web_contents);
   ASSERT_TRUE(dialog_delegate);
-  std::unique_ptr<WebContents> new_tab(dialog_delegate->GetWebContents());
-  ASSERT_TRUE(new_tab.get());
+  WebContents* dialog_contents = dialog_delegate->GetWebContents();
+  ASSERT_TRUE(dialog_contents);
   ASSERT_TRUE(IsShowingWebContentsModalDialog(web_contents));
 
-  ConstrainedWebDialogBrowserTestObserver observer(new_tab.get());
-  dialog_delegate->ReleaseWebContentsOnDialogClose();
+  ConstrainedWebDialogBrowserTestObserver observer(dialog_contents);
+  std::unique_ptr<WebContents> dialog_contents_holder =
+      dialog_delegate->ReleaseWebContents();
   dialog_delegate->OnDialogCloseFromWebUI();
 
   ASSERT_FALSE(observer.contents_destroyed());
   EXPECT_FALSE(IsShowingWebContentsModalDialog(web_contents));
-  new_tab.reset();
+  dialog_contents_holder.reset();
   EXPECT_TRUE(observer.contents_destroyed());
 }
 

chrome/browser/ui/webui/print_preview/print_preview_ui.cc

@@ -655,8 +655,11 @@ void PrintPreviewUI::OnHidePreviewDialog() {
   ConstrainedWebDialogDelegate* delegate = GetConstrainedDelegate();
   if (!delegate)
     return;
-  delegate->ReleaseWebContentsOnDialogClose();
-  background_printing_manager->OwnPrintPreviewDialog(preview_dialog);
+  std::unique_ptr<content::WebContents> preview_contents =
+      delegate->ReleaseWebContents();
+  DCHECK_EQ(preview_dialog, preview_contents.get());
+  background_printing_manager->OwnPrintPreviewDialog(
+      preview_contents.release());
   OnClosePrintPreviewDialog();
 }