Use RenderProcessHost::Shutdown in test to avoid flakiness.

Use RenderProcessHost::Shutdown in test to avoid flakiness.

Speculative fix for timeouts on linux_android_rel_ng in the
FrameTreeBrowserTest.FrameTreeAfterCrash test.

BUG=727725
TEST=Less flakiness
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2919483003
Cr-Commit-Position: refs/heads/master@{#476362}
Committed: https://chromium.googlesource.com/chromium/src/+/298b9328adbfe81b92c2dadce496dac1e8ddfeb5

[Charlie Reis, 2017-06-01 00:16:32]

Description was changed from

==========
Use RenderProcessHost::Shutdown in test to avoid flakiness.

Speculative fix for timeouts on linux_android_rel_ng in the
FrameTreeBrowserTest.FrameTreeAfterCrash test.

BUG=727725
TEST=Less flakiness
==========

to

==========
Use RenderProcessHost::Shutdown in test to avoid flakiness.

Speculative fix for timeouts on linux_android_rel_ng in the
FrameTreeBrowserTest.FrameTreeAfterCrash test.

BUG=727725
TEST=Less flakiness
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-06-01 00:16:59]

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

[commit-bot: I haz the power, 2017-06-01 00:18:04]

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-06-01 01:44:06]

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

[commit-bot: I haz the power, 2017-06-01 01:44:07]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-06-01 15:06:01]

creis@chromium.org changed reviewers:
+ lukasza@chromium.org

[Charlie Reis, 2017-06-01 15:06:02]

Lukasz, can you review?  I'm curious if this helps with the Android flakiness,
since we use this approach in some other tests.

[Łukasz Anforowicz, 2017-06-01 16:10:29]

lgtm, assumming the questions/nits below are answered

I tried looking at the bug and
1) I see that FrameTreeAfterCrash appears in the 2 failed runs pointed out in
#c3 and #c4, but I failed to see any details in the logs (they don't even say if
the test timed out).
2) When looking at other logs I didn't see FrameTreeAfterCrash.

Given above, I am not sure how we will be able to evaluate if this speculative
CL helped or not.

https://codereview.chromium.org/2919483003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/2919483003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_browsertest.cc:138:
shell()->web_contents()->GetMainFrame()->GetProcess()->Shutdown(0, false);
It's probably a good idea to check (probably via EXPECT_TRUE rather than
ASSERT_TRUE) the return value from Shutdown (it can be failure-indicating-false
if !child_process_launcher_.get() or IsStarting().

I wonder if we should pass |true| as the second argument - I see in
base::Process::Terminate that it starts with SIGTERM, but when |wait| is true
and the process takes a long time to die, then it also does SIGKILL.  OTOH, if
the process hung, then I guess chrome://crash would also take a long time...
(but then maybe this is the source of flakiness that we are trying to avoid...).

[Charlie Reis, 2017-06-01 17:04:16]

On 2017/06/01 16:10:29, Łukasz A. wrote:
> lgtm, assumming the questions/nits below are answered
> 
> I tried looking at the bug and
> 1) I see that FrameTreeAfterCrash appears in the 2 failed runs pointed out in
> #c3 and #c4, but I failed to see any details in the logs (they don't even say
if
> the test timed out).
> 2) When looking at other logs I didn't see FrameTreeAfterCrash.
> 
> Given above, I am not sure how we will be able to evaluate if this speculative
> CL helped or not.

Nice find.  I still think this may be worthwhile to try, but you're probably
right that there's a good chance the problems will continue.  Let's test the
theory, though.

https://codereview.chromium.org/2919483003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/2919483003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_browsertest.cc:138:
shell()->web_contents()->GetMainFrame()->GetProcess()->Shutdown(0, false);
On 2017/06/01 16:10:29, Łukasz A. wrote:
> It's probably a good idea to check (probably via EXPECT_TRUE rather than
> ASSERT_TRUE) the return value from Shutdown (it can be
failure-indicating-false
> if !child_process_launcher_.get() or IsStarting().

Good idea.  But this is one of the cases that ASSERT_TRUE is good for-- the rest
of the test doesn't make sense if this kill fails, so it's good to early exit. 
The rest of the results won't have meaning otherwise.

> 
> I wonder if we should pass |true| as the second argument - I see in
> base::Process::Terminate that it starts with SIGTERM, but when |wait| is true
> and the process takes a long time to die, then it also does SIGKILL.  OTOH, if
> the process hung, then I guess chrome://crash would also take a long time...
> (but then maybe this is the source of flakiness that we are trying to
avoid...).

Might as well-- we want this kill to be as effective as possible to make the
test meaningful.

[Charlie Reis, 2017-06-01 17:04:32]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2017-06-01 17:04:32]

The patchset sent to the CQ was uploaded after l-g-t-m from lukasza@chromium.org
Link to the patchset: https://codereview.chromium.org/2919483003/#ps20001
(title: "More effective")

[commit-bot: I haz the power, 2017-06-01 17:05:04]

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-06-01 18:31:55]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496336672072380,
"parent_rev": "ab458a607fbceeee4a548431fbcd9dfd8d49bdcb", "commit_rev":
"298b9328adbfe81b92c2dadce496dac1e8ddfeb5"}

[commit-bot: I haz the power, 2017-06-01 18:32:05]

Description was changed from

==========
Use RenderProcessHost::Shutdown in test to avoid flakiness.

Speculative fix for timeouts on linux_android_rel_ng in the
FrameTreeBrowserTest.FrameTreeAfterCrash test.

BUG=727725
TEST=Less flakiness
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use RenderProcessHost::Shutdown in test to avoid flakiness.

Speculative fix for timeouts on linux_android_rel_ng in the
FrameTreeBrowserTest.FrameTreeAfterCrash test.

BUG=727725
TEST=Less flakiness
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2919483003
Cr-Commit-Position: refs/heads/master@{#476362}
Committed:
https://chromium.googlesource.com/chromium/src/+/298b9328adbfe81b92c2dadce496...
==========

[commit-bot: I haz the power, 2017-06-01 18:32:06]

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