Check failed: shelf_widget_ in tests on disconnecting display and locking screen |
|||||
Issue description
TEST_F(PersistentWindowControllerTest, ReconnectOnLockScreen) {
UpdateDisplay("0+0-500x500,0+501-500x500");
aura::Window* w1 =
CreateTestWindowInShellWithBounds(gfx::Rect(200, 0, 100, 200));
aura::Window* w2 =
CreateTestWindowInShellWithBounds(gfx::Rect(501, 0, 200, 100));
EXPECT_EQ(gfx::Rect(200, 0, 100, 200), w1->GetBoundsInScreen());
EXPECT_EQ(gfx::Rect(501, 0, 200, 100), w2->GetBoundsInScreen());
const int64_t primary_id = WindowTreeHostManager::GetPrimaryDisplayId();
const int64_t secondary_id = display_manager()->GetSecondaryDisplay().id();
display::ManagedDisplayInfo primary_info =
display_manager()->GetDisplayInfo(primary_id);
display::ManagedDisplayInfo secondary_info =
display_manager()->GetDisplayInfo(secondary_id);
// Disconnects secondary display.
std::vector<display::ManagedDisplayInfo> display_info_list;
display_info_list.push_back(primary_info);
display_manager()->OnNativeDisplaysChanged(display_info_list);
EXPECT_EQ(gfx::Rect(200, 0, 100, 200), w1->GetBoundsInScreen());
EXPECT_EQ(gfx::Rect(1, 0, 200, 100), w2->GetBoundsInScreen());
// Enters locked session state.
GetSessionControllerClient()->SetSessionState(SessionState::LOCKED);
}
Check stack trace: https://paste.googleplex.com/6577922559180800
,
Feb 9 2018
,
Feb 9 2018
So, Shelf and ShelfLockingManager outlive the ShelfWidget when the second display is removed? I wonder if the Shelf itself should be destroyed earlier, or if we need to handle that partially shutdown state. Maybe spinning a run loop after OnNativeDisplaysChanged would be enough to workaround this issue? Still, we should probably ensure the code works with this synchronous sequence of events.
,
Feb 9 2018
Yes, adding a run loop works. It should wait destroying ShelfLockingManager.
,
Feb 9 2018
Mike, can I assign this to you, to see if it needs investigation? Something I get is, ShelfWidget::Shutdown() calls before shelf deletion (which is called after RootWindowController's dtor. For the time being, I will use run loop (and cite this bug) to not block the test.
,
Feb 9 2018
I suppose that's okay, I might get a chance to investigate soon.
,
Feb 9 2018
I wouldn't destroy Shelf any earlier. I did a lot of work to make sure the lifetime of Shelf (really the "Shelf controller") matches the lifetime of the RootWindowController. warx, is this maybe due to an async mojo call? Maybe something needs FlushForTesting? The multi-phase shutdown of shelf stuff is unfortunately necessary due to the large volumes of code that reach back into Shelf, ShelfWidget, StatusAreaWidget, etc. at unusual times. I've tried to simplify it a few times, but it's really hard. msw, one thing I thought of that might help (in general) is to reset the ShelfLayoutManager in RWC::CloseAllChildWindows. It's got a lot of code that calls into Shelf/ShelfWidget. Maybe something could happen with ShelfLockingManager too. Feel free to assign to me if you think this is a teardown issue. Unfortunately I've stared at the shelf shutdown code many many times. :-P
,
Feb 9 2018
Per async mojo call, the code I introduced is pure ash implementation: https://chromium-review.googlesource.com/c/chromium/src/+/893644 By checking the stack trace, the only suspicious point is: ash::TestSessionControllerClient::SetSessionState() Adding: Shell::Get()->session_controller()->FlushMojoForTest() after SetSessionState() doesn't help either.
,
Feb 9 2018
,
Mar 2 2018
Update: There's a time period during shutdown where ShelfWidget exists, but its aura::Window NativeWidget is null. This happens after ShelfWidget::Shutdown() calls CloseNow(). It's super-confusing, because lots of code assumes that if the ShelfWidget is valid then its native widget is valid. I just got burned by this in some ChromeVox code. I think we need to untangle ShelfLayoutManager and ShelfWidget, per thoughts in #7. Something like https://chromium-review.googlesource.com/c/chromium/src/+/907390 might help. I can take a look if you're not feeling inspired to dig into shelf shutdown. :-) |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by warx@chromium.org
, Feb 9 2018