Issue metadata
Sign in to add a comment
|
TabUsageRecorderTestCase fails with WKBasedNavigationManager |
||||||||||||||||||||||
Issue descriptionFailing tests with --enable-features=SlimNavigationManager: testEvictedTabReloadSettingsAndBack testEvictedTabReloadSuccess testBackgroundingReloadCount testPageLoadCountBeforeEvictedTab testLinkClickNavigation testEvictedTabSlowReload testColdLaunchReloadCount testEvictedTabReloadSwitchToNTP testEvictedTabReloadBackgrounded testTabSwitchRecorder testPageRedirect All failures are due to the expected UMA counter not existing, e.g. TabUsageRecorderTestCase/testEvictedTabReloadSettingsAndBack: ../../ios/chrome/browser/metrics/tab_usage_recorder_egtest.mm:478: error: -[TabUsageRecorderTestCase testEvictedTabReloadSettingsAndBack] : Exception: GenericFailureException Exception Name: GenericFailureException Exception Reason: Histogram Tab.RestoreUserPersistence does not exist
,
Sep 7
This is related to crbug.com/619971. WebStateImpl::IsEvicted(), when used after chrome_test_util::EvictOtherTabModelTests(), behaves differently if WKBasedNavigationManager is used. Here's the problem: what WebStateImpl::IsEvicted() is really doing is checking that -[CRWWebController viewIsAlive] is false. But when EvictOtherTabModelTests() runs, it calls SetWebUsageEnabled(false) followed by SetWebUsageEnabled(true). The latter triggers session restore in WKBasedNavigationManager, which creates the WKWebView. So when WebStateImpl::IsEvicted() is called later, it returns false (because there is a view), when the test expects true. WebStateImpl::IsEvicted() is called in TabUsageRecorder::WebStateAlreadyEvicted() to update Tab.StatusWhenSwitchedBackToForeground UMA metric. The above behavior causes a sample to be recorded in the IN_MEMORY bucket instead of EVICTED bucket.
,
Sep 7
Forgot to mention, the above analysis is for TabUsageRecorderTestCase/testTabSwitchRecorder.
,
Sep 12
,
Sep 13
All the other failures are caused by the same reason: SlimNav reloads evicted tab during SetWebUsageEnabled(true). This recreates web view before OpenNewIncognitoTabUsingUIAndEvictMainTabs() returns, causing all web_states to be considered IN_MEMORY right after this call when the tests expect the web states to be EVICTED. The navigations triggered by the restore also messes up metrics that are collected in RecordPageLoadDone(). Restoring session in WebStateImpl::SetWebUsageEnabled(true) was initially introduced to fix crbug/781916. Since then, WKBasedNavigationManagerImpl gained detached mode and the ability to restore session when LoadIfNecessary() is called in detached mode. This actually makes the restore logic in WebStateImpl::SetWebUsageEnabled(true) unnecessary, as long as NavigationManager::LoadIfNecessary() is triggered afterwards. This should be the case usually. I'll run through CQ to see if there is any edge cases.
,
Sep 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bc6e5024e81e73ca5a0c09d13a66027b7ed26b7c commit bc6e5024e81e73ca5a0c09d13a66027b7ed26b7c Author: Danyao Wang <danyao@chromium.org> Date: Mon Sep 17 21:17:30 2018 [Nav Experiment] Remove session restore from SetWebUsageEnabled. WKBasedNavigationManagerImpl now has the ability to automatically restore session if LoadIfNecessary() is called when the navigation manager is in DETACHED mode (i.e. not having a WKWebView). As long as all use cases of SetWebUsageEnabled(true) is followed by a LoadIfNecessary(), the session restore in SetWebUsageEnabled() is redundant. This is always the case in production code paths. Restoring session in SetWebUsageEnabled() broke TabUsageRecorderTestCase because this test toggles SetWebUsageEnabled() to simulate tab eviction. Restoring session in SetWebUsageEnabled(true) triggers the reload events of the evicted tab before SwitchToNormalMode() is called and breaks metrics collection. This is not a problem in production because WKWebView eviction is triggered by iOS system and does not actually exercise the SetWebUsageEnabled() code path. With this CL, all TabUsageRecorderTestCase pass again. Bug: 881825 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs Change-Id: I81ebe71ff939d139f0eeca17ec0213bba1648447 Reviewed-on: https://chromium-review.googlesource.com/1225862 Reviewed-by: Eugene But <eugenebut@chromium.org> Commit-Queue: Danyao Wang <danyao@chromium.org> Cr-Commit-Position: refs/heads/master@{#591817} [modify] https://crrev.com/bc6e5024e81e73ca5a0c09d13a66027b7ed26b7c/ios/web/web_state/ui/crw_web_controller.h [modify] https://crrev.com/bc6e5024e81e73ca5a0c09d13a66027b7ed26b7c/ios/web/web_state/ui/crw_web_controller_unittest.mm [modify] https://crrev.com/bc6e5024e81e73ca5a0c09d13a66027b7ed26b7c/ios/web/web_state/web_state_impl.h [modify] https://crrev.com/bc6e5024e81e73ca5a0c09d13a66027b7ed26b7c/ios/web/web_state/web_state_impl.mm [modify] https://crrev.com/bc6e5024e81e73ca5a0c09d13a66027b7ed26b7c/ios/web/web_state/web_state_unittest.mm
,
Sep 17
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by danyao@chromium.org
, Sep 7