Fix use after free for cached_displays_.

Fix use after free for cached_displays_.

There is a gap between when DisplaySnapshots get freed and when
DisplayConfigurator gets an updated list of DisplaySnapshots.
DisplayChangeObserver would try to use the pointers to freed memory if
the touch input-devices changed which might crash chrome.

Erase all entries in DisplayConfigurator::cached_displays_ before the
memory is freed.  It looks like DisplayConfigurator::cached_displays_ is
only used as part of either display configuration or for associating
touchscreens. There shouldn't be any other display configuration
happening between when cached_displays_ is freed and when new display
snapshots are added in DisplayConfigurator::OnConfigured(). Add a check
in DisplayChangeObserver::OnTouchscreenDeviceConfigurationChanged() if
there are no display snapshots then don't do anything to avoid passing
an empty list of ManagedDisplayInfos to DisplayManager.

BUG=669226
Committed: https://crrev.com/3f0ff6fe910fd0a4bbf487a9e411148fee83521d
Cr-Commit-Position: refs/heads/master@{#438518}

[kylechar, 2016-12-12 18:17:53]

kylechar@chromium.org changed reviewers:
+ dnicoara@chromium.org, oshima@chromium.org, rjkroege@chromium.org

[rjkroege, 2016-12-12 18:43:59]

Seems like a good solution given the constraints.

lgtm

[oshima, 2016-12-13 17:43:28]

change lg.

would you mind adding a test? Can you also double check if the content
protection/color calibration stuff are properly updated when reconnected?

[kylechar, 2016-12-13 21:51:49]

On 2016/12/13 17:43:28, oshima wrote:
> change lg.
> 
> would you mind adding a test? Can you also double check if the content
> protection/color calibration stuff are properly updated when reconnected?

Testing the Ozone DRM code requires running with Ozone DRM. I don't think that
is possible with existing Chrome infra unfortunately.

Beyond testing Ozone DRM I guess the most important part is making sure that
erasing |cached_displays_| doesn't have any weird side effects. I've modified
TestNativeDisplayDelegate and DisplayConfigurator. Now in
DisplayConfiguratorTests, DisplayConfigurator will receive
OnDisplaySnapshotsInvalidated() whenever it calls NDD::GetDisplays() in the same
way it would with Ozone DRM. The tests still pass without problems.

Both content protection and color profile information isn't stored on the
DisplaySnapshot objects. As far as I can tell, this change should have no effect
on either. The information isn't being erased, either before or after this
change.

[kylechar, 2016-12-13 21:55:22]

Patchset #2 (id:20001) has been deleted

[oshima, 2016-12-13 22:42:42]

lgtm

On 2016/12/13 21:51:49, kylechar wrote:
> On 2016/12/13 17:43:28, oshima wrote:
> > change lg.
> > 
> > would you mind adding a test? Can you also double check if the content
> > protection/color calibration stuff are properly updated when reconnected?
> 
> Testing the Ozone DRM code requires running with Ozone DRM. I don't think that
> is possible with existing Chrome infra unfortunately.
> 
> Beyond testing Ozone DRM I guess the most important part is making sure that
> erasing |cached_displays_| doesn't have any weird side effects. I've modified
> TestNativeDisplayDelegate and DisplayConfigurator. Now in
> DisplayConfiguratorTests, DisplayConfigurator will receive
> OnDisplaySnapshotsInvalidated() whenever it calls NDD::GetDisplays() in the
same
> way it would with Ozone DRM. The tests still pass without problems.
> 
> Both content protection and color profile information isn't stored on the
> DisplaySnapshot objects. As far as I can tell, this change should have no
effect
> on either. The information isn't being erased, either before or after this
> change.

Thank you for updating the test harness and double checking content
protection&color profile.

[kylechar, 2016-12-14 13:56:10]

The CQ bit was checked by kylechar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 13:56:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 15:04:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 15:04:17]

Dry run: This issue passed the CQ dry run.

[kylechar, 2016-12-14 15:12:17]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-12-14 15:12:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org
Link to the patchset: https://codereview.chromium.org/2571543002/#ps40001
(title: "Add invalidate to tests.")

[commit-bot: I haz the power, 2016-12-14 15:12:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 15:16:25]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481728337677240,
"parent_rev": "28e34288c394d84073209b5afab5c1b45c54e064", "commit_rev":
"54fb36895e96ffce318b52c4fbeffa06a70991df"}

