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

chrome/browser/supervised_user/supervised_user_interstitial.cc

 // Copyright 2014 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 "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"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #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"
 #include "components/infobars/core/infobar_delegate.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/interstitial_page.h"
 #include "content/public/browser/navigation_controller.h"
 #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"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/webui/jstemplate_builder.h"
 #include "ui/base/webui/web_ui_util.h"
 
 using content::BrowserThread;
+using content::WebContents;
+using content::WebContentsObserver;
 
 namespace {
 
 static const int kAvatarSize1x = 45;
 static const int kAvatarSize2x = 90;
 
 std::string BuildAvatarImageUrl(const std::string& url, int size) {
   std::string result = url;
   size_t slash = result.rfind('/');
   if (slash != std::string::npos)
     result.insert(slash, "/s" + base::IntToString(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 =
       new SupervisedUserInterstitial(web_contents, url, callback);
 
   // If Init() does not complete fully, immediately delete the interstitial.
   if (!interstitial->Init())
     delete interstitial;
   // Otherwise |interstitial_page_| is responsible for deleting it.
 }
 
 SupervisedUserInterstitial::SupervisedUserInterstitial(
-    content::WebContents* web_contents,
+    WebContents* web_contents,
     const GURL& url,
     const base::Callback<void(bool)>& callback)
     : web_contents_(web_contents),
       interstitial_page_(NULL),
       url_(url),
       callback_(callback) {}
 
 SupervisedUserInterstitial::~SupervisedUserInterstitial() {}
 
 bool SupervisedUserInterstitial::Init() {
@@ skipping 174 matching lines @@
 void SupervisedUserInterstitial::OnFilteringPrefsChanged() {
   if (ShouldProceed())
     interstitial_page_->Proceed();
 }
 
 void SupervisedUserInterstitial::DispatchContinueRequest(
     bool continue_request) {
   // 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));
 }