Fixing M33 and M34 stable blocker crash with InsertAppLauncherItem

chrome/browser/ui/ash/launcher/chrome_launcher_controller.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/ash/launcher/chrome_launcher_controller.h"
 
 #include <vector>
 
 #include "ash/ash_switches.h"
 #include "ash/desktop_background/desktop_background_controller.h"
@@ skipping 1703 matching lines @@
   }
 }
 
 void ChromeLauncherController::SetShelfBehaviorsFromPrefs() {
   SetShelfAutoHideBehaviorFromPrefs();
   SetShelfAlignmentFromPrefs();
 }
 
 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(
     LauncherItemController* controller,
     const std::string& app_id,
     ash::ShelfItemStatus status,
     int index,
     ash::ShelfItemType shelf_item_type) {
   ash::ShelfID id = model_->next_id();
   CHECK(!HasItemController(id));
@@ skipping 297 matching lines @@
 }
 
 void ChromeLauncherController::ReleaseProfile() {
   if (app_sync_ui_state_)
     app_sync_ui_state_->RemoveObserver(this);
 
   PrefServiceSyncable::FromProfile(profile_)->RemoveObserver(this);
 
   pref_change_registrar_.RemoveAll();
 }