jkim-julie
commented
6 years ago @msisov, there is a crash with 'views_mus_unittests' on ChromeOS with X11DisplayManagerOzone.
This patch sets |native_mode| for DisplaySnapshot, because when there is no |selected_mode| for DisplayId, DisplayConfigurator takes |native_mode|. If there is no |selected_mode| or |native_mode|, GetDisplayLayout() is failed, DisplayConfigurator::OnConfigured() has no |success|, observer.OnDisplayModeChangeFailed() is called and it goes to 'ScreenManagerOzoneInternal::PostDisplayConfigurationChange()'. But |touch_transformcontroller| is not created yet on the sequence and we meet a crash.
Briefly, the sequence is below. Service::StartDisplayInit() ->ScreenManagerOzoneInternal::Init() [1] ->DisplayConfigurator::ForceInitialConfigure() (https://cs.chromium.org/chromium/src/services/ui/display/screen_manager_ozone_internal.cc?sq=package:chromium&q=screen_manager_ozone_&g=0&l=158) ->UpdateDisplayConfigurationTask::Run() ->X11NativeDisplayDelegate::GetDisplays() ->X11DisplayManagerOzone::GetDisplaysSnapshot() ->UpdateDisplayConfigurationTask::OnDisplaysUpdated() ->UpdateDisplayConfigurationTask::EnterState() (It calls 'DisplayConfigurator::DisplayLayoutManagerImpl::GetDisplayLayout()' but if |state->selected_mode| is empty, it returns false. https://cs.chromium.org/chromium/src/ui/display/manager/display_configurator.cc?sq=package:chromium&q=display_confi&g=0&l=279) ->UpdateDisplayConfigurationTask::OnStateEntered() ->FinishConfiguration() ->DisplayConfigurator::OnConfigured() ->NotifyDisplayStateObservers() ->DisplayChangeObserver::OnDisplayModeChangeFailed() ->DisplayChangeObserver::OnDisplayModeChanged() ->DisplayManager::OnNativeDisplaysChanged() ->DisplayManager::UpdateDisplaysWith() ->ScreenManagerOzoneInternal::PostDisplayConfigurationChange() (Crashed because it doesn't create |touch_transformcontroller| yet at [1].)
With this flow, It has 3 problems now. 1) DisplayMode should be in the list in DisplaySnapshot. 2) DisplaySnapshot should have native_mode at least. FakeDisplaySnapshot also has it. (https://cs.chromium.org/chromium/src/ui/display/manager/fake_display_snapshot.cc?sq=package:chromium&q=fake_display_sna&g=0&l=156) 3) Even if DisplayConfigurator gets fail, it sholdn't be crashed.
This patch fixes the problems above and it implements 'X11NativeDisplayDelegate::Configure', which has same behaviour as FakeDisplayDelegate::Configure().
PTAL.
msisov
This patch removed OwnedDisplaySnapshot because it doesn't need to own DisplayMode. DisplaySnapshot is created with DisplaySnapshot::DisplayModeList, which is the list has DisplayModes. Whenever DisplayMode is added to the list, it's cloned and kept in the list and every DisplayMode should be in the list.