Fix ios_chrome_unittests to crash on DCHECK failure.

Fix ios_chrome_unittests to crash on DCHECK failure.

In CollectionViewModelTest.InvalidIndexPath, the global assert logger
was overridden to ignore assertion but never restored to the default
value, causing all subsequent DCHECK failures to be ignored.

This CL fixes this incorrect behaviour by restoring the global assert
logger at the end of the test execution and by fixing all the tests
that were silently broken.

List of individual test fixes:

- introduce an helper class to wait for the PersonalDataManager async
  tasks to complete and use it in autofill tests interacting with the
  PersonalDataManager (without the wait, the SQL database is closed
  while some operations are pending and they fails with NOTREACHED()
  when they finally get scheduled)

- remove individual registration of a SingleThreadTaskRunner with the
  metrics system as there is no way to unregister the task runner once
  it is set and instead register one globally in IOSChromeUnitTestSuite

- add a function to reset the global hidden startup attemp count as the
  CrashLoopDetectionUtilTest.FullCycle test expect the hidden value to
  be uninitialised which is not the case (as some test do initialise it
  as a side-effect of creating the AppState)

- register a local state with the TestingApplicationContext as production
  code exercised by PrivacyCollectionViewControllerTest.TestModel test
  expect to register preference listener on the local state

BUG=650811
Committed: https://crrev.com/37302ec073c605e19030b0d58a0bd0b6a30d0434
Cr-Commit-Position: refs/heads/master@{#440413}

[sdefresne, 2016-12-21 18:36:25]

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

[sdefresne, 2016-12-21 18:36:48]

sdefresne@chromium.org changed reviewers:
+ rohitrao@chromium.org

[sdefresne, 2016-12-21 18:36:49]

Please take a look and send to CQ if LGTY.

[commit-bot: I haz the power, 2016-12-21 18:36:54]

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

[sdefresne, 2016-12-21 18:38:35]

I know that this CL is large, but it only touches test code (except for the
newly added method ResetFailedStartupAttemptCountForTests).

[sdefresne, 2016-12-21 18:46:00]

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

[commit-bot: I haz the power, 2016-12-21 18:46:21]

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, 2016-12-21 19:55:40]

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

[commit-bot: I haz the power, 2016-12-21 19:55:41]

Dry run: This issue passed the CQ dry run.

[justincohen, 2016-12-21 20:03:07]

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

[justincohen, 2016-12-21 20:03:08]

ios/chrome/browser/crash* LGTM

https://codereview.chromium.org/2592173002/diff/20001/ios/chrome/browser/cras...
File ios/chrome/browser/crash_loop_detection_util.h (right):

https://codereview.chromium.org/2592173002/diff/20001/ios/chrome/browser/cras...
ios/chrome/browser/crash_loop_detection_util.h:28: // Resests the hidden state
of failed startup attempt count for testing.
Resets

[sdefresne, 2016-12-22 12:23:18]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-12-22 12:23:21]

The CQ bit was unchecked by sdefresne@chromium.org

[sdefresne, 2016-12-22 12:47:11]

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

[commit-bot: I haz the power, 2016-12-22 12:47:22]

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

[sdefresne, 2016-12-22 12:53:43]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-12-22 12:53:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
justincohen@chromium.org
Link to the patchset: https://codereview.chromium.org/2592173002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-22 12:53:55]

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

[sdefresne, 2016-12-22 12:54:22]

https://codereview.chromium.org/2592173002/diff/20001/ios/chrome/browser/cras...
File ios/chrome/browser/crash_loop_detection_util.h (right):

https://codereview.chromium.org/2592173002/diff/20001/ios/chrome/browser/cras...
ios/chrome/browser/crash_loop_detection_util.h:28: // Resests the hidden state
of failed startup attempt count for testing.
On 2016/12/21 20:03:08, justincohen wrote:
> Resets

Done.

[noyau (Ping after 24h), 2016-12-22 13:17:22]

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

[noyau (Ping after 24h), 2016-12-22 13:17:23]

