Fixing M33 and M34 stable blocker crash with InsertAppLauncherItem

chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc

Index: chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
index a0756249fed4534738aab15d45f12884ce4c92c4..06f07350778d8137bb3048e82b53dcdcccf40b6c 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
@@ -1721,12 +1721,34 @@ void ChromeLauncherController::SetShelfBehaviorsFromPrefs() {
 
 WebContents* ChromeLauncherController::GetLastActiveWebContents(
     const std::string& app_id) {
-  AppIDToWebContentsListMap::const_iterator i =
+  AppIDToWebContentsListMap::iterator i =
       app_id_to_web_contents_list_.find(app_id);
   if (i == app_id_to_web_contents_list_.end())
     return NULL;
-  DCHECK_GT(i->second.size(), 0u);
-  return *i->second.begin();
+
+  // There are many crash records (crbug.com/341250) which indicate that the
+  // app_id_to_web_contents_list_ contains deleted content entries - so there
+  // must be a way that the content does not get properly updated. To fix
+  // M33 and M34 we filter out the invalid items here, but this should be
+  // addressed by a later patch correctly. Looking at the code however, the
+  // real culprit is possibly BrowserStatusMonitor::UpdateAppItemState which
+  // does not call "UpdateAppState(.., APP_STATE_REMOVED)" because due to a
+  // Browser::SwapTabContent operation it isn't able to get the browser. I
+  // think that the real patch is to call anyway when APP_STATE_REMOVED is
+  // requested, but for a backport that seems risky.
+  while (true) {
+    if (!i->second.size()) {

[James Cook, 2014/02/12 19:23:29]

I think this might be clearer if you stored i->second in a temporary variable.
Also, as it stands it looks like you might not exit the loop (even though you
will).

Maybe something like this would be clearer:

WebContentsList* list = i->second();
while (!list->empty()) {
...
}
app_id_to_web_contents_list_.erase(app_id);
return NULL;

[Mr4D (OOO till 08-26), 2014/02/12 20:39:03]

Done!

+      app_id_to_web_contents_list_.erase(app_id);
+      return NULL;
+    }
+    WebContents* contents = *i->second.begin();
+    if (chrome::FindBrowserWithWebContents(contents))
+      return contents;
+    i->second.erase(i->second.begin());
+    // This might not be necessary, but since we do not know why the lists
+    // diverged we also erase it since it cannot be correct either.
+    web_contents_to_app_id_.erase(contents);
+  }
 }
 
 ash::ShelfID ChromeLauncherController::InsertAppLauncherItem(