Fix Launcher crash in ChromeOS guest mode.

chrome/browser/ui/app_list/start_page_service.cc

 // Copyright 2013 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/ui/app_list/start_page_service.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 68 matching lines @@
       state == SPEECH_RECOGNITION_IN_SPEECH;
 }
 
 }  // namespace
 
 class StartPageService::ProfileDestroyObserver
     : public content::NotificationObserver {
  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()));
   }
   ~ProfileDestroyObserver() override {}
 
  private:
   // content::NotificationObserver
   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.)

   }
 
   StartPageService* service_;  // Owner of this class.
   content::NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfileDestroyObserver);
 };
 
 class StartPageService::StartPageWebContentsDelegate
     : public content::WebContentsDelegate {
@@ skipping 552 matching lines @@
 
   // Check for a new doodle.
   content::BrowserThread::PostDelayedTask(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(&StartPageService::FetchDoodleJson,
                  weak_factory_.GetWeakPtr()),
       recheck_delay);
 }
 
 }  // namespace app_list