[ios] Automatically fail any WebTest whose render process crashes.

[ios] Automatically fail any WebTest whose render process crashes.

We speculate that some tests are flaky because iOS is killing WKWebViews while
the tests are running.  This CL adds a GlobalWebStateObserver method to listen
for this and explicitly fail the current test when a WKWebView dies.  Having an
explicit failure will make these cases more apparent when looking through test
logs.

This CL also adds an escape hatch for tests that intentionally crash the
renderer.

BUG=None

Review-Url: https://codereview.chromium.org/2666953003
Cr-Commit-Position: refs/heads/master@{#447615}
Committed: https://chromium.googlesource.com/chromium/src/+/caeca2dde86a85efeda94866988b3659f5951889

[rohitrao (ping after 24h), 2017-02-01 01:18:40]

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

[commit-bot: I haz the power, 2017-02-01 01:19:32]

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-02-01 02:25:59]

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

[commit-bot: I haz the power, 2017-02-01 02:26:00]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-02-01 16:43:03]

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

[Eugene But (OOO till 7-30), 2017-02-01 16:43:04]

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
File ios/web/public/test/web_test.h (right):

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.h:38: // that intentionally crash the render
process.
Would it be helpful to document that default state is false?

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.h:39: void
AllowRenderProcessCrashesDuringTesting(bool allow);
Should this be prefixed with Set (e.g.
SetAllowsRenderProcessCrashesDuringTesting)?

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
File ios/web/public/test/web_test.mm (right):

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.mm:12: namespace {
Drop this?

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.mm:23: void RenderProcessGone(WebState* web_state)
override {
Optional nit: Drop |web_state| argument

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.mm:29: : web_client_(base::WrapUnique(new
TestWebClient)) {
Do we need web client? WebTestSuite already sets it:
https://cs.chromium.org/chromium/src/ios/web/test/web_test_suite.mm?type=cs&q...

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.mm:54: if (allow) {
Optional nit: Do you want to use ternary?
renderer_crash_observer_ = allow ? nullptr :
base::MakeUnique<RendererCrashObserver>();

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
File ios/web/test/web_test_unittest.mm (right):

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:5: #import <UIKit/UIKit.h>
nit: s/<UIKit/UIKit.h>/<WKWebKit/WKWebKit.h> ?
Looks like this file does not use anything from UIKit but uses WKWebView

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:14: WKWebView* g_web_view = nil;
Should |g_web_view| be a member of WebTestFixtureTest? Can you just call
|web::SimulateWKWebViewCrash(webView_);?

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:26:
webView_.reset([web::BuildTerminatedWKWebView() retain]);
s/webView_/web_view_

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:28:
base::scoped_nsobject<TestWebViewContentView> webViewContentView(
s/webViewContentView/web_view_content_view

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:50: TEST_F(WebTestFixtureTest,
SucceedsOnRenderCrash) {
Please add comments

[rohitrao (ping after 24h), 2017-02-01 18:22:55]

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

[rohitrao (ping after 24h), 2017-02-01 18:23:00]

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
File ios/web/public/test/web_test.h (right):

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.h:38: // that intentionally crash the render
process.
On 2017/02/01 16:43:04, Eugene But wrote:
> Would it be helpful to document that default state is false?

Done.

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.h:39: void
AllowRenderProcessCrashesDuringTesting(bool allow);
On 2017/02/01 16:43:04, Eugene But wrote:
> Should this be prefixed with Set (e.g.
> SetAllowsRenderProcessCrashesDuringTesting)?

Done.

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
File ios/web/public/test/web_test.mm (right):

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.mm:12: namespace {
On 2017/02/01 16:43:04, Eugene But wrote:
> Drop this?

Done.

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.mm:23: void RenderProcessGone(WebState* web_state)
override {
On 2017/02/01 16:43:04, Eugene But wrote:
> Optional nit: Drop |web_state| argument

GlobalWebStateObserver needs to pass this, so that individual observers can know
which WebState crashed.

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.mm:29: : web_client_(base::WrapUnique(new
TestWebClient)) {
On 2017/02/01 16:43:04, Eugene But wrote:
> Do we need web client? WebTestSuite already sets it:
>
https://cs.chromium.org/chromium/src/ios/web/test/web_test_suite.mm?type=cs&q...

Some tests call GetWebClient(), so leaving this in for now.

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.mm:54: if (allow) {
On 2017/02/01 16:43:04, Eugene But wrote:
> Optional nit: Do you want to use ternary?
> renderer_crash_observer_ = allow ? nullptr :
> base::MakeUnique<RendererCrashObserver>();

If you don't feel strongly, I'll keep this as-is.

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
File ios/web/test/web_test_unittest.mm (right):

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:5: #import <UIKit/UIKit.h>
On 2017/02/01 16:43:04, Eugene But wrote:
> nit: s/<UIKit/UIKit.h>/<WKWebKit/WKWebKit.h> ?
> Looks like this file does not use anything from UIKit but uses WKWebView

Done.

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:14: WKWebView* g_web_view = nil;
On 2017/02/01 16:43:04, Eugene But wrote:
> Should |g_web_view| be a member of WebTestFixtureTest? Can you just call
> |web::SimulateWKWebViewCrash(webView_);?

I'm still fighting with gtest on this.  EXPECT_FATAL_FAILURE() does something
weird internally that's preventing me from using member variables.

Ok, looks like I can work around this by using a static local variable instead.

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:26:
webView_.reset([web::BuildTerminatedWKWebView() retain]);
On 2017/02/01 16:43:04, Eugene But wrote:
> s/webView_/web_view_

Done.

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:28:
base::scoped_nsobject<TestWebViewContentView> webViewContentView(
On 2017/02/01 16:43:04, Eugene But wrote:
> s/webViewContentView/web_view_content_view

Done.

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:50: TEST_F(WebTestFixtureTest,
SucceedsOnRenderCrash) {
On 2017/02/01 16:43:04, Eugene But wrote:
> Please add comments

Done.

[commit-bot: I haz the power, 2017-02-01 18:23:24]

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

[rohitrao (ping after 24h), 2017-02-01 18:25:37]

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

[commit-bot: I haz the power, 2017-02-01 18:26:10]

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

[Eugene But (OOO till 7-30), 2017-02-01 18:41:45]

lgtm!

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
File ios/web/public/test/web_test.mm (right):

https://codereview.chromium.org/2666953003/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_test.mm:23: void RenderProcessGone(WebState* web_state)
override {
On 2017/02/01 18:22:59, rohitrao wrote:
> On 2017/02/01 16:43:04, Eugene But wrote:
> > Optional nit: Drop |web_state| argument
> 
> GlobalWebStateObserver needs to pass this, so that individual observers can
know
> which WebState crashed.
Sorry, what I meant was:
|void RenderProcessGone(WebState*) override {|

It's not important, so please ignore it :)

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
File ios/web/test/web_test_unittest.mm (right):

https://codereview.chromium.org/2666953003/diff/20001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:14: WKWebView* g_web_view = nil;
On 2017/02/01 18:23:00, rohitrao wrote:
> On 2017/02/01 16:43:04, Eugene But wrote:
> > Should |g_web_view| be a member of WebTestFixtureTest? Can you just call
> > |web::SimulateWKWebViewCrash(webView_);?
> 
> I'm still fighting with gtest on this.  EXPECT_FATAL_FAILURE() does something
> weird internally that's preventing me from using member variables.
> 
> Ok, looks like I can work around this by using a static local variable
instead.
Oh that's because they use local class, which can only access statics per C++
standard.

https://codereview.chromium.org/2666953003/diff/60001/ios/web/test/web_test_u...
File ios/web/test/web_test_unittest.mm (right):

https://codereview.chromium.org/2666953003/diff/60001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:37: static WKWebView* web_view = web_view_;
How about this comment?:
// EXPECT_FATAL_FAILURE() uses a local class which can not access non-static
variables of the enclosing function.

[commit-bot: I haz the power, 2017-02-01 19:31:41]

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

[commit-bot: I haz the power, 2017-02-01 19:31:42]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2017-02-01 20:29:14]

https://codereview.chromium.org/2666953003/diff/60001/ios/web/test/web_test_u...
File ios/web/test/web_test_unittest.mm (right):

https://codereview.chromium.org/2666953003/diff/60001/ios/web/test/web_test_u...
ios/web/test/web_test_unittest.mm:37: static WKWebView* web_view = web_view_;
On 2017/02/01 18:41:45, Eugene But wrote:
> How about this comment?:
> // EXPECT_FATAL_FAILURE() uses a local class which can not access non-static
> variables of the enclosing function.

Done.

[rohitrao (ping after 24h), 2017-02-01 20:30:24]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2017-02-01 20:30:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2666953003/#ps80001
(title: "Fix comment.")

[commit-bot: I haz the power, 2017-02-01 20:30:48]

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-02-01 21:31:22]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485981024922930,
"parent_rev": "f8aac2d7a75dbd08e7e91779f365bb54dbfabaaa", "commit_rev":
"caeca2dde86a85efeda94866988b3659f5951889"}

[commit-bot: I haz the power, 2017-02-01 21:33:06]

Description was changed from

==========
[ios] Automatically fail any WebTest whose render process crashes.

We speculate that some tests are flaky because iOS is killing WKWebViews while
the tests are running.  This CL adds a GlobalWebStateObserver method to listen
for this and explicitly fail the current test when a WKWebView dies.  Having an
explicit failure will make these cases more apparent when looking through test
logs.

This CL also adds an escape hatch for tests that intentionally crash the
renderer.

BUG=None
==========

to

==========
[ios] Automatically fail any WebTest whose render process crashes.

We speculate that some tests are flaky because iOS is killing WKWebViews while
the tests are running.  This CL adds a GlobalWebStateObserver method to listen
for this and explicitly fail the current test when a WKWebView dies.  Having an
explicit failure will make these cases more apparent when looking through test
logs.

This CL also adds an escape hatch for tests that intentionally crash the
renderer.

BUG=None

Review-Url: https://codereview.chromium.org/2666953003
Cr-Commit-Position: refs/heads/master@{#447615}
Committed:
https://chromium.googlesource.com/chromium/src/+/caeca2dde86a85efeda94866988b...
==========

[commit-bot: I haz the power, 2017-02-01 21:33:07]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/caeca2dde86a85efeda94866988b...