New issue
Advanced search Search tips

Issue 921275 link

Starred by 1 user
Status: Assigned
Owner:
afakhry@chromium.org
Cc:
aaboagye@chromium.org
kaznacheev@chromium.org
osh...@chromium.org
nkolluru@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Bug in DisplayManager::UpdateDisplaysWith() may result in removing an existing display even though it was never removed.

Project Member Reported by afakhry@chromium.org, Jan 12 
DisplayManager::UpdateDisplaysWith() sorts the existing and new displays using the least significant 8-bits of the display IDs [1].

But the loop that compares the existing and new display info to determine which displays are added and which ones are removed uses the full display IDs, in particular this branch is wrong: `else if (curr_iter->id() < new_info_iter->id()) {` [2].

In practice I found this to happen while switching to docked mode. The external display is removed then re-added unnecessarily.

[1]: https://cs.chromium.org/chromium/src/ui/display/manager/display_manager.cc?q=DisplayManager::UpdateDisplaysWith&g=0&l=909-912

[2]: https://cs.chromium.org/chromium/src/ui/display/manager/display_manager.cc?q=DisplayManager::UpdateDisplaysWith&g=0&l=1012
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 17 (6 days ago) 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e031e0c90ee6d1e57907c58caa9aa8986283ad57

commit e031e0c90ee6d1e57907c58caa9aa8986283ad57
Author: Ahmed Fakhry <afakhry@chromium.org>
Date: Thu Jan 17 00:53:02 2019

Fix a bug that may result in the wrong removal of an external display

Iterating through the new and current display list to figure out
which new ones where added and which ones were removed used to use
a different comparator than the one used to sort those lists.
This sometimes caused an external display to be removed then added
unnecessarily when going to docked mode.

This CL fixes this issue.

BUG=921275
TEST=Added new test that fails without the fix.

Change-Id: Ie2a9bd93588c00070d6f458946127d0c97c63a20
Reviewed-on: https://chromium-review.googlesource.com/c/1416331
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623477}
[modify] https://crrev.com/e031e0c90ee6d1e57907c58caa9aa8986283ad57/ash/display/display_manager_unittest.cc
[modify] https://crrev.com/e031e0c90ee6d1e57907c58caa9aa8986283ad57/ui/display/manager/display_manager.cc

Comment 2 by nkolluru@google.com, Jan 19 (4 days ago)

Cc: aaboagye@chromium.org nkolluru@google.com
►

Sign in to add a comment