Fix Launcher crash in ChromeOS guest mode.

chrome/browser/ui/app_list/start_page_service.cc

Index: chrome/browser/ui/app_list/start_page_service.cc
diff --git a/chrome/browser/ui/app_list/start_page_service.cc b/chrome/browser/ui/app_list/start_page_service.cc
index 068bdafee57d17f27955f3635b5934534854752e..9f57ccc664b754d2a0e97b869679ca18047dd274 100644
--- a/chrome/browser/ui/app_list/start_page_service.cc
+++ b/chrome/browser/ui/app_list/start_page_service.cc
@@ -86,6 +86,15 @@ class StartPageService::ProfileDestroyObserver
  public:
   explicit ProfileDestroyObserver(StartPageService* service)
       : service_(service) {
+    if (service_->profile()->IsOffTheRecord()) {
+      // We need to be notified when the original profile gets destroyed as well
+      // as the OTR profile, because the original profile will be destroyed
+      // first, and a DCHECK at that time ensures that the OTR profile has 0
+      // hosts. See http://crbug.com/463419.
+      registrar_.Add(
+          this, chrome::NOTIFICATION_PROFILE_DESTROYED,
+          content::Source<Profile>(service_->profile()->GetOriginalProfile()));

[tapted, 2015/03/04 05:01:11]

is that bug likely to get fixed? (could you just replace the registrar_.Add
below with the GetOriginalProfile() version?)

i.e. maybe there's just as much that assumes the order is as it currently
exists, and it's intentional that NOTIFICATION_PROFILE_DESTROYED goes out for
the original profile first.

[Matt Giuca, 2015/03/04 06:07:05]

I think it's definitely a bug either way, with one of two acceptable solutions:

- Swap the destruction order, or (if that doesn't work),
- Relax the DCHECK to allow services to exist on the OTR profile while the
Original profile is destroyed.

Note the asymmetry in the DCHECK at the moment: it is illegal to kill the
original profile while there are hosts on the OTR profile, but legal to kill the
OTR profile while there are hosts on the original profile. So killing the OTR
profile *first* should be fine, UNLESS there is code that explicitly assumes the
original profile is killed first (as you are asking me to do here).

We shouldn't write code that assumes the presence of the existing bug (because
what you are suggesting would break if the OTR profile was destroyed first). It
is an acceptable work-around to have the code work correctly regardless of
whether the bug exists.

[tapted, 2015/03/04 07:45:09]

Acknowledged.

+    }
     registrar_.Add(this,
                    chrome::NOTIFICATION_PROFILE_DESTROYED,
                    content::Source<Profile>(service_->profile()));
@@ -97,8 +106,10 @@ class StartPageService::ProfileDestroyObserver
   void Observe(int type,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override {
+    Profile* source_profile = content::Source<Profile>(source).ptr();
     DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type);
-    DCHECK_EQ(service_->profile(), content::Source<Profile>(source).ptr());
+    DCHECK(source_profile == service_->profile() ||

[tapted, 2015/03/04 05:01:10]

up to you, but you could also do

DCHECK(service_->profile()->IsSameProfile(content::Source<Profile>(source).ptr())));

[Matt Giuca, 2015/03/04 06:07:06]

Done. (Yeah, that's better.)

(Although I had to spend 30 minutes digging around in the implementation of
IsSameProfile to convince myself it worked.)

+           source_profile == service_->profile()->GetOriginalProfile());
     service_->Shutdown();

[tapted, 2015/03/04 05:01:11]

I doubt it really matters, but is it worth doing registrar_.RemoveAll() to avoid
a duplicate call to Shutdown()?

[Matt Giuca, 2015/03/04 06:07:05]

Done. (Although I tested that Shutdown was idempotent; it seems to be, but this
is better.)

   }