Fix crashes in DownloadRequestLimiter when extension popups/bubbles initiate downloads automatically

Fix crashes in DownloadRequestLimiter when extension popups/bubbles initiate downloads automatically

Automatically cancel automatic (non-user-gesture) downloads from bubbles after the first.

Merge DownloadRequestLimiterObserver into TabDownloadState. (NavigationController and WebContents are 1:1.)

TODO eventually: send a message to extensions' consoles about chrome.downloads.download() when automatic downloads are automatically cancelled.
Every WebContents continues to get one automatic download.

This patch is based on Avi's https://chromiumcodereview.appspot.com/10409031/

BUG=128368
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139554

[benjhayden, 2012-05-23 20:45:25]

I'm still looking into why second/third/etc. downloads are not marked
OnUserGesture(), so feel free to either wait for an update tomorrow or PTAL now.

[Avi (use Gerrit), 2012-05-23 20:53:28]

To the extent that I'm allowed to do so with a patch that I wrote half of, what
you have so far LGTM.

"Still better than crashing."

Indeed.

I look forward to your further discoveries re user actions.

http://codereview.chromium.org/10412061/diff/1004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10412061/diff/1004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:422: //
WebContentsDelegate::OnStartDownload() is public. Seems silly to me.
Then fix it.

[Randy Smith (Not in Mondays), 2012-05-23 21:57:53]

DownloadRequestLimiter changes LGTM; I don't know the extension_host* code.

http://codereview.chromium.org/10412061/diff/1004/chrome/browser/download/dow...
File chrome/browser/download/download_request_limiter.cc (right):

http://codereview.chromium.org/10412061/diff/1004/chrome/browser/download/dow...
chrome/browser/download/download_request_limiter.cc:89: if (!tab_wrapper) {
A sentence or two aside from the TODOs indicating what case or cases are being
handled by this test?

[benjhayden, 2012-05-24 20:43:38]

PTAL

Which suggested chrome/browser/ui owner do you think would be best?
    phajdan.jr@chromium.org
    ben@chromium.org
    kalman@chromium.org

http://codereview.chromium.org/10412061/diff/1004/chrome/browser/download/dow...
File chrome/browser/download/download_request_limiter.cc (right):

http://codereview.chromium.org/10412061/diff/1004/chrome/browser/download/dow...
chrome/browser/download/download_request_limiter.cc:89: if (!tab_wrapper) {
On 2012/05/23 21:57:54, rdsmith wrote:
> A sentence or two aside from the TODOs indicating what case or cases are being
> handled by this test?

Done.

http://codereview.chromium.org/10412061/diff/1004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10412061/diff/1004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:422: //
WebContentsDelegate::OnStartDownload() is public. Seems silly to me.
On 2012/05/23 20:53:28, Avi wrote:
> Then fix it.

Done.

[Avi (use Gerrit), 2012-05-24 20:48:44]

Re who to ask, pick one and try it.

I stopped reviewing after seeing your comment because the way you phrased things
made it sound like you made fundamental design decisions based on this
misunderstanding. And I actually do want to know where you read that; I need to
fix that broken documentation.

http://codereview.chromium.org/10412061/diff/12011/chrome/browser/download/do...
File chrome/browser/download/download_request_limiter.cc (right):

http://codereview.chromium.org/10412061/diff/12011/chrome/browser/download/do...
chrome/browser/download/download_request_limiter.cc:314: // many WebContents per
NavigationController. A NavigationController is
Whoa. Where did you read that? It hasn't been that way for years.

WebContents are 1:1 with NavControllers. In fact, if you look at
WebContentsImpl, the first sub-object is a NavControllerImpl.

[benjhayden, 2012-05-24 21:01:14]

http://codereview.chromium.org/10412061/diff/12011/chrome/browser/download/do...
File chrome/browser/download/download_request_limiter.cc (right):

http://codereview.chromium.org/10412061/diff/12011/chrome/browser/download/do...
chrome/browser/download/download_request_limiter.cc:314: // many WebContents per
NavigationController. A NavigationController is
On 2012/05/24 20:48:44, Avi wrote:
> Whoa. Where did you read that? It hasn't been that way for years.
> 
> WebContents are 1:1 with NavControllers. In fact, if you look at
> WebContentsImpl, the first sub-object is a NavControllerImpl.

http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/browser/nav...
"The NavigationController also owns all WebContents for the tab."

Now that I look at navigation_controller_impl.h, it looks like maybe I can make
TabDownloadState observe the WebContentsImpl directly.

Will a WebContentsImpl ever switch from one NavigationController to another?
Will a NavigationController ever switch from one WebContentsImpl to another?

Why are these separate classes if they're 1:1?

[Avi (use Gerrit), 2012-05-24 21:28:41]

On 2012/05/24 21:01:14, benjhayden_chromium wrote:
> "The NavigationController also owns all WebContents for the tab."

Holy moly. That's sooo wrong. Fsck!

> Will a WebContentsImpl ever switch from one NavigationController to another?
> Will a NavigationController ever switch from one WebContentsImpl to another?

No. No.

> Why are these separate classes if they're 1:1?

Historically they were different, then shuffled around. Complete accident.

[benjhayden, 2012-05-25 15:08:32]

PTAL

[Avi (use Gerrit), 2012-05-25 16:19:16]

The approach looks reasonable to me; get download and extensions people to sign
off on it.

http://codereview.chromium.org/10412061/diff/23001/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/10412061/diff/23001/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:270: // WebContents, don't
put it here!
This class is going to be renamed "TabContents" so this comment is unnecessary.

[benjhayden, 2012-05-25 18:41:10]

Aaron, Pawel, Randy: PTAL.

http://codereview.chromium.org/10412061/diff/23001/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/10412061/diff/23001/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:270: // WebContents, don't
put it here!
On 2012/05/25 16:19:16, Avi wrote:
> This class is going to be renamed "TabContents" so this comment is
unnecessary.

Done.

[Randy Smith (Not in Mondays), 2012-05-25 18:52:34]

High level comments:
* The issue description seems contradictory to offline conversations with Avi;
could you do some combination of updating it, getting him to review it, and
teaching me the error of my evaluation?  :-}

* I would also modify the issue description to be of the form:

<One line summary>

<User visible behavior change.>

<Implementation notes.>

* I believe this does result in a behavior change.  If we get a user gesture on
a page that results in a download, what used to happen is that we'd get the user
gesture forwarded to us, which would result in a GetDownloadState() which would
create the TabDownloadState(), and then we'd forward the user gesture to the
download state.  I believe this means that if we have a download initiated by a
user gesture we wouldn't use up our one free programmatic download.  However
now, (as I read the code--please check me), we won't see the initial user
gesture because we won't have a TabDownloadState until the initial CanDownload
question comes in.  So if a download is iniitated by a user gesture, we'll use
up our free programmatic download.  Do you agree, do you see an easy way to fix
it, and do you think it's a big deal?  I'm tempted to think it's not a big deal,
but that's the kind of judgement that seems to always come back and bite me, so
if you see a way to fix the behavior, that would be better :-}.  (Ditto for
finding a way to test it.)

