Fix debug-only crash installing app from app launcher.

chrome/browser/ui/browser.cc

 // Copyright 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/ui/browser.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <shellapi.h>
 #endif  // defined(OS_WIN)
@@ skipping 1280 matching lines @@
       GlobalErrorServiceFactory::GetForProfile(profile());
   GlobalError* error = service->GetFirstGlobalErrorWithBubbleView();
   if (error)
     error->ShowBubbleView(this);
 }
 
 void Browser::ShowFirstRunBubble() {
   window()->GetLocationBar()->ShowFirstRunBubble();
 }
 
 void Browser::ShowDownload(content::DownloadItem* download) {

[sky, 2014/10/20 20:20:16]

How do we end up with calling into Browser with a WebContents Browser doesn't
know about? How owns the DownloadItem?

[Matt Giuca, 2014/10/21 00:22:03]

It's not "a WebContents Browser doesn't know about"; rather it's a WebContents
that has no tab in the current tab strip model.

WebstoreDataFetcher creates a URLFetcher and uses it to download the file. I
haven't dug too deep on this --- I don't know where the WebContents comes from,
but presumably URLFetcher is creating a WebContents (but not giving it a tab).

In any case, chrome::CloseWebContents is already applying the logic: "if the web
contents has no tab, don't try to close the tab" in Release builds (but is
DCHECK-failing in this case on Debug builds). I think in this case
(ShowDownload), it is safe to say this is a valid condition. A download does not
need to have a tab, and if it doesn't have one, we don't need to close it.

[sky, 2014/10/21 03:14:08]

I'm wondering why we do anything here if this browser doesn't know about the
download? Shouldn't we early out entirely?

[Matt Giuca, 2014/10/21 22:59:14]

Currently, even if the browser doesn't have a tab for the download, it still
opens the download shelf and shows it there.

In general, I believe this is important because otherwise the user has no way of
knowing that the download is happening, cancelling it, etc. In the specific case
of the app launcher, it's less important since we have our own progress bar
there. But we still don't have any cancelling UI so for large downloads, I think
we still want to show the download in the browser window. (But if we don't,
removing the app download from the shelf is a decision for the product managers
and should not be rolled into this bugfix CL. If we wanted to do that, it should
be using the ShouldShowInShelf mechanism, rather than by bypassing this entire
function.)

asanka:
I think it is a little bit but we don't have the proper UI for putting it in the
app launcher right now.

   if (!window())
     return;
 
   // If the download occurs in a new tab, and it's not a save page
   // download (started before initial navigation completed) close it.
   WebContents* source = download->GetWebContents();
   if (source && source->GetController().IsInitialNavigation() &&
-      tab_strip_model_->count() > 1 && !download->IsSavePackageDownload()) {
+      tab_strip_model_->count() > 1 &&
+      tab_strip_model_->GetIndexOfWebContents(source) !=
+          TabStripModel::kNoTab &&
+      !download->IsSavePackageDownload()) {
     CloseContents(source);
   }
 
   // Some (app downloads) are not supposed to appear on the shelf.
   if (!DownloadItemModel(download).ShouldShowInShelf())
     return;
 
   // GetDownloadShelf creates the download shelf if it was not yet created.
   DownloadShelf* shelf = window()->GetDownloadShelf();
   shelf->AddDownload(download);
@@ skipping 1173 matching lines @@
   if (contents && !allow_js_access) {
     contents->web_contents()->GetController().LoadURL(
         target_url,
         content::Referrer(),
         ui::PAGE_TRANSITION_LINK,
         std::string());  // No extra headers.
   }
 
   return contents != NULL;
 }