The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7d50f07ecaab93463b331ec54f3fc3b5d8c57382

commit 7d50f07ecaab93463b331ec54f3fc3b5d8c57382
Author: Jérôme Lebel <jlebel@chromium.org>
Date: Wed Jun 13 12:00:40 2018

[iOS] Disabling test in ExternalURLTabUsageRecorderTestCase

ExternalURLTabUsageRecorderTestCase.testEvictedTabReloadFailure always
fails, simulator and device.

"This site can’t be reached" can't be find (but it shown on the page).

Bug:  852341 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I6ccb5d93ecd470ed3ac5e01400fca1677ebfbfeb
TBR: eugenebut@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1098923
Reviewed-by: Jérôme Lebel <jlebel@chromium.org>
Commit-Queue: Jérôme Lebel <jlebel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566805}
[modify] https://crrev.com/7d50f07ecaab93463b331ec54f3fc3b5d8c57382/ios/chrome/browser/metrics/external_url_tab_usage_recorder_egtest.mm

Assigning to Olivier, who owns ios/chrome/browser/metrics

I don't think we should be fixing testEvictedTabReloadFailure however. This test is a gross misuse of EG testing framework, which is intended for end-to-end UI testing. Whatever is tested in testEvictedTabReloadFailure can be a unit test.

Hi eugene

The test fails because the CRWWebControllerContainerView now has two children WKWebView, the second one being the static html view.

Is this expected? Or should there always be one WKWebView per WebState?

Generally speaking CRWWebControllerContainerView is a private class and the tests should not rely on CRWWebControllerContainerView's subviews (actually there is a bug for that: crbug.com/773901).

Kurt, do you know if CRWWebControllerContainerView is allowed to have 2 subviews?

Yes, I understand that we should not depend on this.
But as I found odd to have two WKWebView for one WebState, I thought that maybe it could be a bug. 

No, that sounds like a bug in CRWWebControllerContainerView.  |-displayNativeContent:|, which is used to display static html views, clears out self.webViewContentView, which should have removed the WKWebView.

I wonder if this bug is caused by https://chromium-review.googlesource.com/1093018 ?

Possibly?  That CL is affecting the top-level CRWWebControllerContainerView, whereas this issue is caused by that container view's subviews.  Extending the lifetime of the view shouldn't change the behavior of |-displayNativeContent| for that view.

To solve the dependency issue, do you think we could allow 
|CRWWebController executeJavaScript:completionHandler:|

to call
|CRWNativeContent executeJavaScript:completionHandler:|
when it is implemented?
This would allow to merge 
waitForStaticHTMLViewContainingText and waitForWebViewContainingText

For the double WKWebView:
The tab is showing an error in a WKWebView.
Then it is evicted then reloaded.
It still contains the previous WKWebView which was not deleted, an aa new one for the new error page.

Re to comment #11: Why do we want to pipe executeJavaScript:completionHandler to CRWNativeContext? I'm working on removal CRWNativeContent usage for error pages, so error page will be displayed in WKWebView.

Re to comment #12: That sounds like a bug. Maybe something that was caused by Danyao's refactoring.

re to #13:
If you do that, I guess this will fix the test.
Do you know when this can be done?

I want to flip WebErrorPages flag for M69, but I'm aways busy with various web-related fires. Does this bug actually affect M68?

No.
If bug mentioned in #12 is fixed, it will also fix the test, so I guess this is the priority.

Thanks! Punting to M69 RBB, because this bug does not affect M68.

Pls apply appropriate OSs label. 

We should be able to rewrite this test to EG test w/o relying on external URLs (see ErrorPageTestCase as example). 

Pri-0 bugs are critical regressions or serious emergencies, and this bug has not been updated in three days. Could you please provide an update, or adjust the priority to a more appropriate level if applicable?

If a fix is in active development, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Hi all, should this truly block M69 beta? If so, is there an update? First beta is scheduled for next Wednesday 7/25. Thank you.

