Supervised user interstitial: Don't destroy WebContents that might still be in use

chrome/browser/supervised_user/supervised_user_interstitial.cc

Index: chrome/browser/supervised_user/supervised_user_interstitial.cc
diff --git a/chrome/browser/supervised_user/supervised_user_interstitial.cc b/chrome/browser/supervised_user/supervised_user_interstitial.cc
index a87ed8cd127aed77d99c486199fbe8bc3f400ed8..73f0e7709e05300ed6c1c7e23d4cd6cd2ab7158f 100644
--- a/chrome/browser/supervised_user/supervised_user_interstitial.cc
+++ b/chrome/browser/supervised_user/supervised_user_interstitial.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/supervised_user/supervised_user_interstitial.h"
 
+#include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_number_conversions.h"
@@ -13,6 +14,8 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/supervised_user/supervised_user_service.h"
 #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/infobars/core/infobar.h"
@@ -23,6 +26,7 @@
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_ui.h"
 #include "grit/browser_resources.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -31,6 +35,8 @@
 #include "ui/base/webui/web_ui_util.h"
 
 using content::BrowserThread;
+using content::WebContents;
+using content::WebContentsObserver;
 
 namespace {
 
@@ -45,11 +51,53 @@ std::string BuildAvatarImageUrl(const std::string& url, int size) {
   return result;
 }
 
-} // namespace
+class TabCloser : WebContentsObserver {
+ public:
+  static void CloseTab(WebContents* web_contents) {
+    // TabCloser will delete itself.
+    new TabCloser(web_contents);
+  }
+
+ private:
+  explicit TabCloser(WebContents* web_contents)
+      : WebContentsObserver(web_contents), weak_ptr_factory_(this) {
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&TabCloser::CloseTabImpl, weak_ptr_factory_.GetWeakPtr()));
+  }
+  virtual ~TabCloser() {}
+
+  virtual void WebContentsDestroyed() OVERRIDE {
+    delete this;

[Bernhard Bauer, 2014/09/08 16:29:46]

If you want to tie the lifetime of this object to the lifetime of the
WebContents, you can use WebContentsUserData<>.

[Marc Treib, 2014/09/09 09:17:09]

That does seem less cumbersome, thanks! Looking...

[Marc Treib, 2014/09/09 09:30:47]

Hm, on closer inspection: Only a single instance of a given class can be
attached as UserData. We "should" not actually ever get more than one, but it
does seem fragile. I'd prefer to stay with the observer.

[Bernhard Bauer, 2014/09/09 09:41:55]

Um... if we were to attach two instances of this observer to a WebContents, we
would close it twice, wouldn't we?

[Marc Treib, 2014/09/09 09:44:05]

No, the second one would get WebContentsDestroyed and delete itself first.

[Bernhard Bauer, 2014/09/09 10:50:27]

That does not seem less fragile than the WebContentsUserData approach to me. You
said yourself that we shouldn't ever get more than one of these, but if that
should ever happen, we will silently ignore it. With WebContentsUserData, we can
actually DCHECK that there isn't one already attached to it.

[Marc Treib, 2014/09/09 11:14:37]

You're right, silently ignoring a second request to close the tab  should be
fine. Done.

+  }
+
+  void CloseTabImpl() {
+    WebContents* contents = web_contents();
+    DCHECK(contents);
+    Browser* browser = chrome::FindBrowserWithWebContents(contents);
+    DCHECK(browser);
+    TabStripModel* tab_strip = browser->tab_strip_model();
+    // Don't close the last tab in the window.
+    if (tab_strip->count() > 1) {
+      int index = tab_strip->GetIndexOfWebContents(contents);
+      DCHECK_NE(index, TabStripModel::kNoTab);

[Bernhard Bauer, 2014/09/08 16:29:46]

Switch arguments please.

[Marc Treib, 2014/09/09 09:17:09]

Done.

+      tab_strip->CloseWebContentsAt(
+          index, TabStripModel::CLOSE_CREATE_HISTORICAL_TAB);
+      // This causes WebContentsDestroyed to be called, which will delete us.
+    } else {
+      delete this;
+    }
+  }
+
+  base::WeakPtrFactory<TabCloser> weak_ptr_factory_;
+};
+
+}  // namespace
 
 // static
 void SupervisedUserInterstitial::Show(
-    content::WebContents* web_contents,
+    WebContents* web_contents,
     const GURL& url,
     const base::Callback<void(bool)>& callback) {
   SupervisedUserInterstitial* interstitial =
@@ -62,7 +110,7 @@ void SupervisedUserInterstitial::Show(
 }
 
 SupervisedUserInterstitial::SupervisedUserInterstitial(
-    content::WebContents* web_contents,
+    WebContents* web_contents,
     const GURL& url,
     const base::Callback<void(bool)>& callback)
     : web_contents_(web_contents),
@@ -257,7 +305,7 @@ void SupervisedUserInterstitial::DispatchContinueRequest(
   // If there is no history entry to go back to, close the tab instead.
   int nav_entry_count = web_contents_->GetController().GetEntryCount();
   if (!continue_request && nav_entry_count == 0)
-    web_contents_->Close();
+    TabCloser::CloseTab(web_contents_);

[Bernhard Bauer, 2014/09/08 16:29:46]

This is a bit weird. The WebContents might get closed on its own between now and
the next time the message loop runs, but we might also need it to stay around?
Could you get me that stack trace maybe, so I can take a closer look at what
happens here? If it's just about canceling the blocked requests, we could add an
additional flag here to distinguish between an explicit user action to go back,
and being canceled for another reason.

[Marc Treib, 2014/09/09 09:17:09]

"we" = this interstitial? Then I don't see why we might still need the
WebContents. We're in the process of being closed.
I've posted a stack trace + explanation on the bug. Not sure what you mean about
canceling blocked requests? All that's already done by this point.

 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE, base::Bind(callback_, continue_request));