Fixing a "use after free".

Fixing a "use after free".

BUG=NONE

Committed: https://crrev.com/05bc9c84e44660e9ba56f5566092228d3674679a
Cr-Commit-Position: refs/heads/master@{#300565}

[mfomitchev, 2014-10-14 20:37:07]

mfomitchev@chromium.org changed reviewers:
+ mohsen@chromium.org, sky@chromium.org

[mohsen, 2014-10-14 20:41:05]

Change in touch selection controller LGTM.

[sky, 2014-10-14 22:55:50]

https://codereview.chromium.org/651323002/diff/20001/ui/aura/window_event_dis...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/651323002/diff/20001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:381: if (details.dispatcher_destroyed ||
details.target_destroyed)
How about test coverage?

[mfomitchev, 2014-10-20 23:13:14]

Good point. I've added a unit test for WindowEventDispatcher. 
As for the change in touch_selection_controller_impl, I've talked with Mohsen
and he believes we shouldn't need to Set/Release capture on text selection
handles once we move to the unified touch selection logic, which should happen
pretty soon. I've logged crbug.com/425340 to ensure that this doesn't fall
through the cracks. Considering this logic should be going away, I'd rather not
write a unit test for touch_selection_controller_impl. Are you okay with that?

[sky, 2014-10-21 02:45:32]

https://codereview.chromium.org/651323002/diff/40001/ui/aura/window_event_dis...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/651323002/diff/40001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:381: if (details.dispatcher_destroyed ||
details.target_destroyed)
If target_destroyed is true, isn't the only thing we want to avoid calling
OnCaptureLost? In other words, shouldn't the if be around 384?

[mfomitchev, 2014-10-21 20:02:04]

I also reverted the changes in TouchSelectionControllerImpl since they break
another test. The real fix there is to fix the cursor client/--touch-devices
flag interaction. I will look at this separately.

https://codereview.chromium.org/651323002/diff/40001/ui/aura/window_event_dis...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/651323002/diff/40001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:381: if (details.dispatcher_destroyed ||
details.target_destroyed)
On 2014/10/21 02:45:32, sky wrote:
> If target_destroyed is true, isn't the only thing we want to avoid calling
> OnCaptureLost? In other words, shouldn't the if be around 384?

Done.

[sky, 2014-10-21 20:08:10]

https://codereview.chromium.org/651323002/diff/80001/ui/aura/window_event_dis...
File ui/aura/window_event_dispatcher_unittest.cc (right):

https://codereview.chromium.org/651323002/diff/80001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher_unittest.cc:2037: virtual void
OnMouseEvent(ui::MouseEvent* event) override {
When is a mouse event generated by the code you have that triggers destruction?
More generically, does your test fail without the patch?

[mfomitchev, 2014-10-21 20:35:33]

https://codereview.chromium.org/651323002/diff/80001/ui/aura/window_event_dis...
File ui/aura/window_event_dispatcher_unittest.cc (right):

https://codereview.chromium.org/651323002/diff/80001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher_unittest.cc:2037: virtual void
OnMouseEvent(ui::MouseEvent* event) override {
Yes, the test fails without the patch. The mouse event is the
ET_MOUSE_CAPTURE_CHANGED generated in WindowEventDispatcher::UpdateCapture
called from line 2100 of the test.

[sky, 2014-10-21 20:57:40]

LGTM

[mfomitchev, 2014-10-21 20:58:37]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-10-21 21:00:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/651323002/80001

[commit-bot: I haz the power, 2014-10-21 23:14:31]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-21 23:16:14]

Patchset 5 (id:??) landed as
https://crrev.com/05bc9c84e44660e9ba56f5566092228d3674679a
Cr-Commit-Position: refs/heads/master@{#300565}