Don't prompt to save malicious downloads on exit

On browser close, the browser checks to see if there are any pending downloads.

Currently: If there are any pending downloads, it halts the download and asks the user to keep or save the download.

New behavior: The user won't be prompted for pending downloads that are malicious (DANGEROUS_HOST, DANGEROUS_URL, DANGEROUS_CONTENT). Instead, they'll just be automatically cleaned up on exit. Note that if a user is prompted for *other* downloads, they may still be taken to the chrome://downloads page and see the malicious download while they're there.
Added a new test in chrome/browser/lifetime/browser_close_manager_browsertest.cc.

BUG=306230, 305387
TEST=
1. Download a malicious file (e.g., http://download.safebrowsingtest.com/download/test)
2. Quit the browser
3. The browser should close without a warning
4. Download a dangerous file like a SWF
5. Quit the browser
6. The browser should pop up a dialog asking you whether you should save the file

R=asanka@chromium.org, benjhayden@chromium.org, joi@chromium.org, pkasting@chromium.org, thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229784

[felt, 2013-10-11 02:44:42]

Hi Asanka,

Can you please review this CL?

Adrienne

[asanka, 2013-10-11 16:02:35]

I'm guessing it's intentional that file type based dangerous downloads will also
be discarded?

As an aside, we should keep an eye on the Download.Discard UMA to see if there's
a significant uptick after this change.

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/download/d...
File chrome/browser/download/download_service.h (right):

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/download/d...
chrome/browser/download/download_service.h:48: int DownloadCount() const;
Are DownloadCount() and DownloadCountAllProfiles() used anywhere after this
change?

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:62: // If there are any
downloads active, all browsers are not closeable.
Nit: Mention that dangerous downloads don't block browser closing.

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/lifetime/b...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/lifetime/b...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:634:
content::DownloadTestObserverInterrupted observer(
You should be able to use DownloadTestObserverInProgress(..,
ON_DANGEROUS_DOWNLOAD_IGNORE) to achieve the same result.

File type based dangerous downloads are marked as such when filename
determination is done, which is when the DownloadTestObserverInProgress quits
waiting for it. This way you don't have to introduce ON_DANGEROUS_DOWNLOAD_QUIT.

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_close_browsertest.cc (right):

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
chrome/browser/ui/browser_close_browsertest.cc:282: << ": " <<
check_case.DebugString();
Minor nit: Adding a SCOPED_TRACE at the top of the function block would be
better than generating these debug strings everywhere. It could look like this:

SCOPED_TRACE(testing::Message() << "Case " << i
                                << ": " << check_case.DebugString())

See
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

https://codereview.chromium.org/26938003/diff/16001/content/public/browser/do...
File content/public/browser/download_manager.h (right):

https://codereview.chromium.org/26938003/diff/16001/content/public/browser/do...
content/public/browser/download_manager.h:159: virtual int InProgressCount()
const = 0;
Is InProgressCount() used anywhere? And if so, can those uses switch over to
NonDangerousInProgressCount() ?

[felt, 2013-10-11 17:26:43]

As far as the DANGEROUS_FILE ones go... I am waffling on that. I can see an
argument either way and have gone back and forth on the implementation. What do
you think?

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/download/d...
File chrome/browser/download/download_service.h (right):

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/download/d...
chrome/browser/download/download_service.h:48: int DownloadCount() const;
On 2013/10/11 16:02:35, asanka wrote:
> Are DownloadCount() and DownloadCountAllProfiles() used anywhere after this
> change?

They seemed like potentially useful utility functions, but I suppose if someone
actually needs one they can add it back in. Removed.

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:62: // If there are any
downloads active, all browsers are not closeable.
On 2013/10/11 16:02:35, asanka wrote:
> Nit: Mention that dangerous downloads don't block browser closing.

Done.

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_close_browsertest.cc (right):

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
chrome/browser/ui/browser_close_browsertest.cc:282: << ": " <<
check_case.DebugString();
I assume you mean for the whole file, not just for the few lines I touched?
Newest patchset refactors the whole test.

On 2013/10/11 16:02:35, asanka wrote:
> Minor nit: Adding a SCOPED_TRACE at the top of the function block would be
> better than generating these debug strings everywhere. It could look like
this:
> 
> SCOPED_TRACE(testing::Message() << "Case " << i
>                                 << ": " << check_case.DebugString())
> 
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

https://codereview.chromium.org/26938003/diff/16001/content/public/browser/do...
File content/public/browser/download_manager.h (right):

https://codereview.chromium.org/26938003/diff/16001/content/public/browser/do...
content/public/browser/download_manager.h:159: virtual int InProgressCount()
const = 0;
On 2013/10/11 16:02:35, asanka wrote:
> Is InProgressCount() used anywhere? And if so, can those uses switch over to
> NonDangerousInProgressCount() ?

It's used in several browser tests, and I think those browser tests actually
want the number of all downloads in progress.

[felt, 2013-10-11 17:39:37]

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/lifetime/b...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/lifetime/b...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:634:
content::DownloadTestObserverInterrupted observer(
On 2013/10/11 16:02:35, asanka wrote:
> You should be able to use DownloadTestObserverInProgress(..,
> ON_DANGEROUS_DOWNLOAD_IGNORE) to achieve the same result.
> 
> File type based dangerous downloads are marked as such when filename
> determination is done, which is when the DownloadTestObserverInProgress quits
> waiting for it. This way you don't have to introduce
ON_DANGEROUS_DOWNLOAD_QUIT.
> 

Missed this comment before. I tried this first, and it didn't work: the download
completes without ever being marked dangerous.

[asanka, 2013-10-11 19:09:41]

Regarding DANGEROUS_FILE: Our signal is currently is that the file *could* be
dangerous. At the moment this isn't a strong signal as evidenced by the
click-through rate. Unfortunately when we mark a download as being
DANGEROUS_FILE, it is likely safe and wanted. I'd be inclined to count them for
the in-progress download warning.

Unfortunately, this will complicate this CL a bit, since there's no *is
malicious* flag exposed from DownloadItem. DownloadManager could call
DownloadItem::GetDangerType() and count based on whether the returned danger
type is malicious.

Another option is to implement the counting in DownloadService itself. It could
call DownloadManager::GetAllDownloads() and then count the DownloadItems for
which DownloadItemModel::IsMalicious() is true. Let me know if you want to go
this route.

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_close_browsertest.cc (right):

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
chrome/browser/ui/browser_close_browsertest.cc:282: << ": " <<
check_case.DebugString();
On 2013/10/11 17:26:44, felt wrote:
> I assume you mean for the whole file, not just for the few lines I touched?
> Newest patchset refactors the whole test.

Yup. However, my suggestion seems to have been misunderstood :)

The SCOPED_TRACE() is only needed at the top of the function block. So you can
avoid all the message generation everywhere else in the method. In other words:

bool ExecuteDownloadCloseCheckCase(size_t i) {
  SCOPED_TRACE(testing::Message() << "Case " << ... );

  ...
  EXPECT_EQ(first_profile_, ...);
  // Note: no message generation here.
  ...
  EXPECT_EQ(0, total_download_count);
  // Or here...
  ...
  ...
  EXPECT_...;
  // Or here.
  ...
}

If any of EXPECT_foo within the scope of the SCOPED_TRACE() fails, then it will
log the message. It won't log anything if all the expectations succeed.

> On 2013/10/11 16:02:35, asanka wrote:
> > Minor nit: Adding a SCOPED_TRACE at the top of the function block would be
> > better than generating these debug strings everywhere. It could look like
> this:
> > 
> > SCOPED_TRACE(testing::Message() << "Case " << i
> >                                 << ": " << check_case.DebugString())
> > 
> > See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...
>

[felt, 2013-10-11 21:12:19]

I have switched the if-statement to exclude DANGEROUS_FILE, but now I am not
sure how to test. I can't figure out how to force any other kind of dangerous
state. Do you know how to do this?

On 2013/10/11 19:09:41, asanka wrote:
> Regarding DANGEROUS_FILE: Our signal is currently is that the file *could* be
> dangerous. At the moment this isn't a strong signal as evidenced by the
> click-through rate. Unfortunately when we mark a download as being
> DANGEROUS_FILE, it is likely safe and wanted. I'd be inclined to count them
for
> the in-progress download warning.
> 
> Unfortunately, this will complicate this CL a bit, since there's no *is
> malicious* flag exposed from DownloadItem. DownloadManager could call
> DownloadItem::GetDangerType() and count based on whether the returned danger
> type is malicious.
> 
> Another option is to implement the counting in DownloadService itself. It
could
> call DownloadManager::GetAllDownloads() and then count the DownloadItems for
> which DownloadItemModel::IsMalicious() is true. Let me know if you want to go
> this route.
> 
>
https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
> File chrome/browser/ui/browser_close_browsertest.cc (right):
> 
>
https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
> chrome/browser/ui/browser_close_browsertest.cc:282: << ": " <<
> check_case.DebugString();
> On 2013/10/11 17:26:44, felt wrote:
> > I assume you mean for the whole file, not just for the few lines I touched?
> > Newest patchset refactors the whole test.
> 
> Yup. However, my suggestion seems to have been misunderstood :)
> 
> The SCOPED_TRACE() is only needed at the top of the function block. So you can
> avoid all the message generation everywhere else in the method. In other
words:
> 
> bool ExecuteDownloadCloseCheckCase(size_t i) {
>   SCOPED_TRACE(testing::Message() << "Case " << ... );
> 
>   ...
>   EXPECT_EQ(first_profile_, ...);
>   // Note: no message generation here.
>   ...
>   EXPECT_EQ(0, total_download_count);
>   // Or here...
>   ...
>   ...
>   EXPECT_...;
>   // Or here.
>   ...
> }
> 
> If any of EXPECT_foo within the scope of the SCOPED_TRACE() fails, then it
will
> log the message. It won't log anything if all the expectations succeed.
> 
> > On 2013/10/11 16:02:35, asanka wrote:
> > > Minor nit: Adding a SCOPED_TRACE at the top of the function block would be
> > > better than generating these debug strings everywhere. It could look like
> > this:
> > > 
> > > SCOPED_TRACE(testing::Message() << "Case " << i
> > >                                 << ": " << check_case.DebugString())
> > > 
> > > See
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...
> >

[asanka, 2013-10-11 22:18:30]

On 2013/10/11 21:12:19, felt wrote:
> I have switched the if-statement to exclude DANGEROUS_FILE, but now I am not
> sure how to test. I can't figure out how to force any other kind of dangerous
> state. Do you know how to do this?

1. Create a stalled download (BrowserCloseTest::CreateStalledDownloads(...))
2. Call DownloadManager::GetAllDownloads() to get all the DownloadItems. There
should only be 1.
3. Call DownloadItem::OnAllDataSaved("") to make the download item think all the
data has been received.
4. Call DownloadItem::OnContentCheckCompleted(DOWNLOAD_DANGER_TYPE_...) to set
the danger type.

> On 2013/10/11 19:09:41, asanka wrote:
> > Regarding DANGEROUS_FILE: Our signal is currently is that the file *could*
be
> > dangerous. At the moment this isn't a strong signal as evidenced by the
> > click-through rate. Unfortunately when we mark a download as being
> > DANGEROUS_FILE, it is likely safe and wanted. I'd be inclined to count them
> for
> > the in-progress download warning.
> > 
> > Unfortunately, this will complicate this CL a bit, since there's no *is
> > malicious* flag exposed from DownloadItem. DownloadManager could call
> > DownloadItem::GetDangerType() and count based on whether the returned danger
> > type is malicious.
> > 
> > Another option is to implement the counting in DownloadService itself. It
> could
> > call DownloadManager::GetAllDownloads() and then count the DownloadItems for
> > which DownloadItemModel::IsMalicious() is true. Let me know if you want to
go
> > this route.
> > 
> >
>
https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
> > File chrome/browser/ui/browser_close_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
> > chrome/browser/ui/browser_close_browsertest.cc:282: << ": " <<
> > check_case.DebugString();
> > On 2013/10/11 17:26:44, felt wrote:
> > > I assume you mean for the whole file, not just for the few lines I
touched?
> > > Newest patchset refactors the whole test.
> > 
> > Yup. However, my suggestion seems to have been misunderstood :)
> > 
> > The SCOPED_TRACE() is only needed at the top of the function block. So you
can
> > avoid all the message generation everywhere else in the method. In other
> words:
> > 
> > bool ExecuteDownloadCloseCheckCase(size_t i) {
> >   SCOPED_TRACE(testing::Message() << "Case " << ... );
> > 
> >   ...
> >   EXPECT_EQ(first_profile_, ...);
> >   // Note: no message generation here.
> >   ...
> >   EXPECT_EQ(0, total_download_count);
> >   // Or here...
> >   ...
> >   ...
> >   EXPECT_...;
> >   // Or here.
> >   ...
> > }
> > 
> > If any of EXPECT_foo within the scope of the SCOPED_TRACE() fails, then it
> will
> > log the message. It won't log anything if all the expectations succeed.
> > 
> > > On 2013/10/11 16:02:35, asanka wrote:
> > > > Minor nit: Adding a SCOPED_TRACE at the top of the function block would
be
> > > > better than generating these debug strings everywhere. It could look
like
> > > this:
> > > > 
> > > > SCOPED_TRACE(testing::Message() << "Case " << i
> > > >                                 << ": " << check_case.DebugString())
> > > > 
> > > > See
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...
> > >

