[Android] Fix crash when closing tab during infobar cancellation

[Android] Fix crash when closing tab during infobar cancellation

Cancelling the 'dangerous file' dialog could cause the current tab to be
closed, triggering the infobar to be deleted. The now deleted infobar
would then try to close itself after cancellation, causing a crash.

Also reworks InfoBarUtil to click infobar buttons directly rather than
through tap events, reducing flakiness.

BUG=615280
TEST=DownloadTest,GeolocationTest,InfoBarTest,InfoBarContainerTest

Committed: https://crrev.com/0861c56c582da4866a84104dc5764a8adad84dd6
Cr-Commit-Position: refs/heads/master@{#397017}

[mcdavid, 2016-05-27 01:09:47]

Description was changed from

==========
[Android] Fix crash when closing tab during infobar cancellation

Cancelling the 'dangerous file' dialog could cause the current tab to be
closed, triggering the infobar to be deleted. The now deleted infobar
would then try to close itself after cancellation, causing a crash.

Also reworks InfoBarUtil to click infobar buttons directly rather than
through tap events, reducing flakiness.

BUG=615280
TEST=DownloadTest,GeolocationTest,InfoBarTest,InfoBarContainerTest
==========

to

==========
[Android] Fix crash when closing tab during infobar cancellation

Cancelling the 'dangerous file' dialog could cause the current tab to be
closed, triggering the infobar to be deleted. The now deleted infobar
would then try to close itself after cancellation, causing a crash.

Also reworks InfoBarUtil to click infobar buttons directly rather than
through tap events, reducing flakiness.

BUG=615280
TEST=DownloadTest,GeolocationTest,InfoBarTest,InfoBarContainerTest
==========

[mcdavid, 2016-05-27 01:09:47]

mcdavid@amazon.com changed reviewers:
+ dfalcantara@chromium.org, qinmin@chromium.org

[mcdavid, 2016-05-27 01:13:36]

The api in confirm_infobar_delegate.h already requires to return false if the
infobar was closed, but SimpleConfirmInfoBarDelegate was breaking that. PTAL

[gone, 2016-05-27 01:43:03]

The DangerousDownloadInfoBar should really have been written as a real infobar
backed by a native-side ConfirmInfoBarDelegate to avoid this issue, since its
strange tab-closing behavior has resulted in many, many bugs.

I tried to patch this class by writing the SimpleConfirmInfoBarBuilder but
apparently missed this hole.  Thanks for looking into it.

https://codereview.chromium.org/2019533002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/2019533002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:108:
mPendingRequest = null;
These two lines should probably be moved above to the else if (confirm) block. 
That's the only situation where this can be hit.

https://codereview.chromium.org/2019533002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/SimpleConfirmInfoBarBuilder.java
(right):

https://codereview.chromium.org/2019533002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SimpleConfirmInfoBarBuilder.java:38:
*         true otherwise.
This condition should be flipped so that true is returned if something happened
and false if nothing happened.  It'd avoid having to negate the result of
closeBlankTab() immediately, as well.

https://codereview.chromium.org/2019533002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SimpleConfirmInfoBarBuilder.java:92:
return true;
Depending on whether the return value definition is changed above:
return (listener == null) ? true : listener.onInfoBarButtonClicked(isPrimary);

https://codereview.chromium.org/2019533002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java
(right):

https://codereview.chromium.org/2019533002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:119:
}));
Would the crash normally happen as soon as the tab count is correct?  I'm
worried that the test would be declared a success too early, but I don't know
what other conditions we could check for.

https://codereview.chromium.org/2019533002/diff/1/chrome/test/android/javates...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/InfoBarUtil.java
(right):

https://codereview.chromium.org/2019533002/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/InfoBarUtil.java:25:
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
Is the instrumentation available here?  If so, use
TestTouchUtils#performClickOnMainSync instead.  If not, this is fine.

[mcdavid, 2016-05-27 02:06:03]

On 2016/05/27 01:43:03, dfalcantara wrote:
>
https://codereview.chromium.org/2019533002/diff/1/chrome/android/javatests/sr...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java
> (right):
> 
>
https://codereview.chromium.org/2019533002/diff/1/chrome/android/javatests/sr...
>
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:119:
> }));
> Would the crash normally happen as soon as the tab count is correct?  I'm
> worried that the test would be declared a success too early, but I don't know
> what other conditions we could check for.

I was worried about that as well which is why I polled on the UI thread - the
thread is blocked until the tab close + following infobar close both (would
have) complete

> 
>
https://codereview.chromium.org/2019533002/diff/1/chrome/test/android/javates...
> File
>
chrome/test/android/javatests/src/org/chromium/chrome/test/util/InfoBarUtil.java
> (right):
> 
>
https://codereview.chromium.org/2019533002/diff/1/chrome/test/android/javates...
>
chrome/test/android/javatests/src/org/chromium/chrome/test/util/InfoBarUtil.java:25:
> ThreadUtils.runOnUiThreadBlocking(new Runnable() {
> Is the instrumentation available here?  If so, use
> TestTouchUtils#performClickOnMainSync instead.  If not, this is fine.

Doesn't look like it - I could pass it in?

[gone, 2016-05-27 16:46:04]

On 2016/05/27 02:06:03, mcdavid wrote:
> On 2016/05/27 01:43:03, dfalcantara wrote:
> >
>
https://codereview.chromium.org/2019533002/diff/1/chrome/android/javatests/sr...
> > File
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2019533002/diff/1/chrome/android/javatests/sr...
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:119:
> > }));
> > Would the crash normally happen as soon as the tab count is correct?  I'm
> > worried that the test would be declared a success too early, but I don't
know
> > what other conditions we could check for.
> 
> I was worried about that as well which is why I polled on the UI thread - the
> thread is blocked until the tab close + following infobar close both (would
> have) complete