http://codereview.chromium.org/10412061/diff/16005/chrome/browser/download/do...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

http://codereview.chromium.org/10412061/diff/16005/chrome/browser/download/do...
chrome/browser/download/download_request_limiter_unittest.cc:272: //
ALLOW_ONE_DOWNLOAD when there's no TCW?
I'm inclined to think so.  The most likely (?) situation in which I can see that
happening is that the page tries to auto-download some crap, we block it, and
then a user later clicks on the page, not knowing about the auto-download
behavior.  A malicious page could take advantage of that, but it would have to
react to the user gesture/time things right, and it would just get one download
at the cost of letting the user know something wasn't right (because their click
didn't do anything).  And I think the case where the user actually wants to
download is more likely.  So I vote in favor.

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/download/do...
File chrome/browser/download/download_request_limiter.h (right):

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/download/do...
chrome/browser/download/download_request_limiter.h:103: return
content::WebContentsObserver::web_contents();
This made me do a double take.  Maybe worth a comment?  "Promoting protected
method to public" or something like that?

[Aaron Boodman, 2012-05-25 19:27:42]

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.cc:417: Browser* browser =
browser::FindTabbedBrowser(
Any old browser?

Seems like you want the one associated with the extension host, if any. I think
there is a way to get this through extension_function_dispatcher_->delegate()?
Or maybe plumb this through base_window()?

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:268: // WARNING: There is
no TabContentsWrapper for extension popups/bubbles and
This might be too much, but...

// WARNING: Not every place where HTML can run has a 
// TabContentsWrapper. This class is *only* used in a 
// visible, actual, tab inside a browser. Examples of things
// that do not have tab wrappers include:
//
// - Extension background pages and popup bubbles
// - HTML notification bubbles
// - Screensavers on Chrome OS
// - Other random places we decide to display HTML over time
//
// Consider carefully whether your feature is something that
// makes sense only when a tab is displayed, or could make
// sense in other cases we use HTML. It may makes sense to push 
// down into WebContents and make configurable, or at least to
// make easy for other WebContents hosts to include and support.

[Avi (use Gerrit), 2012-05-25 19:49:52]

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:268: // WARNING: There is
no TabContentsWrapper for extension popups/bubbles and
I actually love this comment, though it would be better to put it higher-up in
the file.

[benjhayden, 2012-05-25 20:41:17]

PTAL

http://codereview.chromium.org/10412061/diff/16005/chrome/browser/download/do...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

http://codereview.chromium.org/10412061/diff/16005/chrome/browser/download/do...
chrome/browser/download/download_request_limiter_unittest.cc:272: //
ALLOW_ONE_DOWNLOAD when there's no TCW?
On 2012/05/25 18:52:34, rdsmith wrote:
> I'm inclined to think so.  The most likely (?) situation in which I can see
that
> happening is that the page tries to auto-download some crap, we block it, and
> then a user later clicks on the page, not knowing about the auto-download
> behavior.  A malicious page could take advantage of that, but it would have to
> react to the user gesture/time things right, and it would just get one
download
> at the cost of letting the user know something wasn't right (because their
click
> didn't do anything).  And I think the case where the user actually wants to
> download is more likely.  So I vote in favor.

Done.

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/download/do...
File chrome/browser/download/download_request_limiter.h (right):

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/download/do...
chrome/browser/download/download_request_limiter.h:103: return
content::WebContentsObserver::web_contents();
On 2012/05/25 18:52:34, rdsmith wrote:
> This made me do a double take.  Maybe worth a comment?  "Promoting protected
> method to public" or something like that?

Done.

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.cc:417: Browser* browser =
browser::FindTabbedBrowser(
On 2012/05/25 19:27:42, Aaron Boodman wrote:
> Any old browser?
> 
> Seems like you want the one associated with the extension host, if any. I
think
> there is a way to get this through extension_function_dispatcher_->delegate()?
> Or maybe plumb this through base_window()?

I just copied what AddNewContents() does.

There's a comment advising against using
GetExtensionWindowController()->GetBrowser()
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

I searched and searched and could not find this "base_window()" you mention. I
found the BaseWindow class, but not how it relates to ExtensionHost.

I'm happy to use something other than FindTabbedBrowser(), I just don't see
what.

Note also that it falls back to view()->browser() if FindTabbedBrowser() fails.
I could swap it around so that it would fall back to FindTabbedBrowser() if
view()->browser() is NULL if you want.

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:268: // WARNING: There is
no TabContentsWrapper for extension popups/bubbles and
On 2012/05/25 19:49:52, Avi wrote:
> I actually love this comment, though it would be better to put it higher-up in
> the file.

Done.

http://codereview.chromium.org/10412061/diff/18003/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:268: // WARNING: There is
no TabContentsWrapper for extension popups/bubbles and
On 2012/05/25 19:27:42, Aaron Boodman wrote:
> This might be too much, but...
> 
> // WARNING: Not every place where HTML can run has a 
> // TabContentsWrapper. This class is *only* used in a 
> // visible, actual, tab inside a browser. Examples of things
> // that do not have tab wrappers include:
> //
> // - Extension background pages and popup bubbles
> // - HTML notification bubbles
> // - Screensavers on Chrome OS
> // - Other random places we decide to display HTML over time
> //
> // Consider carefully whether your feature is something that
> // makes sense only when a tab is displayed, or could make
> // sense in other cases we use HTML. It may makes sense to push 
> // down into WebContents and make configurable, or at least to
> // make easy for other WebContents hosts to include and support.

Done.

[Avi (use Gerrit), 2012-05-25 20:44:56]

http://codereview.chromium.org/10412061/diff/20022/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/10412061/diff/20022/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:280: // least to make easy
for other WebContents hosts to include and support.
Move the comment up to be a class comment, line 76 or so.

[benjhayden, 2012-05-25 21:00:49]

http://codereview.chromium.org/10412061/diff/20022/chrome/browser/ui/tab_cont...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.h (right):

http://codereview.chromium.org/10412061/diff/20022/chrome/browser/ui/tab_cont...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:280: // least to make easy
for other WebContents hosts to include and support.
On 2012/05/25 20:44:56, Avi wrote:
> Move the comment up to be a class comment, line 76 or so.

Done.

[Randy Smith (Not in Mondays), 2012-05-25 21:31:37]

Avi: I just want to confirm that the top level CL comment that
NavigationController and WebContents are 1:1 is accurate?  I thought you
indicated that that was broken in offline email.

Beyond that, LGTM.

[Avi (use Gerrit), 2012-05-25 21:34:44]

Content lgtm.

On 2012/05/25 21:31:37, rdsmith wrote:
> Avi: I just want to confirm that the top level CL comment that
> NavigationController and WebContents are 1:1 is accurate?

It is correct. An earlier version of this CL was built on a misunderstanding of
the relationship between NavController and WC. I'm fixing that documentation:

https://chromiumcodereview.appspot.com/10448014/

[benjhayden, 2012-05-26 20:49:08]

-Pawel
+Ben Goodger

I'll ping Ben and Aaron on Monday.

[tfarina, 2012-05-26 22:46:17]

http://codereview.chromium.org/10412061/diff/16009/chrome/browser/download/do...
File chrome/browser/download/download_request_limiter.h (right):

http://codereview.chromium.org/10412061/diff/16009/chrome/browser/download/do...
chrome/browser/download/download_request_limiter.h:102: // Promote protected
accessor to public.
nit: what is this comment for? Is it a TODO?

[Randy Smith (Not in Mondays), 2012-05-27 13:28:58]

http://codereview.chromium.org/10412061/diff/16009/chrome/browser/download/do...
File chrome/browser/download/download_request_limiter.h (right):

http://codereview.chromium.org/10412061/diff/16009/chrome/browser/download/do...
chrome/browser/download/download_request_limiter.h:102: // Promote protected
accessor to public.
On 2012/05/26 22:46:18, tfarina wrote:
> nit: what is this comment for? Is it a TODO?

It's describing what the following method does--I found it confusing to have an
explicit specification of a subclass method and thought a comment on why that
was happening would be useful.  Do you disagree?  Do you see a better way to say
it?

[tfarina, 2012-05-27 13:41:40]

http://codereview.chromium.org/10412061/diff/16009/chrome/browser/download/do...
File chrome/browser/download/download_request_limiter.h (right):

http://codereview.chromium.org/10412061/diff/16009/chrome/browser/download/do...
chrome/browser/download/download_request_limiter.h:102: // Promote protected
accessor to public.
On 2012/05/27 13:28:59, rdsmith wrote:
> On 2012/05/26 22:46:18, tfarina wrote:
> > nit: what is this comment for? Is it a TODO?
> 
> It's describing what the following method does--I found it confusing to have
an
> explicit specification of a subclass method and thought a comment on why that
> was happening would be useful.  Do you disagree?  Do you see a better way to
say
> it?
I had to look at content/public/browser/web_contents_observer.h to understand
what it means. For me this comment was confused, but since that is not my code,
the decision is not mine ;). Although I'd ask, why not store a "WebContents*
contents_" as a private data member variable, instead of querying the
WebContentsObserver for this, it might not be worth though.

[Ben Goodger (Google), 2012-05-29 18:23:48]

Instead, you need to provide the type to EH that exposes the minimal set of
functionality it requires, rather than using things like browser::Find* and
passing Browser*s.

To your comment of making public interface methods public, disagree. In general,
I like to make as much private as possible. WCD is a content client interface
intended for use by content, not random other bits of the Chrome frontend.
Insofar as a feature wants to call functionality on Browser, it should do so
through a focused interface.

http://codereview.chromium.org/10412061/diff/16009/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10412061/diff/16009/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.cc:418: profile, false);  // Match
incognito exactly.
So, this pattern is effectively equivalent to BrowserList::GetLastActive() and
you shouldn't be doing this.

The ExtensionHost should (and may already) carry some state that allows proper
context to be obtained safely.

http://codereview.chromium.org/10412061/diff/16009/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10412061/diff/16009/chrome/browser/ui/browser....
chrome/browser/ui/browser.h:821: // Overridden from
content::WebContentsDelegate:
... and also, I don't like having these methods exposed publicly on Browser. I
may not be able to perpetually guarantee that Browser will always be a WCD, so I
don't like exposing these as part of Browser's public API.

[Avi (use Gerrit), 2012-05-29 18:38:12]

On 2012/05/29 18:23:48, Ben Goodger (Google) wrote:
> I don't like exposing these as part of Browser's public API.

I'll take the blame for this, suggesting that Ben do so.

[benjhayden, 2012-05-29 20:29:09]

I'll separate the contentious ExtensionHost/Browser changes out into another CL.
They aren't necessary for fixing the crashes, just the DownloadShelf showing.

[benjhayden, 2012-05-29 20:46:26]

On 2012/05/29 20:29:09, benjhayden_chromium wrote:
> I'll separate the contentious ExtensionHost/Browser changes out into another
CL.
> They aren't necessary for fixing the crashes, just the DownloadShelf showing.

Aaron is off the hook.

Ben, I still need approval for removing DownloadRequestLimiterObserver from
TabContentsWrapper.

[Ben Goodger (Google), 2012-05-29 21:02:05]

LGTM for that bit

[commit-bot: I haz the power, 2012-05-30 15:35:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10412061/11021

[commit-bot: I haz the power, 2012-05-30 17:04:32]

Change committed as 139554