WAI. The file descriptor is still closed if close returns ENODEV.

From davidben@ on https://bugs.chromium.org/p/chromium/issues/detail?id=660960#c34:

> And EventConverterEvdev does not care about the close() error.

It probably should. :-) At a glance, I see a double-close. Both parent and base class try to close that fd.

https://cs.chromium.org/chromium/src/ui/events/ozone/evdev/event_converter_evdev_impl.cc?rcl=0&l=53
https://cs.chromium.org/chromium/src/ui/events/ozone/evdev/event_converter_evdev.cc?rcl=1484655952&l=34

=====

spang@, double close on a fd is like double free a buffer and could result in undefined behavior. Could you double check?

I agree double close causes a serious race but it would result in EBADF, not ENODEV.

I don't believe ENODEV indicates an issue, and that error code should not cause a crash.

The class that caused your crash doesn't even have the double close bug. Retitled to the double close bug noticed by inspection, thanks davidben@.

Thanks spang@ for taking care of this.

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

commit 13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1
Author: spang <spang@chromium.org>
Date: Wed Jan 18 23:36:10 2017

Fix double-close in EventConverterEvdevImpl

This fixes a double close that occurs when a keyboard or mouse is
unplugged from the system, which will cause breakage if there is a
concurrent open(). It also switches to a scoped file descriptor wrapper,
with a special close function because close() can return ENODEV on an
input device that was unplugged.

BUG= 681865 , 660960
TEST=events_unittests

Review-Url: https://codereview.chromium.org/2639043002
Cr-Commit-Position: refs/heads/master@{#444544}

[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/BUILD.gn
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/event_converter_evdev.cc
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/event_converter_evdev_impl.cc
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/event_converter_evdev_impl.h
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/event_converter_evdev_impl_unittest.cc
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/input_device_factory_evdev.cc
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.cc
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.h
[add] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/scoped_input_device.cc
[add] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/scoped_input_device.h
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/tablet_event_converter_evdev.cc
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/tablet_event_converter_evdev.h
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/tablet_event_converter_evdev_unittest.cc
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/touch_event_converter_evdev.cc
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/touch_event_converter_evdev.h
[modify] https://crrev.com/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1/ui/events/ozone/evdev/touch_event_converter_evdev_unittest.cc

The fix alleviates the fd problems greatly. We had about 20 crashes per day on canary before the fix. Canary starts to serve 2990 yesterday and so far we don't have any.

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_ChromeOS%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27browser%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27%5BAssert%5D%20CloseOverride%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D#samplereports:5,+productversion

Digging through the old crashes (2984), I also found two interesting ones: 
  http://crash/6658678880000000 (Crash in base::SharedMemory::Close())
  http://crash/d7b5a1e580000000 (Crash in base::WriteFile)
The last good close of both were from ~EventConverterEvdev.

M56 still suffers from the fd issue. Let's merge the fix there and see how much it would help.

This bug requires manual review: We are only 5 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit df2217a1a4a5803474b51bd59a1f4ca11160c21d
Author: Xiyuan Xia <xiyuan@google.com>
Date: Wed Jan 25 19:43:23 2017

Merge "Fix double-close in EventConverterEvdevImpl"

> This fixes a double close that occurs when a keyboard or mouse is
> unplugged from the system, which will cause breakage if there is a
> concurrent open(). It also switches to a scoped file descriptor wrapper,
> with a special close function because close() can return ENODEV on an
> input device that was unplugged.
>
> BUG= 681865 , 660960
> TEST=events_unittests
>
> Review-Url: https://codereview.chromium.org/2639043002
> Cr-Commit-Position: refs/heads/master@{#444544}
> (cherry picked from commit 13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1)

Review-Url: https://codereview.chromium.org/2657533008 .
Cr-Commit-Position: refs/branch-heads/2924@{#863}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/BUILD.gn
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/event_converter_evdev.cc
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/event_converter_evdev_impl.cc
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/event_converter_evdev_impl.h
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/event_converter_evdev_impl_unittest.cc
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/input_device_factory_evdev.cc
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.cc
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.h
[add] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/scoped_input_device.cc
[add] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/scoped_input_device.h
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/tablet_event_converter_evdev.cc
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/tablet_event_converter_evdev.h
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/tablet_event_converter_evdev_unittest.cc
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/touch_event_converter_evdev.cc
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/touch_event_converter_evdev.h
[modify] https://crrev.com/df2217a1a4a5803474b51bd59a1f4ca11160c21d/ui/events/ozone/evdev/touch_event_converter_evdev_unittest.cc

Issue 596592 has been merged into this issue.