Gotcha.

> > 
> >
>
https://codereview.chromium.org/2019533002/diff/1/chrome/test/android/javates...
> > File
> >
>
chrome/test/android/javatests/src/org/chromium/chrome/test/util/InfoBarUtil.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2019533002/diff/1/chrome/test/android/javates...
> >
>
chrome/test/android/javatests/src/org/chromium/chrome/test/util/InfoBarUtil.java:25:
> > ThreadUtils.runOnUiThreadBlocking(new Runnable() {
> > Is the instrumentation available here?  If so, use
> > TestTouchUtils#performClickOnMainSync instead.  If not, this is fine.
> 
> Doesn't look like it - I could pass it in?

Nah, it's fine.  That change would require wider changes than you're fixing. 
Just patch up the other comments and I think we'll be good.

[mcdavid, 2016-05-27 21:49:13]

On 2016/05/27 01:43:03, dfalcantara wrote:
>
https://codereview.chromium.org/2019533002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/SimpleConfirmInfoBarBuilder.java
> (right):
> 
>
https://codereview.chromium.org/2019533002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/infobar/SimpleConfirmInfoBarBuilder.java:38:
> *         true otherwise.
> This condition should be flipped so that true is returned if something
happened
> and false if nothing happened.  It'd avoid having to negate the result of
> closeBlankTab() immediately, as well.

Reversed it up to the point where the result is passed back from Accept/Cancel -
going further than that would require changing all ~30 uses of
ConfirmInfoBarDelegate. Addressed everything else. PTAL

[gone, 2016-05-31 18:12:45]

lgtm

[mcdavid, 2016-05-31 18:53:29]

The CQ bit was checked by mcdavid@amazon.com

[commit-bot: I haz the power, 2016-05-31 18:53:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019533002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2019533002/20001

[commit-bot: I haz the power, 2016-05-31 22:16:12]

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

[commit-bot: I haz the power, 2016-05-31 22:16:13]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mcdavid, 2016-05-31 23:37:34]

The CQ bit was checked by mcdavid@amazon.com

[mcdavid, 2016-05-31 23:37:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2019533002/#ps40001
(title: "Fix test on mobile")

[commit-bot: I haz the power, 2016-05-31 23:38:19]

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

[commit-bot: I haz the power, 2016-06-01 01:36:26]

Description was changed from

==========
[Android] Fix crash when closing tab during infobar cancellation

Cancelling the 'dangerous file' dialog could cause the current tab to be
closed, triggering the infobar to be deleted. The now deleted infobar
would then try to close itself after cancellation, causing a crash.

Also reworks InfoBarUtil to click infobar buttons directly rather than
through tap events, reducing flakiness.

BUG=615280
TEST=DownloadTest,GeolocationTest,InfoBarTest,InfoBarContainerTest
==========

to

==========
[Android] Fix crash when closing tab during infobar cancellation

Cancelling the 'dangerous file' dialog could cause the current tab to be
closed, triggering the infobar to be deleted. The now deleted infobar
would then try to close itself after cancellation, causing a crash.

Also reworks InfoBarUtil to click infobar buttons directly rather than
through tap events, reducing flakiness.

BUG=615280
TEST=DownloadTest,GeolocationTest,InfoBarTest,InfoBarContainerTest
==========

[commit-bot: I haz the power, 2016-06-01 01:36:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-01 01:37:56]

Description was changed from

==========
[Android] Fix crash when closing tab during infobar cancellation

Cancelling the 'dangerous file' dialog could cause the current tab to be
closed, triggering the infobar to be deleted. The now deleted infobar
would then try to close itself after cancellation, causing a crash.

Also reworks InfoBarUtil to click infobar buttons directly rather than
through tap events, reducing flakiness.

BUG=615280
TEST=DownloadTest,GeolocationTest,InfoBarTest,InfoBarContainerTest
==========

to

==========
[Android] Fix crash when closing tab during infobar cancellation

Cancelling the 'dangerous file' dialog could cause the current tab to be
closed, triggering the infobar to be deleted. The now deleted infobar
would then try to close itself after cancellation, causing a crash.

Also reworks InfoBarUtil to click infobar buttons directly rather than
through tap events, reducing flakiness.

BUG=615280
TEST=DownloadTest,GeolocationTest,InfoBarTest,InfoBarContainerTest

Committed: https://crrev.com/0861c56c582da4866a84104dc5764a8adad84dd6
Cr-Commit-Position: refs/heads/master@{#397017}
==========

[commit-bot: I haz the power, 2016-06-01 01:37:57]

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