[felt, 2013-10-11 23:40:54]

On 2013/10/11 22:18:30, asanka wrote:
> On 2013/10/11 21:12:19, felt wrote:
> > I have switched the if-statement to exclude DANGEROUS_FILE, but now I am not
> > sure how to test. I can't figure out how to force any other kind of
dangerous
> > state. Do you know how to do this?
> 
> 1. Create a stalled download (BrowserCloseTest::CreateStalledDownloads(...))
> 2. Call DownloadManager::GetAllDownloads() to get all the DownloadItems. There
> should only be 1.
> 3. Call DownloadItem::OnAllDataSaved("") to make the download item think all
the
> data has been received.
> 4. Call DownloadItem::OnContentCheckCompleted(DOWNLOAD_DANGER_TYPE_...) to set
> the danger type.

This didn't work unfortunately -- "faking" the danger type causes some problems
with the download animation state, which causes a segfault in Linux.

Instead, here's what I did: the test still downloads a SWF (so we get the right
animation / UI set up). Then I change the type to be DANGER_TYPE_URL.

Note that I had to add SetDangerType to DownloadItem.h to do this. (Or I could
have added OnAllDataSaved to do steps 3 and 4 as you suggested; either way
required adding a method to DownloadItem.h, but the SetDangerType version
invokes less extra code.)

