New issue
Advanced search Search tips

Issue 919707 link

Starred by 1 user
Status: Available
Owner: ----
Cc:
x...@chromium.org
derat@chromium.org
minch@chromium.org
omrilio@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

RunUntilIdle in ash_unittests exits tablet mode

Project Member Reported by osh...@chromium.org, Jan 8 
I found this while working on refactoring.
It looks like the following code eventually to emit the switch state event that exits the tablet mode in ash_unittests.
I'll try to fix the test so that it won't require RunUntilIdle, but it'd also be nice if we can somehow fix this behavior
because it can be confusing and can make a test flaky.

xdai@, derat@ do you have any suggestion?

https://cs.chromium.org/chromium/src/ash/wm/tablet_mode/tablet_mode_controller.cc?rcl=ccbba773dd5e3114ee88ef84a2dc5b0f30dcb3ad&l=146


#0 0x7f0c3797d51d base::debug::StackTrace::StackTrace()
#1 0x7f0c3767887a base::debug::StackTrace::StackTrace()
#2 0x7f0c3a0fad7b ash::TabletModeController::EnableTabletModeWindowManager()
#3 0x7f0c3a0fc45a ash::TabletModeController::AttemptLeaveTabletMode()
#4 0x7f0c3a0fd28e ash::TabletModeController::LidEventReceived()
#5 0x7f0c3a0fa4c2 ash::TabletModeController::OnGetSwitchStates()
#6 0x7f0c39b717b1 void base::internal::FunctorTraits<void (ash::DetachableBaseHandler::*)(base::Optional<chromeos::PowerManagerClient::SwitchStates>), void>::Invoke<void (ash::DetachableBaseHandler::*)(base::Optional<chromeos::PowerManagerClient::SwitchStates>), base::WeakPtr<ash::DetachableBaseHandler>, base::Optional<chromeos::PowerManagerClient::SwitchStates> >(void (ash::DetachableBaseHandler::*)(base::Optional<chromeos::PowerManagerClient::SwitchStates>), base::WeakPtr<ash::DetachableBaseHandler>&&, base::Optional<chromeos::PowerManagerClient::SwitchStates>&&)
#7 0x7f0c3a0ffec5 void base::internal::InvokeHelper<true, void>::MakeItSo<void (ash::TabletModeController::*)(base::Optional<chromeos::PowerManagerClient::SwitchStates>), base::WeakPtr<ash::TabletModeController>, base::Optional<chromeos::PowerManagerClient::SwitchStates> >(void (ash::TabletModeController::*&&)(base::Optional<chromeos::PowerManagerClient::SwitchStates>), base::WeakPtr<ash::TabletModeController>&&, base::Optional<chromeos::PowerManagerClient::SwitchStates>&&)
#8 0x7f0c3a0ffe40 void base::internal::Invoker<base::internal::BindState<void (ash::TabletModeController::*)(base::Optional<chromeos::PowerManagerClient::SwitchStates>), base::WeakPtr<ash::TabletModeController> >, void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>::RunImpl<void (ash::TabletModeController::*)(base::Optional<chromeos::PowerManagerClient::SwitchStates>), std::__Cr::tuple<base::WeakPtr<ash::TabletModeController> >, 0ul>(void (ash::TabletModeController::*&&)(base::Optional<chromeos::PowerManagerClient::SwitchStates>), std::__Cr::tuple<base::WeakPtr<ash::TabletModeController> >&&, std::__Cr::integer_sequence<unsigned long, 0ul>, base::Optional<chromeos::PowerManagerClient::SwitchStates>&&)
#9 0x7f0c3a0ffd79 base::internal::Invoker<base::internal::BindState<void (ash::TabletModeController::*)(base::Optional<chromeos::PowerManagerClient::SwitchStates>), base::WeakPtr<ash::TabletModeController> >, void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>::RunOnce(base::internal::BindStateBase*, base::Optional<chromeos::PowerManagerClient::SwitchStates>&&)
#10 0x55c0b5cf4a0f base::OnceCallback<void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>::Run(base::Optional<chromeos::PowerManagerClient::SwitchStates>) &&
#11 0x55c0b5cf498d void base::internal::FunctorTraits<base::OnceCallback<void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>, void>::Invoke<base::OnceCallback<void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>, chromeos::PowerManagerClient::SwitchStates>(base::OnceCallback<void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>&&, chromeos::PowerManagerClient::SwitchStates&&)
#12 0x55c0b5cf487d void base::internal::InvokeHelper<false, void>::MakeItSo<base::OnceCallback<void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>, chromeos::PowerManagerClient::SwitchStates>(base::OnceCallback<void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>&&, chromeos::PowerManagerClient::SwitchStates&&)
#13 0x55c0b5cf4840 void base::internal::Invoker<base::internal::BindState<base::OnceCallback<void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>, chromeos::PowerManagerClient::SwitchStates>, void ()>::RunImpl<base::OnceCallback<void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>, std::__Cr::tuple<chromeos::PowerManagerClient::SwitchStates>, 0ul>(base::OnceCallback<void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>&&, std::__Cr::tuple<chromeos::PowerManagerClient::SwitchStates>&&, std::__Cr::integer_sequence<unsigned long, 0ul>)
#14 0x55c0b5cf4789 base::internal::Invoker<base::internal::BindState<base::OnceCallback<void (base::Optional<chromeos::PowerManagerClient::SwitchStates>)>, chromeos::PowerManagerClient::SwitchStates>, void ()>::RunOnce(base::internal::BindStateBase*)
#15 0x7f0c3762a3ee base::OnceCallback<void ()>::Run() &&
#16 0x7f0c37679d2a base::debug::TaskAnnotator::RunTask()

Comment 1 by osh...@chromium.org, Jan 8

Summary: RunUntilIdle in ash_unittests exits tablet mode (was: RunUntilIdle in ash_unittests exists tablet mode)

Comment 2 by derat@chromium.org, Jan 8

Cc: minch@chromium.org
Hmm. If a test cares about whether the system is in tablet mode or not, we probably want it to ensure that FakePowerManagerClient (IIRC) is configured to return the correct state there.

If TabletModeController is initialized synchronously, maybe it should be changed to instead post a task that calls GetSwitchStates so tests will have time to configure FPMC first.

Comment 3 by osh...@chromium.org, Jan 9 

There are many tests that try to enter tablet mode using

TabletModeController::EnableTabletModeWindowManager.

Looking at the comment, it's probably better to have a method for test, that
locks the stat somehow, and make this private.

Comment 4 by x...@chromium.org, Today (12 hours ago) 

It seems to me that it might cause by the calling order of LidEventReceived() and TabletModeEventReceived() in TabletModeController::OnGetSwitchStates() function (https://cs.chromium.org/chromium/src/ash/wm/tablet_mode/tablet_mode_controller.cc?rcl=c79fd2419a918afd184dfb25c7c8a004b5295944&l=490). LidEventReceived() is called first and if tablet mode switch value if false, it will try to exit tablet mode. But the tablet mode switch value is guaranteed to be false at this moment as this value is only updated in TabletModeEventReceived() function which is called after LidEventReceived. 

I think it should make sense to switch to calling order of these two functions.

Comment 5 by x...@chromium.org, Today (12 hours ago) 

But the thing I don't understand is why tablet mode is enabled in the first place (and thus LidEventReceived() then exit tablet mode). Tablet mode should be disabled by default in the ctor of TabletModeController.

Comment 6 by osh...@chromium.org, Today (5 hours ago) 

The issue is that there are tests that enteres tablet mode by calling EnableTabletModeWindowManager.
►

Sign in to add a comment