lgtm

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/collection_view_model_unittest.mm
(right):

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/collection_view_model_unittest.mm:305:
logging::SetLogAssertHandler(&LogSink);
Suggestions: This would probably be better is handled via a scoped object
resetting the log handler to its previous value when deallocated. Could be done
in a separate CL.

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/autofill_collection_view_controller_unittest.mm
(right):

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/autofill_collection_view_controller_unittest.mm:57:
observer.Wait();
I would suggest a comment on all uses of Wait().

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/personal_data_manager_data_changed_observer.cc
(right):

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/personal_data_manager_data_changed_observer.cc:25:
run_loop_.QuitWhenIdle();
Any risks of this not being called at all?

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/personal_data_manager_data_changed_observer.h
(right):

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/personal_data_manager_data_changed_observer.h:31:
void Wait();
Need comment.

[sdefresne, 2016-12-22 13:22:01]

The CQ bit was unchecked by sdefresne@chromium.org

[sdefresne, 2016-12-22 13:27:54]

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

[sdefresne, 2016-12-22 13:27:55]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-12-22 13:27:56]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org,
justincohen@chromium.org
Link to the patchset: https://codereview.chromium.org/2592173002/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2016-12-22 13:28:09]

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

[sdefresne, 2016-12-22 13:28:12]

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/collection_view_model_unittest.mm
(right):

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/collection_view_model_unittest.mm:305:
logging::SetLogAssertHandler(&LogSink);
On 2016/12/22 13:17:22, noyau wrote:
> Suggestions: This would probably be better is handled via a scoped object
> resetting the log handler to its previous value when deallocated. Could be
done
> in a separate CL.

I agree (there is a similar scoped object in net/test/ but it install a
different assert handler). I'll do it in a followup CL.

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/autofill_collection_view_controller_unittest.mm
(right):

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/autofill_collection_view_controller_unittest.mm:57:
observer.Wait();
On 2016/12/22 13:17:22, noyau wrote:
> I would suggest a comment on all uses of Wait().  

Done.

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/personal_data_manager_data_changed_observer.cc
(right):

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/personal_data_manager_data_changed_observer.cc:25:
run_loop_.QuitWhenIdle();
On 2016/12/22 13:17:22, noyau wrote:
> Any risks of this not being called at all?

If this is not called, it means that no method causing a modification of the
PersonalDataManager was called since the creation of this object. This would
indicate a bug in the test (either it should not wait for a modification or it
should create the object before asking for the modification).

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/personal_data_manager_data_changed_observer.h
(right):

https://codereview.chromium.org/2592173002/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/personal_data_manager_data_changed_observer.h:31:
void Wait();
On 2016/12/22 13:17:22, noyau wrote:
> Need comment.

Done.

[commit-bot: I haz the power, 2016-12-22 14:28:06]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482413275997330,
"parent_rev": "94965f6d9e080c3762bebb35b4a8f2140ff4bc1d", "commit_rev":
"448fe7a3a0c348615d159abc3fcf5917a37c3921"}

[commit-bot: I haz the power, 2016-12-22 14:28:35]

Description was changed from

==========
Fix ios_chrome_unittests to crash on DCHECK failure.

In CollectionViewModelTest.InvalidIndexPath, the global assert logger
was overridden to ignore assertion but never restored to the default
value, causing all subsequent DCHECK failures to be ignored.

This CL fixes this incorrect behaviour by restoring the global assert
logger at the end of the test execution and by fixing all the tests
that were silently broken.

List of individual test fixes:

- introduce an helper class to wait for the PersonalDataManager async
  tasks to complete and use it in autofill tests interacting with the
  PersonalDataManager (without the wait, the SQL database is closed
  while some operations are pending and they fails with NOTREACHED()
  when they finally get scheduled)

- remove individual registration of a SingleThreadTaskRunner with the
  metrics system as there is no way to unregister the task runner once
  it is set and instead register one globally in IOSChromeUnitTestSuite

- add a function to reset the global hidden startup attemp count as the
  CrashLoopDetectionUtilTest.FullCycle test expect the hidden value to
  be uninitialised which is not the case (as some test do initialise it
  as a side-effect of creating the AppState)

- register a local state with the TestingApplicationContext as production
  code exercised by PrivacyCollectionViewControllerTest.TestModel test
  expect to register preference listener on the local state

BUG=650811
==========

to

==========
Fix ios_chrome_unittests to crash on DCHECK failure.

In CollectionViewModelTest.InvalidIndexPath, the global assert logger
was overridden to ignore assertion but never restored to the default
value, causing all subsequent DCHECK failures to be ignored.