> 
> > On 2013/10/11 19:09:41, asanka wrote:
> > > Regarding DANGEROUS_FILE: Our signal is currently is that the file *could*
> be
> > > dangerous. At the moment this isn't a strong signal as evidenced by the
> > > click-through rate. Unfortunately when we mark a download as being
> > > DANGEROUS_FILE, it is likely safe and wanted. I'd be inclined to count
them
> > for
> > > the in-progress download warning.
> > > 
> > > Unfortunately, this will complicate this CL a bit, since there's no *is
> > > malicious* flag exposed from DownloadItem. DownloadManager could call
> > > DownloadItem::GetDangerType() and count based on whether the returned
danger
> > > type is malicious.
> > > 
> > > Another option is to implement the counting in DownloadService itself. It
> > could
> > > call DownloadManager::GetAllDownloads() and then count the DownloadItems
for
> > > which DownloadItemModel::IsMalicious() is true. Let me know if you want to
> go
> > > this route.
> > > 
> > >
> >
>
https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
> > > File chrome/browser/ui/browser_close_browsertest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
> > > chrome/browser/ui/browser_close_browsertest.cc:282: << ": " <<
> > > check_case.DebugString();
> > > On 2013/10/11 17:26:44, felt wrote:
> > > > I assume you mean for the whole file, not just for the few lines I
> touched?
> > > > Newest patchset refactors the whole test.
> > > 
> > > Yup. However, my suggestion seems to have been misunderstood :)
> > > 
> > > The SCOPED_TRACE() is only needed at the top of the function block. So you
> can
> > > avoid all the message generation everywhere else in the method. In other
> > words:
> > > 
> > > bool ExecuteDownloadCloseCheckCase(size_t i) {
> > >   SCOPED_TRACE(testing::Message() << "Case " << ... );
> > > 
> > >   ...
> > >   EXPECT_EQ(first_profile_, ...);
> > >   // Note: no message generation here.
> > >   ...
> > >   EXPECT_EQ(0, total_download_count);
> > >   // Or here...
> > >   ...
> > >   ...
> > >   EXPECT_...;
> > >   // Or here.
> > >   ...
> > > }
> > > 
> > > If any of EXPECT_foo within the scope of the SCOPED_TRACE() fails, then it
> > will
> > > log the message. It won't log anything if all the expectations succeed.
> > > 
> > > > On 2013/10/11 16:02:35, asanka wrote:
> > > > > Minor nit: Adding a SCOPED_TRACE at the top of the function block
would
> be
> > > > > better than generating these debug strings everywhere. It could look
> like
> > > > this:
> > > > > 
> > > > > SCOPED_TRACE(testing::Message() << "Case " << i
> > > > >                                 << ": " << check_case.DebugString())
> > > > > 
> > > > > See
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...
> > > >

