Adds some debugging code for tracking down a crash

chrome/browser/sessions/session_restore.cc

 // Copyright (c) 2012 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/sessions/session_restore.h"
 
 #include <algorithm>
 #include <list>
 #include <set>
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
+#include "base/debug/alias.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/metrics/histogram.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/performance_monitor/startup_timer.h"
@@ skipping 119 matching lines @@
   // Returns the RenderWidgetHost associated with a tab if there is one,
   // NULL otherwise.
   static RenderWidgetHost* GetRenderWidgetHost(NavigationController* tab);
 
   // Register for necessary notifications on a tab navigation controller.
   void RegisterForNotifications(NavigationController* controller);
 
   // Called when a tab goes away or a load completes.
   void HandleTabClosedOrLoaded(NavigationController* controller);
 
+  // TODO(sky): remove. For debugging 368236.
+  void CheckNotObserving(NavigationController* controller);
+
   content::NotificationRegistrar registrar_;
 
   // Current delay before a new tab is loaded. See class description for
   // details.
   int64 force_load_delay_;
 
   // Has Load been invoked?
   bool loading_;
 
   // Have we recorded the times for a tab paint?
@@ skipping 31 matching lines @@
 };
 
 // static
 TabLoader* TabLoader::GetTabLoader(base::TimeTicks restore_started) {
   if (!shared_tab_loader)
     shared_tab_loader = new TabLoader(restore_started);
   return shared_tab_loader;
 }
 
 void TabLoader::ScheduleLoad(NavigationController* controller) {
+  CheckNotObserving(controller);
   DCHECK(controller);
   DCHECK(find(tabs_to_load_.begin(), tabs_to_load_.end(), controller) ==
          tabs_to_load_.end());
   tabs_to_load_.push_back(controller);
   RegisterForNotifications(controller);
 }
 
 void TabLoader::TabIsLoading(NavigationController* controller) {
+  CheckNotObserving(controller);
   DCHECK(controller);
   DCHECK(find(tabs_loading_.begin(), tabs_loading_.end(), controller) ==
          tabs_loading_.end());
   tabs_loading_.insert(controller);
   RenderWidgetHost* render_widget_host = GetRenderWidgetHost(controller);
   DCHECK(render_widget_host);
   render_widget_hosts_loading_.insert(render_widget_host);
   RegisterForNotifications(controller);
 }
 
@@ skipping 249 matching lines @@
             base::TimeDelta::FromSeconds(100),
             100,
             base::Histogram::kUmaTargetedHistogramFlag);
     counter_for_count->AddTime(time_to_load);
 
     UMA_HISTOGRAM_COUNTS_100("SessionRestore.ParallelTabLoads",
                              max_parallel_tab_loads_);
   }
 }
 
+void TabLoader::CheckNotObserving(NavigationController* controller) {
+  const bool in_tabs_to_load =
+      find(tabs_to_load_.begin(), tabs_to_load_.end(), controller) !=
+          tabs_to_load_.end();

[marja, 2014/05/23 08:30:43]

Style nit, I find the shorter style more readable:

find(tabs_to_load_.begin(), tabs_to_load_.end(), controller)

-> tabs_to_load_.find(controller)

[sky, 2014/05/23 15:38:41]

I agree with you, but find isn't on list, right?

[marja, 2014/05/27 07:54:55]

Ahh, I see, I missed that it's a list.

+  const bool in_tabs_loading =
+      find(tabs_loading_.begin(), tabs_loading_.end(), controller) !=
+          tabs_loading_.end();
+  const bool observing =
+      registrar_.IsRegistered(
+          this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
+          content::Source<WebContents>(controller->GetWebContents())) ||
+      registrar_.IsRegistered(
+          this, content::NOTIFICATION_LOAD_STOP,
+          content::Source<NavigationController>(controller)) ||
+      registrar_.IsRegistered(
+          this, content::NOTIFICATION_LOAD_START,
+          content::Source<NavigationController>(controller));
+  base::debug::Alias(&in_tabs_to_load);

[marja, 2014/05/23 08:30:43]

Wouldn't it be simpler to CHECK(!foo) for all the conditions separately (from
least suspected to most suspected), so you'd see directly from the stack trace
which one fails?

(This comment is not important; feel free to ignore.)

[sky, 2014/05/23 15:38:41]

I was tempted to do as you suggest, but that would mean I wouldn't know if more
than one thing is true. This is why I went the alias route.

+  base::debug::Alias(&in_tabs_loading);
+  base::debug::Alias(&observing);
+  CHECK(!in_tabs_to_load && !in_tabs_loading && !observing);
+}
+
 // SessionRestoreImpl ---------------------------------------------------------
 
 // SessionRestoreImpl is responsible for fetching the set of tabs to create
 // from SessionService. SessionRestoreImpl deletes itself when done.
 
 class SessionRestoreImpl : public content::NotificationObserver {
  public:
   SessionRestoreImpl(Profile* profile,
                      Browser* browser,
                      chrome::HostDesktopType host_desktop_type,
@@ skipping 460 matching lines @@
         if (!restored_tab)
           continue;
 
         // If this isn't the selected tab, there's nothing else to do.
         if (is_not_selected_tab)
           continue;
 
         ShowBrowser(
             browser,
             browser->tab_strip_model()->GetIndexOfWebContents(restored_tab));
+        // TODO(sky): remove. For debugging 368236.
+        CHECK_EQ(browser->tab_strip_model()->GetActiveWebContents(),
+                 restored_tab);
         tab_loader_->TabIsLoading(&browser->tab_strip_model()
                                        ->GetActiveWebContents()
                                        ->GetController());
       }
     } else {
       // If the browser already has tabs, we want to restore the new ones after
       // the existing ones. E.g. this happens in Win8 Metro where we merge
       // windows or when launching a hosted app from the app launcher.
       int tab_index_offset = initial_tab_count;
       for (int i = 0; i < static_cast<int>(window.tabs.size()); ++i) {
@@ skipping 288 matching lines @@
   if (!active_session_restorers)
     return false;
   for (std::set<SessionRestoreImpl*>::const_iterator it =
            active_session_restorers->begin();
        it != active_session_restorers->end(); ++it) {
     if ((*it)->synchronous())
       return true;
   }
   return false;
 }