Moving to RBS as suggested in comment 17

This bug should track the failure of this test case on simulator, as well as on device as outlined in  issue 847948 .

I'll send out a cl to fix the in code crbug reference for testEvictedTabReloadFailure as well such that it will only point to this crbug as the tracker for the test being disabled.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f44a47ef657a9aefcf740c91866038624a1f0e32

commit f44a47ef657a9aefcf740c91866038624a1f0e32
Author: Lindsay Pasricha <lindsayw@google.com>
Date: Wed Jul 25 16:29:02 2018

Fix disable comment to point to only one tracker for both device and simulator failure.

Bug:  847948 , 852341 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I77f63093d642a51193787641568a639f80d5c5e1
Reviewed-on: https://chromium-review.googlesource.com/1148796
Commit-Queue: Lindsay Pasricha <lindsayw@chromium.org>
Reviewed-by: Mohammad Refaat <mrefaat@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577925}
[modify] https://crrev.com/f44a47ef657a9aefcf740c91866038624a1f0e32/ios/chrome/browser/metrics/external_url_tab_usage_recorder_egtest.mm

We can't explain what this test is trying to verify. It may be to:
- verify that WKWebViews would get evicted and re-rendered,
- verify that Error Pages are not (or are) evicted.

The underlying code changed from UIWebView to WKWebView to non-modal incognito.
And the test itself transmogrified from KIF to Earl Grey tests. The genealogy can no longer be followed. I propose that we delete this external_url_tab_usage_recorder_egtest.mm and rely on the unit tests in tab_usage_recorder_unittest.mm.

If we want additional coverage, I would rather add some unit tests that we actually understand. Fixing tests that we no longer understand is a fruitless exercise.

Filed issue 873261 to track issue pointed out in comment 5, etc.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f7fafa75c2080ef3a234472c8926be2ade8b04b

commit 7f7fafa75c2080ef3a234472c8926be2ade8b04b
Author: Peter K. Lee <pkl@chromium.org>
Date: Fri Aug 10 21:32:38 2018

Removed ExternalURLTabUsageRecorderTestCase

Removed ExternalURLTabUsageRecorderTestCase since it contains only one
disabled Earl Grey test. After some evaluation of the intention of the
test, it is not clear what purpose it serves/served or whether the code
being tested isn't already covered by unit tests.

Bug:  852341 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I988f4fc5512b40f331db565af2424e03b5c23b79
Reviewed-on: https://chromium-review.googlesource.com/1171365
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Peter Lee <pkl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582342}
[modify] https://crrev.com/7f7fafa75c2080ef3a234472c8926be2ade8b04b/ios/chrome/browser/metrics/BUILD.gn
[delete] https://crrev.com/c20cee27cd645da5b90c992546aa2cb8ae52d8d1/ios/chrome/browser/metrics/external_url_tab_usage_recorder_egtest.mm
[modify] https://crrev.com/7f7fafa75c2080ef3a234472c8926be2ade8b04b/ios/chrome/test/earl_grey/BUILD.gn

kariahda: This deletes a test that has already been disabled. I don't think there's any value in cherrypicking this change. Let me know if you think otherwise.

[Auto-generated comment by a script] We noticed that this issue is targeted for M-69; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-69 label, otherwise remove Merge-TBD label. Thanks.

Per c#28 are there any additional unit tests that should be added to cover the deleted EG tests?

Existing unit tests in TabUsageRecorderTest:
https://cs.chromium.org/chromium/src/ios/chrome/browser/metrics/tab_usage_recorder_unittest.mm

Existing EG tests:
https://cs.chromium.org/chromium/src/ios/chrome/browser/metrics/tab_usage_recorder_egtest.mm

Ok, I was referring to this part of c#28 "If we want additional coverage, I would rather add some unit tests..."

If you think the current tests are sufficient, then this is fine. No merge needed.

Ack. Current tests are sufficient.