[felt, 2013-10-11 23:41:09]

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_close_browsertest.cc (right):

https://codereview.chromium.org/26938003/diff/16001/chrome/browser/ui/browser...
chrome/browser/ui/browser_close_browsertest.cc:282: << ": " <<
check_case.DebugString();
On 2013/10/11 19:09:42, asanka wrote:
> On 2013/10/11 17:26:44, felt wrote:
> > I assume you mean for the whole file, not just for the few lines I touched?
> > Newest patchset refactors the whole test.
> 
> Yup. However, my suggestion seems to have been misunderstood :)
> 
> The SCOPED_TRACE() is only needed at the top of the function block. So you can
> avoid all the message generation everywhere else in the method. In other
words:
> 
> bool ExecuteDownloadCloseCheckCase(size_t i) {
>   SCOPED_TRACE(testing::Message() << "Case " << ... );
> 
>   ...
>   EXPECT_EQ(first_profile_, ...);
>   // Note: no message generation here.
>   ...
>   EXPECT_EQ(0, total_download_count);
>   // Or here...
>   ...
>   ...
>   EXPECT_...;
>   // Or here.
>   ...
> }
> 
> If any of EXPECT_foo within the scope of the SCOPED_TRACE() fails, then it
will
> log the message. It won't log anything if all the expectations succeed.
> 
> > On 2013/10/11 16:02:35, asanka wrote:
> > > Minor nit: Adding a SCOPED_TRACE at the top of the function block would be
> > > better than generating these debug strings everywhere. It could look like
> > this:
> > > 
> > > SCOPED_TRACE(testing::Message() << "Case " << i
> > >                                 << ": " << check_case.DebugString())
> > > 
> > > See
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...
> > 
> 