[commit-bot: I haz the power, 2016-12-14 15:16:55]

Description was changed from

==========
Fix use after free for cached_displays_.

There is a gap between when DisplaySnapshots get freed and when
DisplayConfigurator gets an updated list of DisplaySnapshots.
DisplayChangeObserver would try to use the pointers to freed memory if
the touch input-devices changed which might crash chrome.

Erase all entries in DisplayConfigurator::cached_displays_ before the
memory is freed.  It looks like DisplayConfigurator::cached_displays_ is
only used as part of either display configuration or for associating
touchscreens. There shouldn't be any other display configuration
happening between when cached_displays_ is freed and when new display
snapshots are added in DisplayConfigurator::OnConfigured(). Add a check
in DisplayChangeObserver::OnTouchscreenDeviceConfigurationChanged() if
there are no display snapshots then don't do anything to avoid passing
an empty list of ManagedDisplayInfos to DisplayManager.

BUG=669226
==========

to

==========
Fix use after free for cached_displays_.

There is a gap between when DisplaySnapshots get freed and when
DisplayConfigurator gets an updated list of DisplaySnapshots.
DisplayChangeObserver would try to use the pointers to freed memory if
the touch input-devices changed which might crash chrome.

Erase all entries in DisplayConfigurator::cached_displays_ before the
memory is freed.  It looks like DisplayConfigurator::cached_displays_ is
only used as part of either display configuration or for associating
touchscreens. There shouldn't be any other display configuration
happening between when cached_displays_ is freed and when new display
snapshots are added in DisplayConfigurator::OnConfigured(). Add a check
in DisplayChangeObserver::OnTouchscreenDeviceConfigurationChanged() if
there are no display snapshots then don't do anything to avoid passing
an empty list of ManagedDisplayInfos to DisplayManager.

BUG=669226

Review-Url: https://codereview.chromium.org/2571543002
==========

[commit-bot: I haz the power, 2016-12-14 15:16:55]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-12-14 15:18:31]

Description was changed from

==========
Fix use after free for cached_displays_.

There is a gap between when DisplaySnapshots get freed and when
DisplayConfigurator gets an updated list of DisplaySnapshots.
DisplayChangeObserver would try to use the pointers to freed memory if
the touch input-devices changed which might crash chrome.

Erase all entries in DisplayConfigurator::cached_displays_ before the
memory is freed.  It looks like DisplayConfigurator::cached_displays_ is
only used as part of either display configuration or for associating
touchscreens. There shouldn't be any other display configuration
happening between when cached_displays_ is freed and when new display
snapshots are added in DisplayConfigurator::OnConfigured(). Add a check
in DisplayChangeObserver::OnTouchscreenDeviceConfigurationChanged() if
there are no display snapshots then don't do anything to avoid passing
an empty list of ManagedDisplayInfos to DisplayManager.

BUG=669226

Review-Url: https://codereview.chromium.org/2571543002
==========

to

==========
Fix use after free for cached_displays_.

There is a gap between when DisplaySnapshots get freed and when
DisplayConfigurator gets an updated list of DisplaySnapshots.
DisplayChangeObserver would try to use the pointers to freed memory if
the touch input-devices changed which might crash chrome.

Erase all entries in DisplayConfigurator::cached_displays_ before the
memory is freed.  It looks like DisplayConfigurator::cached_displays_ is
only used as part of either display configuration or for associating
touchscreens. There shouldn't be any other display configuration
happening between when cached_displays_ is freed and when new display
snapshots are added in DisplayConfigurator::OnConfigured(). Add a check
in DisplayChangeObserver::OnTouchscreenDeviceConfigurationChanged() if
there are no display snapshots then don't do anything to avoid passing
an empty list of ManagedDisplayInfos to DisplayManager.

BUG=669226

Committed: https://crrev.com/3f0ff6fe910fd0a4bbf487a9e411148fee83521d
Cr-Commit-Position: refs/heads/master@{#438518}
==========

[commit-bot: I haz the power, 2016-12-14 15:18:32]

Patchset 2 (id:??) landed as
https://crrev.com/3f0ff6fe910fd0a4bbf487a9e411148fee83521d
Cr-Commit-Position: refs/heads/master@{#438518}