Fix flaky DownloadTest.FeedbackServiceKeepDownload

Fix flaky DownloadTest.FeedbackServiceKeepDownload

BUG=705224
Review-Url: https://codereview.chromium.org/2778853002
Cr-Original-Commit-Position: refs/heads/master@{#460894}
Committed: https://chromium.googlesource.com/chromium/src/+/0e417c028e62d771638997c4545a359ba93027ec
Review-Url: https://codereview.chromium.org/2778853002
Cr-Commit-Position: refs/heads/master@{#461287}
Committed: https://chromium.googlesource.com/chromium/src/+/bf73243bf9eb70d813d800f9b4e49df688452b80

[Jialiu Lin, 2017-03-27 19:43:09]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 19:43:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 20:20:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 20:20:42]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-03-27 20:33:45]

jialiul@chromium.org changed reviewers:
+ asanka@chromium.org

[Jialiu Lin, 2017-03-27 20:33:46]

Hi asanka@,
DownloadTest.FeedbackServiceKeepDownload is recently flaky on try bots. Seems it
only happens on Linux. (https://crbug.com/705224)

I could not reproduce this flakiness locally. But I guess it is somehow due to
the the current_path_ of a DownloadItem gets reset after a KEEP command.

We didn't change anything on safe_browsing::DownloadFeedbackService. Do you know
if there is anything changed in c/b/download/* recently can cause this
flakiness?

Thanks!

[asanka, 2017-03-27 23:31:06]

asanka@chromium.org changed reviewers:
+ dtrainor@chromium.org

[asanka, 2017-03-27 23:31:07]

Not sure. I haven't kept up with changes in downloads land. Maybe dtrainor might
have a better idea.

[asanka, 2017-03-27 23:31:23]

On 2017/03/27 20:33:46, Jialiu Lin wrote:
> Hi asanka@,
> DownloadTest.FeedbackServiceKeepDownload is recently flaky on try bots. Seems
it
> only happens on Linux. (https://crbug.com/705224)
> 
> I could not reproduce this flakiness locally. But I guess it is somehow due to
> the the current_path_ of a DownloadItem gets reset after a KEEP command.
> 
> We didn't change anything on safe_browsing::DownloadFeedbackService. Do you
know
> if there is anything changed in c/b/download/* recently can cause this
> flakiness?
> 
> Thanks!

Not sure. I haven't kept up with changes in downloads land. Maybe dtrainor might
have a better idea.

[David Trainor- moved to gerrit, 2017-03-28 03:15:45]

dtrainor@chromium.org changed reviewers:
+ qinmin@chromium.org

[David Trainor- moved to gerrit, 2017-03-28 03:15:46]

Adding qinmin@ who has been landing a lot of CLs there locally.  I'll take a
look tomorrow as well.

[qinmin, 2017-03-29 00:12:21]

On 2017/03/28 03:15:46, David Trainor-ping if over 24h wrote:
> Adding qinmin@ who has been landing a lot of CLs there locally.  I'll take a
> look tomorrow as well.

Do we actually have some stacktrace about this? Since the download completes,
the file always exists at target path.
Can we  log what is the current path when the test fails? So that we can better
analyze why this happens.

[Jialiu Lin, 2017-03-29 00:24:24]

On 2017/03/29 at 00:12:21, qinmin wrote:
> On 2017/03/28 03:15:46, David Trainor-ping if over 24h wrote:
> > Adding qinmin@ who has been landing a lot of CLs there locally.  I'll take a
> > look tomorrow as well.
> 
> Do we actually have some stacktrace about this? Since the download completes,
the file always exists at target path.
> Can we  log what is the current path when the test fails? So that we can
better analyze why this happens.

I was unable to reproduce this failure locally. But from try bot logs, the
failure looks like following:
DownloadTest.FeedbackServiceKeepDownload (run #1):
[ RUN      ] DownloadTest.FeedbackServiceKeepDownload
Xlib:  extension "RANDR" missing on display ":99".
[9522:9522:0327/013233.121835:WARNING:audio_manager.cc(295)] Multiple instances
of AudioManager detected
[9522:9522:0327/013233.121870:WARNING:audio_manager.cc(254)] Multiple instances
of AudioManager detected
[9522:9522:0327/013233.209304:WARNING:password_store_factory.cc(247)] Using
basic (unencrypted) store for password storage. See
https://chromium.googlesource.com/chromium/src/+/master/docs/linux_password_s...
for more information about password storage options.
../../chrome/browser/download/download_browsertest.cc:3492: Failure
Value of: PathExists(updated_downloads[0]->GetFullPath())
  Actual: false
Expected: true
[9522:9522:0327/013234.892222:ERROR:two_phase_uploader.cc(118)] URLFetcher
failed, status=3 err=-11
[9522:9522:0327/013234.967119:WARNING:url_request_context_getter.cc(43)]
URLRequestContextGetter leaking due to no owning thread.
[  FAILED  ] DownloadTest.FeedbackServiceKeepDownload, where TypeParam =  and
GetParam() =  (2090 ms)

My current plan is to check both current path and target path. At least one of
them should exist.
If you agree this is safe to do without masking any potential problem, I'll go
ahead submit this change.

[qinmin, 2017-03-30 18:43:31]

On 2017/03/29 00:24:24, Jialiu Lin wrote:
> On 2017/03/29 at 00:12:21, qinmin wrote:
> > On 2017/03/28 03:15:46, David Trainor-ping if over 24h wrote:
> > > Adding qinmin@ who has been landing a lot of CLs there locally.  I'll take
a
> > > look tomorrow as well.
> > 
> > Do we actually have some stacktrace about this? Since the download
completes,
> the file always exists at target path.
> > Can we  log what is the current path when the test fails? So that we can
> better analyze why this happens.
> 
> I was unable to reproduce this failure locally. But from try bot logs, the
> failure looks like following:
> DownloadTest.FeedbackServiceKeepDownload (run #1):
> [ RUN      ] DownloadTest.FeedbackServiceKeepDownload
> Xlib:  extension "RANDR" missing on display ":99".
> [9522:9522:0327/013233.121835:WARNING:audio_manager.cc(295)] Multiple
instances
> of AudioManager detected
> [9522:9522:0327/013233.121870:WARNING:audio_manager.cc(254)] Multiple
instances
> of AudioManager detected
> [9522:9522:0327/013233.209304:WARNING:password_store_factory.cc(247)] Using
> basic (unencrypted) store for password storage. See
>
https://chromium.googlesource.com/chromium/src/+/master/docs/linux_password_s...
> for more information about password storage options.
> ../../chrome/browser/download/download_browsertest.cc:3492: Failure
> Value of: PathExists(updated_downloads[0]->GetFullPath())
>   Actual: false
> Expected: true
> [9522:9522:0327/013234.892222:ERROR:two_phase_uploader.cc(118)] URLFetcher
> failed, status=3 err=-11
> [9522:9522:0327/013234.967119:WARNING:url_request_context_getter.cc(43)]
> URLRequestContextGetter leaking due to no owning thread.
> [  FAILED  ] DownloadTest.FeedbackServiceKeepDownload, where TypeParam =  and
> GetParam() =  (2090 ms)
> 
> My current plan is to check both current path and target path. At least one of
> them should exist.
> If you agree this is safe to do without masking any potential problem, I'll go
> ahead submit this change.

lgtm % comment
So the issue is that Download is in the middle of renaming to the final target
when this happens.
I am ok with the current change, but it would be better to add a
DownloadTestObserverNotInProgress to wait for the download to complete. Then
check the TargetFilePath() rather than assumeing the download can be either in a
completing or a complete state.

[Jialiu Lin, 2017-03-30 19:58:49]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 19:59:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-03-30 20:00:50]

jialiul@chromium.org changed reviewers:
- asanka@chromium.org

[Jialiu Lin, 2017-03-30 20:00:50]

Thanks qinmin@! Very good suggestion. 
My latest patch added a DownloadTestObserverNotInProgress to make sure
evaluation happens after download complete. 

Thanks,

[commit-bot: I haz the power, 2017-03-30 21:04:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 21:04:43]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-03-30 21:29:34]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-03-30 21:29:34]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2778853002/#ps20001
(title: "address qinmin's comment")

[commit-bot: I haz the power, 2017-03-30 21:30:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 21:46:25]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490909374306680,
"parent_rev": "a02e5db6a496574c516985350f2f6366771ff741", "commit_rev":
"0e417c028e62d771638997c4545a359ba93027ec"}

[commit-bot: I haz the power, 2017-03-30 21:47:18]

Description was changed from

==========
Fix flaky DownloadTest.FeedbackServiceKeepDownload

BUG=705224
==========

to

==========
Fix flaky DownloadTest.FeedbackServiceKeepDownload

BUG=705224

Review-Url: https://codereview.chromium.org/2778853002
Cr-Commit-Position: refs/heads/master@{#460894}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e417c028e62d771638997c4545a...
==========

[commit-bot: I haz the power, 2017-03-30 21:47:26]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0e417c028e62d771638997c4545a...

[asanka, 2017-03-31 14:37:23]

asanka@chromium.org changed reviewers:
+ asanka@chromium.org

[asanka, 2017-03-31 14:37:24]

https://codereview.chromium.org/2778853002/diff/20001/chrome/browser/download...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2778853002/diff/20001/chrome/browser/download...
chrome/browser/download/download_browsertest.cc:3487:
std::unique_ptr<DownloadTestObserverNotInProgress> completion_observer(
Instead, could you use a DownloadTestObserverTerminal ? That can begin observing
before calling BeginFeedbackForDownload.

The Dangerous -> SAFE -> Complete transition should then deterministically
completed after the download file has been copied and renamed.

Also, you can avoid the IsFinished()->StartObserving()->WaitForFinished()
sequence.

[Jialiu Lin, 2017-03-31 16:24:41]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2786103005/ by jialiul@chromium.org.

The reason for reverting is: Should use DownloadTestObserverTerminal instead. .

[Jialiu Lin, 2017-03-31 20:25:36]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 20:26:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 21:03:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 21:03:57]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-03-31 21:34:34]

Thanks asanka@.
My latest patch uses DownloadTestObserverTerminal instead. Let me know if you
have further comment. 

Thanks,

https://codereview.chromium.org/2778853002/diff/20001/chrome/browser/download...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2778853002/diff/20001/chrome/browser/download...
chrome/browser/download/download_browsertest.cc:3487:
std::unique_ptr<DownloadTestObserverNotInProgress> completion_observer(
On 2017/03/31 at 14:37:24, asanka wrote:
> Instead, could you use a DownloadTestObserverTerminal ? That can begin
observing before calling BeginFeedbackForDownload.
> 
> The Dangerous -> SAFE -> Complete transition should then deterministically
completed after the download file has been copied and renamed.
> 
> Also, you can avoid the IsFinished()->StartObserving()->WaitForFinished()
sequence.

Done.

https://codereview.chromium.org/2778853002/diff/20001/chrome/browser/download...
chrome/browser/download/download_browsertest.cc:3487:
std::unique_ptr<DownloadTestObserverNotInProgress> completion_observer(
On 2017/03/31 at 14:37:24, asanka wrote:
> Instead, could you use a DownloadTestObserverTerminal ? That can begin
observing before calling BeginFeedbackForDownload.
> 
> The Dangerous -> SAFE -> Complete transition should then deterministically
completed after the download file has been copied and renamed.
> 
> Also, you can avoid the IsFinished()->StartObserving()->WaitForFinished()
sequence.

Done.

[asanka, 2017-03-31 21:45:17]

lgtm

[Jialiu Lin, 2017-04-01 00:26:42]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-04-01 00:26:43]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2778853002/#ps40001
(title: "Use DownloadTestObserverTerminal instead")

[commit-bot: I haz the power, 2017-04-01 00:27:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-01 00:32:25]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491006402743910,
"parent_rev": "ac2fbde6e594cfb40f4806daf8c3c108d88d7574", "commit_rev":
"bf73243bf9eb70d813d800f9b4e49df688452b80"}

[commit-bot: I haz the power, 2017-04-01 00:33:40]

Description was changed from

==========
Fix flaky DownloadTest.FeedbackServiceKeepDownload

BUG=705224

Review-Url: https://codereview.chromium.org/2778853002
Cr-Commit-Position: refs/heads/master@{#460894}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e417c028e62d771638997c4545a...
==========

to

==========
Fix flaky DownloadTest.FeedbackServiceKeepDownload

BUG=705224

Review-Url: https://codereview.chromium.org/2778853002
Cr-Original-Commit-Position: refs/heads/master@{#460894}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e417c028e62d771638997c4545a...
Review-Url: https://codereview.chromium.org/2778853002
Cr-Commit-Position: refs/heads/master@{#461287}
Committed:
https://chromium.googlesource.com/chromium/src/+/bf73243bf9eb70d813d800f9b4e4...
==========

[commit-bot: I haz the power, 2017-04-01 00:33:41]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/bf73243bf9eb70d813d800f9b4e4...