New issue
Advanced search Search tips

Issue 881825 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 17
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug-Regression

Blocking:
issue 807428



Sign in to add a comment

TabUsageRecorderTestCase fails with WKBasedNavigationManager

Project Member Reported by danyao@chromium.org, Sep 7

Issue description

Failing 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
 
Labels: Type-Bug-Regression
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.
Forgot to mention, the above analysis is for TabUsageRecorderTestCase/testTabSwitchRecorder.
Labels: ReleaseBlock-Stable
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.
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Sign in to add a comment