Add a SupervisedUserServiceObserver.

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 468820a2ba053052760bf0d6924cd84ef1888562..6057740ab6db119879c2974be34040e1fb9beb23 100644
--- a/chrome/browser/supervised_user/supervised_user_interstitial.cc
+++ b/chrome/browser/supervised_user/supervised_user_interstitial.cc
@@ -115,7 +115,9 @@ SupervisedUserInterstitial::SupervisedUserInterstitial(
       url_(url),
       callback_(callback) {}
 
-SupervisedUserInterstitial::~SupervisedUserInterstitial() {}
+SupervisedUserInterstitial::~SupervisedUserInterstitial() {
+  DCHECK(!web_contents_);
+}
 
 bool SupervisedUserInterstitial::Init() {
   if (ShouldProceed()) {
@@ -152,24 +154,11 @@ bool SupervisedUserInterstitial::Init() {
     }
   }
 
-  // TODO(bauerb): Extract an observer callback on SupervisedUserService for
-  // this.
   Profile* profile =
       Profile::FromBrowserContext(web_contents_->GetBrowserContext());
-  PrefService* prefs = profile->GetPrefs();
-  pref_change_registrar_.Init(prefs);
-  pref_change_registrar_.Add(
-      prefs::kDefaultSupervisedUserFilteringBehavior,
-      base::Bind(&SupervisedUserInterstitial::OnFilteringPrefsChanged,
-                 base::Unretained(this)));
-  pref_change_registrar_.Add(
-      prefs::kSupervisedUserManualHosts,
-      base::Bind(&SupervisedUserInterstitial::OnFilteringPrefsChanged,
-                 base::Unretained(this)));
-  pref_change_registrar_.Add(
-      prefs::kSupervisedUserManualURLs,
-      base::Bind(&SupervisedUserInterstitial::OnFilteringPrefsChanged,
-                 base::Unretained(this)));
+  SupervisedUserService* supervised_user_service =
+      SupervisedUserServiceFactory::GetForProfile(profile);
+  supervised_user_service->AddObserver(this);
 
   interstitial_page_ =
       content::InterstitialPage::Create(web_contents_, true, url_, this);
@@ -287,7 +276,13 @@ void SupervisedUserInterstitial::OnDontProceed() {
   DispatchContinueRequest(false);
 }
 
+void SupervisedUserInterstitial::OnURLFilterChanged() {
+  if (web_contents_ && ShouldProceed())

[Bernhard Bauer, 2014/09/24 11:51:59]

Why do we need the |web_contents_| check now? Just because we need the
|web_contents_| to get the profile from it, to unregister ourselves from the
SUService?

[Marc Treib, 2014/09/24 12:02:27]

Because ShouldProceed uses the WebContents to get the Profile->SUService to
check the URLFilter. If we outlive the WebContents, that won't work. I guess we
could also cache a Profile*, but in this case it doesn't matter anyway.

+    interstitial_page_->Proceed();
+}
+
 bool SupervisedUserInterstitial::ShouldProceed() {
+  DCHECK(web_contents_);
   Profile* profile =
       Profile::FromBrowserContext(web_contents_->GetBrowserContext());
   SupervisedUserService* supervised_user_service =
@@ -298,13 +293,19 @@ bool SupervisedUserInterstitial::ShouldProceed() {
          SupervisedUserURLFilter::BLOCK;
 }
 
-void SupervisedUserInterstitial::OnFilteringPrefsChanged() {
-  if (ShouldProceed())
-    interstitial_page_->Proceed();
-}
-
 void SupervisedUserInterstitial::DispatchContinueRequest(
     bool continue_request) {
+  DCHECK(web_contents_);
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents_->GetBrowserContext());
+  SupervisedUserService* supervised_user_service =
+      SupervisedUserServiceFactory::GetForProfile(profile);
+  supervised_user_service->RemoveObserver(this);
+
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE, base::Bind(callback_, continue_request));
+
+  // After this, the WebContents may be destroyed. Make sure we don't try to use
+  // it again.
+  web_contents_ = NULL;

[Bernhard Bauer, 2014/09/24 11:51:59]

Is it guaranteed that this will be called, even if e.g. the tab/window is
closed?

Also, under what circumstances does the interstitial actually outlive the
WebContents?

[Marc Treib, 2014/09/24 12:02:27]

Yes, InterstitialPageImpl calls OnDontProceed when the tab/window is closed.
Apparently, we (almost?) always outlive the WebContents - the order of
destruction is a bit weird here. See InterstitialPageImpl::WebContentsDestroyed,
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

[Bernhard Bauer, 2014/09/24 12:58:43]

Hm, accessing |web_contents_| above already worries me... WebContentsDestroyed()
is called from the WebContentsImpl destructor, so we're calling into a partially
destroyed WebContents.

I think caching the Profile instead would be much easier, as it's guaranteed
that that will outlive us. That would also get rid of the additional
|web_contents_| check in OnURLFilterChanged().

[Marc Treib, 2014/09/24 13:25:25]

That's a good point.
Done.

 }