Fix debug-only crash installing app from app launcher.

Fix debug-only crash installing app from app launcher.

Browser::ShowDownload does not try to close the web contents if there is
no corresponding tab. This avoids hitting a NOTREACHED later on which
checks for this case.

BUG=424413
Committed: https://crrev.com/51307ca7a09f7be8da0572a7eff2b8e47bdb8d78
Cr-Commit-Position: refs/heads/master@{#301023}

[Matt Giuca, 2014-10-20 00:03:51]

mgiuca@chromium.org changed reviewers:
+ sky@chromium.org

[asanka, 2014-10-20 15:10:13]

Does this also happen if a download is initiated from an iframe in a regular
window?

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

https://codereview.chromium.org/666803002/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/666803002/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1301: void
Browser::ShowDownload(content::DownloadItem* download) {
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]

> Does this also happen if a download is initiated from an
> iframe in a regular window?

No, it's not possible to trigger this from an iframe. If you left-click a
download in an iframe, IsInitialNavigation() is false. If you middle-click a
download in an iframe, it opens a new top-level tab which can then be closed as
normal.

https://codereview.chromium.org/666803002/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/666803002/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1301: void
Browser::ShowDownload(content::DownloadItem* download) {
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]

https://codereview.chromium.org/666803002/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/666803002/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1301: void
Browser::ShowDownload(content::DownloadItem* download) {
On 2014/10/21 00:22:03, Matt Giuca wrote:
> 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.

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

[asanka, 2014-10-21 15:39:59]

On 2014/10/21 03:14:08, sky wrote:
> https://codereview.chromium.org/666803002/diff/1/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
> 
>
https://codereview.chromium.org/666803002/diff/1/chrome/browser/ui/browser.cc...
> chrome/browser/ui/browser.cc:1301: void
> Browser::ShowDownload(content::DownloadItem* download) {
> On 2014/10/21 00:22:03, Matt Giuca wrote:
> > 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.
> 
> I'm wondering why we do anything here if this browser doesn't know about the
> download? Shouldn't we early out entirely?

The WebContents for a download initiated from a background page of an extension
may not have an associated browser window. So when there's no owning browser, we
pick the most recently active browser window for that profile. That way the
download shows up somewhere visible to the user as long as there's at least one
browser window open for that profile.

Depending on what the extension is doing, the user may or may not have any idea
what the download is. But the current behavior was deemed better than not
exposing the download at all.

My feeling is that this patch is correct because currently Browsers should be
prepared to show downloads from an external WebContents.

I'm not so sure whether the behavior is correct for an app. Isn't it confusing
when the user is interacting with a foreground app and the download shows up in
some other browser window?

[Matt Giuca, 2014-10-21 22:59:15]

PS. asanka: Please reply to comments using the Reitveld comment UI to keep it
all grouped together.

https://codereview.chromium.org/666803002/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/666803002/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1301: void
Browser::ShowDownload(content::DownloadItem* download) {
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'm not so sure whether the behavior is correct for an app. Isn't it
> confusing when the user is interacting with a foreground app and the
> download shows up in some other browser window?
I think it is a little bit but we don't have the proper UI for putting it in the
app launcher right now.

[Matt Giuca, 2014-10-23 23:27:01]

ping?

[sky, 2014-10-24 00:06:36]

LGTM - please add a comment as to when and why we end up in this situation for
future generations.

[Matt Giuca, 2014-10-24 00:28:06]

Done.

[Matt Giuca, 2014-10-24 00:28:20]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-10-24 00:30:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666803002/40001

[commit-bot: I haz the power, 2014-10-24 01:43:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-24 01:45:00]

Patchset 3 (id:??) landed as
https://crrev.com/51307ca7a09f7be8da0572a7eff2b8e47bdb8d78
Cr-Commit-Position: refs/heads/master@{#301023}