This CL fixes this incorrect behaviour by restoring the global assert
logger at the end of the test execution and by fixing all the tests
that were silently broken.

List of individual test fixes:

- introduce an helper class to wait for the PersonalDataManager async
  tasks to complete and use it in autofill tests interacting with the
  PersonalDataManager (without the wait, the SQL database is closed
  while some operations are pending and they fails with NOTREACHED()
  when they finally get scheduled)

- remove individual registration of a SingleThreadTaskRunner with the
  metrics system as there is no way to unregister the task runner once
  it is set and instead register one globally in IOSChromeUnitTestSuite

- add a function to reset the global hidden startup attemp count as the
  CrashLoopDetectionUtilTest.FullCycle test expect the hidden value to
  be uninitialised which is not the case (as some test do initialise it
  as a side-effect of creating the AppState)

- register a local state with the TestingApplicationContext as production
  code exercised by PrivacyCollectionViewControllerTest.TestModel test
  expect to register preference listener on the local state

BUG=650811

Review-Url: https://codereview.chromium.org/2592173002
==========

[commit-bot: I haz the power, 2016-12-22 14:28:37]

Committed patchset #4 (id:60001)

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

Description was changed from

==========
Fix ios_chrome_unittests to crash on DCHECK failure.

In CollectionViewModelTest.InvalidIndexPath, the global assert logger
was overridden to ignore assertion but never restored to the default
value, causing all subsequent DCHECK failures to be ignored.

This CL fixes this incorrect behaviour by restoring the global assert
logger at the end of the test execution and by fixing all the tests
that were silently broken.

List of individual test fixes:

- introduce an helper class to wait for the PersonalDataManager async
  tasks to complete and use it in autofill tests interacting with the
  PersonalDataManager (without the wait, the SQL database is closed
  while some operations are pending and they fails with NOTREACHED()
  when they finally get scheduled)

- remove individual registration of a SingleThreadTaskRunner with the
  metrics system as there is no way to unregister the task runner once
  it is set and instead register one globally in IOSChromeUnitTestSuite

- add a function to reset the global hidden startup attemp count as the
  CrashLoopDetectionUtilTest.FullCycle test expect the hidden value to
  be uninitialised which is not the case (as some test do initialise it
  as a side-effect of creating the AppState)

- register a local state with the TestingApplicationContext as production
  code exercised by PrivacyCollectionViewControllerTest.TestModel test
  expect to register preference listener on the local state

BUG=650811

Review-Url: https://codereview.chromium.org/2592173002
==========

to

==========
Fix ios_chrome_unittests to crash on DCHECK failure.

In CollectionViewModelTest.InvalidIndexPath, the global assert logger
was overridden to ignore assertion but never restored to the default
value, causing all subsequent DCHECK failures to be ignored.

This CL fixes this incorrect behaviour by restoring the global assert
logger at the end of the test execution and by fixing all the tests
that were silently broken.

List of individual test fixes:

- introduce an helper class to wait for the PersonalDataManager async
  tasks to complete and use it in autofill tests interacting with the
  PersonalDataManager (without the wait, the SQL database is closed
  while some operations are pending and they fails with NOTREACHED()
  when they finally get scheduled)

- remove individual registration of a SingleThreadTaskRunner with the
  metrics system as there is no way to unregister the task runner once
  it is set and instead register one globally in IOSChromeUnitTestSuite

- add a function to reset the global hidden startup attemp count as the
  CrashLoopDetectionUtilTest.FullCycle test expect the hidden value to
  be uninitialised which is not the case (as some test do initialise it
  as a side-effect of creating the AppState)

- register a local state with the TestingApplicationContext as production
  code exercised by PrivacyCollectionViewControllerTest.TestModel test
  expect to register preference listener on the local state

BUG=650811

Committed: https://crrev.com/37302ec073c605e19030b0d58a0bd0b6a30d0434
Cr-Commit-Position: refs/heads/master@{#440413}
==========

[commit-bot: I haz the power, 2016-12-22 14:31:13]

Patchset 4 (id:??) landed as
https://crrev.com/37302ec073c605e19030b0d58a0bd0b6a30d0434
Cr-Commit-Position: refs/heads/master@{#440413}