Done.

[felt, 2013-10-15 17:53:56]

thakis@, benjhayden@, pkasting@, and joi@ could you please review the following
files?

thakis - chrome/browser/app_controller_mac.mm, chrome/browser/lifetime/*

benjhayden -
chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc	

pkasting - chrome/browser/ui/*

joi - content/public/browser/*, content/public/test/*

[Nico, 2013-10-15 18:22:18]

my two files lgtm

[Jói, 2013-10-15 19:00:44]

LGTM for content/public with a nit.

https://codereview.chromium.org/26938003/diff/49001/content/public/browser/do...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/26938003/diff/49001/content/public/browser/do...
content/public/browser/download_item.h:325: virtual std::string DebugString(bool
verbose) const = 0;
Are these two methods only used by tests? Could they be renamed to end in
ForTesting?

[benjhayden, 2013-10-15 19:03:02]

downloads_api_browsertest.cc lgtm

[Peter Kasting, 2013-10-15 19:10:27]

c/b/ui LGTM, but this is a very risky change.  I left some comments on the bug. 
Additionally, you should gather metrics on how often this triggers, and ideally
how often people retry downloads canceled this way.

Please also clear this with the UI leads if you have not already done so.

https://codereview.chromium.org/26938003/diff/49001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_close_browsertest.cc (right):

https://codereview.chromium.org/26938003/diff/49001/chrome/browser/ui/browser...
chrome/browser/ui/browser_close_browsertest.cc:259: << ": " <<
check_case.DebugString());
Nit: <<s should line up; try

    SCOPED_TRACE(testing::Message() << "Case" << i << ": "
                                    << check_case.DebugString());

https://codereview.chromium.org/26938003/diff/49001/chrome/browser/ui/browser...
chrome/browser/ui/browser_close_browsertest.cc:277: return false;
Don't EXPECT_EQ and then handle failure; if you truly need to abort the test on
failure, just use an ASSERT_EQ instead.  (Generally, you should only abort the
test if proceeding will result in a NULL deref or some other crash.)  (2 places)

[felt, 2013-10-15 20:03:52]

On 2013/10/15 19:10:27, Peter Kasting wrote:
> c/b/ui LGTM, but this is a very risky change.  I left some comments on the
bug. 
> Additionally, you should gather metrics on how often this triggers, and
ideally
> how often people retry downloads canceled this way.

I left a response on the bug. In general, I think this is a very important
change to getting the malware CTR down. We really, really don't want people to
be installing the malware files. Then on browser close, we ask them if they're
sure the don't want to keep the files? This encourages them to hit "keep" and
save them (which is the dangerous behavior that we don't want).

However, I'm happy to limit this to only the very-low-FP warnings for now and
then see if that yields any problems or user complaints.

> 
> Please also clear this with the UI leads if you have not already done so.

Yes, this is all going through UI review.

> 
>
https://codereview.chromium.org/26938003/diff/49001/chrome/browser/ui/browser...
> File chrome/browser/ui/browser_close_browsertest.cc (right):
> 
>
https://codereview.chromium.org/26938003/diff/49001/chrome/browser/ui/browser...
> chrome/browser/ui/browser_close_browsertest.cc:259: << ": " <<
> check_case.DebugString());
> Nit: <<s should line up; try
> 
>     SCOPED_TRACE(testing::Message() << "Case" << i << ": "
>                                     << check_case.DebugString());
> 
>
https://codereview.chromium.org/26938003/diff/49001/chrome/browser/ui/browser...
> chrome/browser/ui/browser_close_browsertest.cc:277: return false;
> Don't EXPECT_EQ and then handle failure; if you truly need to abort the test
on
> failure, just use an ASSERT_EQ instead.  (Generally, you should only abort the
> test if proceeding will result in a NULL deref or some other crash.)  (2
places)

[asanka, 2013-10-15 21:39:07]

Apologies for the delay.

https://codereview.chromium.org/26938003/diff/49001/chrome/browser/lifetime/b...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/26938003/diff/49001/chrome/browser/lifetime/b...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:654:
content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL);
It would be great if we didn't add a public interface to DownloadItem for this
test. A SetDangerType() did exist once and we removed it because allowing the
danger type to change at arbitrary points in time made it difficult to reason
about the state transitions for a download. So we now allow the danger type to
be set at two points in time: during file name determination using
content::DownloadTargetCallback, and after all the data has been downloaded
using DownloadItem::OnContentCheckCompleted().

It's more code, but the cleanest way to test this, IMO, is to inject a
ChromeDownloadManagerDelegate subclass that overrides
DownloadManagerDelegate::DetermineDownloadTarget() to:
- Wrap the incoming DownloadTargetCallback with one that sets the danger type to
DOWNLOAD_DANGER_TYPE_DANGEROUS_URL.
- invokes CDMD::DetermineDownloadTarget() with the wrapped callback.

You can then set this CDMD subclass as the DownloadManagerDelegate to use using
DownloadService::SetDownloadManagerDelegateForTesting().

This way, the download will behave the same as one which was flagged by
SafeBrowsing as DANGEROUS_URL.

Failing that, the SetDangerType could be renamed to SetDangerTypeForTesting().
However, calling it on all platforms in this case is only safe because the
download was already marked as DANGEROUS_FILE. Setting it to DANGEROUS_URL
doesn't change the IsDangerous() state.

https://codereview.chromium.org/26938003/diff/49001/content/public/browser/do...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/26938003/diff/49001/content/public/browser/do...
content/public/browser/download_item.h:325: virtual std::string DebugString(bool
verbose) const = 0;
On 2013/10/15 19:00:44, Jói wrote:
> Are these two methods only used by tests? Could they be renamed to end in
> ForTesting?

DebugString() is supposed to be used for logging as well for non-test targets.

[felt, 2013-10-18 01:50:53]

Asanka, I created a TestDownloadManagerDelegate.

https://codereview.chromium.org/26938003/diff/49001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_close_browsertest.cc (right):

https://codereview.chromium.org/26938003/diff/49001/chrome/browser/ui/browser...
chrome/browser/ui/browser_close_browsertest.cc:259: << ": " <<
check_case.DebugString());
On 2013/10/15 19:10:28, Peter Kasting wrote:
> Nit: <<s should line up; try
> 
>     SCOPED_TRACE(testing::Message() << "Case" << i << ": "
>                                     << check_case.DebugString());

Done.

https://codereview.chromium.org/26938003/diff/49001/chrome/browser/ui/browser...
chrome/browser/ui/browser_close_browsertest.cc:277: return false;
On 2013/10/15 19:10:28, Peter Kasting wrote:
> Don't EXPECT_EQ and then handle failure; if you truly need to abort the test
on
> failure, just use an ASSERT_EQ instead.  (Generally, you should only abort the
> test if proceeding will result in a NULL deref or some other crash.)  (2
places)

This isn't my test -- I am just tweaking someone else's test to use the new
method instead of the old method. rdsmith@ wrote the test and I trust him, so I
am wondering if there is some reason for it. I've pinged him to ask why he is
returning false instead of ASSERT_EQ-ing. I'll go back and refactor if he says
there's no reason.

https://codereview.chromium.org/26938003/diff/49001/content/public/browser/do...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/26938003/diff/49001/content/public/browser/do...
content/public/browser/download_item.h:325: virtual std::string DebugString(bool
verbose) const = 0;
On 2013/10/15 21:39:08, asanka wrote:
> On 2013/10/15 19:00:44, Jói wrote:
> > Are these two methods only used by tests? Could they be renamed to end in
> > ForTesting?
> 
> DebugString() is supposed to be used for logging as well for non-test targets.

Removed.

https://codereview.chromium.org/26938003/diff/71001/content/browser/download/...
File content/browser/download/download_manager_impl.cc (right):

https://codereview.chromium.org/26938003/diff/71001/content/browser/download/...
content/browser/download/download_manager_impl.cc:681:
it->second->GetDangerType() != DOWNLOAD_DANGER_TYPE_DANGEROUS_URL &&
pkasting: changed to this, to address your concern.

[felt, 2013-10-18 23:24:48]

Hi Asanka,

I can't figure out why the browser test fails on CrOS. It looks like either the
download hangs on CrOS, or it is never marked as dangerous. As a result, the
test doesn't run and eventually times out. However, as far as I can tell the
patch itself does not cause problems in CrOS.

Here is my proposal: I'll land this with the tests disabled for CrOS (so we get
the benefit of the fix into the dev build on Monday?). Then I'll file a bug on
myself and get them working in CrOS asap.

Adrienne

[Nico, 2013-10-18 23:27:40]

On Fri, Oct 18, 2013 at 4:24 PM, <felt@chromium.org> wrote:

> Hi Asanka,
>
> I can't figure out why the browser test fails on CrOS. It looks like
> either the
> download hangs on CrOS, or it is never marked as dangerous. As a result,
> the
> test doesn't run and eventually times out. However, as far as I can tell
> the
> patch itself does not cause problems in CrOS.
>
> Here is my proposal: I'll land this with the tests disabled for CrOS (so
> we get
> the benefit of the fix into the dev build on Monday?). Then I'll file a
> bug on
> myself and get them working in CrOS asap.
>

Why not get it working first?


>
> Adrienne
>
>
https://codereview.chromium.**org/26938003/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[felt, 2013-10-18 23:45:34]

On 2013/10/18 23:27:40, Nico wrote:
> On Fri, Oct 18, 2013 at 4:24 PM, <mailto:felt@chromium.org> wrote:
> 
> > Hi Asanka,
> >
> > I can't figure out why the browser test fails on CrOS. It looks like
> > either the
> > download hangs on CrOS, or it is never marked as dangerous. As a result,
> > the
> > test doesn't run and eventually times out. However, as far as I can tell
> > the
> > patch itself does not cause problems in CrOS.
> >
> > Here is my proposal: I'll land this with the tests disabled for CrOS (so
> > we get
> > the benefit of the fix into the dev build on Monday?). Then I'll file a
> > bug on
> > myself and get them working in CrOS asap.
> >
> 
> Why not get it working first?

This is going into M32; if it lands Monday morning it will be able to go through
next week's Dev build to help find problems.

However, this is kind of a backup plan... I am currently working on debugging
it, I'd like the CrOS browsertests to be green.

> 
> 
> >
> > Adrienne
> >
> >
>
https://codereview.chromium.**org/26938003/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nico, 2013-10-18 23:53:22]

On Fri, Oct 18, 2013 at 4:45 PM, <felt@chromium.org> wrote:

> On 2013/10/18 23:27:40, Nico wrote:
>
>  On Fri, Oct 18, 2013 at 4:24 PM, <mailto:felt@chromium.org> wrote:
>>
>
>  > Hi Asanka,
>> >
>> > I can't figure out why the browser test fails on CrOS. It looks like
>> > either the
>> > download hangs on CrOS, or it is never marked as dangerous. As a result,
>> > the
>> > test doesn't run and eventually times out. However, as far as I can tell
>> > the
>> > patch itself does not cause problems in CrOS.
>> >
>> > Here is my proposal: I'll land this with the tests disabled for CrOS (so
>> > we get
>> > the benefit of the fix into the dev build on Monday?). Then I'll file a
>> > bug on
>> > myself and get them working in CrOS asap.
>> >
>>
>
>  Why not get it working first?
>>
>
> This is going into M32;


Things are done when they are done. We don't rush things in just because of
milestones. Marathon not sprint etc.


> if it lands Monday morning it will be able to go through
> next week's Dev build to help find problems.
>
> However, this is kind of a backup plan... I am currently working on
> debugging
> it, I'd like the CrOS browsertests to be green.
>
>
>
>  >
>> > Adrienne
>> >
>> >
>>
>
> https://codereview.chromium.****org/26938003/%3Chttps://codere**
> view.chromium.org/26938003/ <http://codereview.chromium.org/26938003/>>
>
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to
mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org>
>> .
>>
>
>
>
>
https://codereview.chromium.**org/26938003/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[asanka, 2013-10-19 00:42:30]

https://codereview.chromium.org/26938003/diff/323001/chrome/browser/lifetime/...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/26938003/diff/323001/chrome/browser/lifetime/...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:169: :
ChromeDownloadManagerDelegate(profile) {
On ChromeOS, the DriveIntegrationService kicks in very early and pokes at the
DownloadManager causing ChromeDownloadManagerDelegate and the DownloadHistory
classes to be created and initialized. One bit that has changed recently is that
CDMD::SetNextId() needs to be called in order for CDMD to be able to handle new
downloads. This is done once by DownloadHistory.

By the time your test calls DownloadService::SetDownloadManagerForTesting(),
this has already happened on Chrome OS. So the new TestDownloadManagerDelegate
never gets SetNextId() called on it. This is likely why your download hangs.

You can just call "SetNextId(content::DownloadItem::kInvalidId + 1);" here in
the constructor so that this test delegate can handle downloads.

https://codereview.chromium.org/26938003/diff/323001/chrome/browser/lifetime/...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:178: item,
dangerous_callback);
Nit: For extra resiliency:

return CDMD::DetermineDownloadTarget(...);

https://codereview.chromium.org/26938003/diff/323001/chrome/browser/lifetime/...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:679: // This .swf
normally would be categorized as DANGEROUS_HOST, but
Nit: DANGEROUS_FILE

[felt, 2013-10-19 02:50:08]

On 2013/10/18 23:53:22, Nico wrote:
> On Fri, Oct 18, 2013 at 4:45 PM, <mailto:felt@chromium.org> wrote:
> 
> > On 2013/10/18 23:27:40, Nico wrote:
> >
> >  On Fri, Oct 18, 2013 at 4:24 PM, <mailto:felt@chromium.org> wrote:
> >>
> >
> >  > Hi Asanka,
> >> >
> >> > I can't figure out why the browser test fails on CrOS. It looks like
> >> > either the
> >> > download hangs on CrOS, or it is never marked as dangerous. As a result,
> >> > the
> >> > test doesn't run and eventually times out. However, as far as I can tell
> >> > the
> >> > patch itself does not cause problems in CrOS.
> >> >
> >> > Here is my proposal: I'll land this with the tests disabled for CrOS (so
> >> > we get
> >> > the benefit of the fix into the dev build on Monday?). Then I'll file a
> >> > bug on
> >> > myself and get them working in CrOS asap.
> >> >
> >>
> >
> >  Why not get it working first?
> >>
> >
> > This is going into M32;
> 
> 
> Things are done when they are done. We don't rush things in just because of
> milestones. Marathon not sprint etc.

You're right, this was a stupid/tired idea on my part. Apologies.

> 
> 
> > if it lands Monday morning it will be able to go through
> > next week's Dev build to help find problems.
> >
> > However, this is kind of a backup plan... I am currently working on
> > debugging
> > it, I'd like the CrOS browsertests to be green.
> >
> >
> >
> >  >
> >> > Adrienne
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.****org/26938003/%253Chttps://codere**
> > view.chromium.org/26938003/ <http://codereview.chromium.org/26938003/>>
> >
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to
>
mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org>
> >> .
> >>
> >
> >
> >
> >
>
https://codereview.chromium.**org/26938003/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[felt, 2013-10-19 02:51:49]

https://codereview.chromium.org/26938003/diff/323001/chrome/browser/lifetime/...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/26938003/diff/323001/chrome/browser/lifetime/...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:169: :
ChromeDownloadManagerDelegate(profile) {
On 2013/10/19 00:42:31, asanka wrote:
> On ChromeOS, the DriveIntegrationService kicks in very early and pokes at the
> DownloadManager causing ChromeDownloadManagerDelegate and the DownloadHistory
> classes to be created and initialized. One bit that has changed recently is
that
> CDMD::SetNextId() needs to be called in order for CDMD to be able to handle
new
> downloads. This is done once by DownloadHistory.
> 
> By the time your test calls DownloadService::SetDownloadManagerForTesting(),
> this has already happened on Chrome OS. So the new TestDownloadManagerDelegate
> never gets SetNextId() called on it. This is likely why your download hangs.
> 
> You can just call "SetNextId(content::DownloadItem::kInvalidId + 1);" here in
> the constructor so that this test delegate can handle downloads.

Thank you for looking into this. That has fixed it!!! (The failures that remain
in the try jobs are unrelated to this change. All the relevant browsertests are
running green.)

https://codereview.chromium.org/26938003/diff/323001/chrome/browser/lifetime/...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:178: item,
dangerous_callback);
On 2013/10/19 00:42:31, asanka wrote:
> Nit: For extra resiliency:
> 
> return CDMD::DetermineDownloadTarget(...);

Done.

https://codereview.chromium.org/26938003/diff/323001/chrome/browser/lifetime/...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:679: // This .swf
normally would be categorized as DANGEROUS_HOST, but
On 2013/10/19 00:42:31, asanka wrote:
> Nit: DANGEROUS_FILE

Done.

[asanka, 2013-10-20 04:32:27]

LGTM

[commit-bot: I haz the power, 2013-10-20 17:14:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felt@chromium.org/26938003/313001

[commit-bot: I haz the power, 2013-10-21 03:10:37]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[felt, 2013-10-21 12:10:47]

Committed patchset #17 manually as r229784 (presubmit successful).