New issue
Advanced search Search tips

Issue 645114 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 652529

Blocking:
issue 653916



Sign in to add a comment

New File Descriptor Watch API

Project Member Reported by fdoray@chromium.org, Sep 8 2016

Issue description

Create a new API to register callbacks that are invoked when a file descriptor becomes readable or writable without blocking.

This API’s only requirement should be that SequencedTaskRunnerHandle::Get() returns a SequencedTaskRunner (i.e. in particular, it can be used from SINGLE_THREADED and SEQUENCED tasks running in base/task_scheduler). As opposed to the existing API which is only available from single-threaded tasks running in a MessageLoopForIO.

This API will facilitate the migration of BrowserThreads backed by MessageLoopForIO to base/task_scheduler.

Design doc: https://docs.google.com/document/d/1F5CjON2JNtCtdEug3LOL8-avj188k_xOgqzdv288Q_s/edit?usp=sharing
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 15 2016

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

commit f9f6c1e92b168281ba2fccef2693469380243db8
Author: fdoray <fdoray@chromium.org>
Date: Thu Sep 15 13:23:24 2016

Add a new file descriptor watch API.

This API can be used from any task from which
SequencedTaskRunnerHandle::Get() returns a SequencedTaskRunner (i.e. in
particular, it can be used from SINGLE_THREADED and SEQUENCED tasks
running in base/task_scheduler). As opposed to the existing API which
is only available from single-threaded tasks running in a
MessageLoopForIO.

Threads that want to support this API must instantiate a
FileDescriptorWatcher. The constructor of FileDescriptorWatcher takes
as argument a MessageLoopForIO to use to watch file descriptors for
which callbacks are registered on the thread on which it is invoked. The
MessageLoopForIO does *not* have to run on that thread. When the
MessageLoopForIO detects that watched file descriptors are readable
and/or writable without blocking, it posts a task to run the callback on
the sequence on which it was registered.

Design doc:
https://docs.google.com/document/d/1F5CjON2JNtCtdEug3LOL8-avj188k_xOgqzdv288Q_s/edit?usp=sharing

Discussion on chromium-dev:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/voOAab4mV9A/S9xXdpPkBgAJ

BUG= 645114 

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

[modify] https://crrev.com/f9f6c1e92b168281ba2fccef2693469380243db8/base/BUILD.gn
[add] https://crrev.com/f9f6c1e92b168281ba2fccef2693469380243db8/base/files/file_descriptor_watcher_posix.cc
[add] https://crrev.com/f9f6c1e92b168281ba2fccef2693469380243db8/base/files/file_descriptor_watcher_posix.h
[add] https://crrev.com/f9f6c1e92b168281ba2fccef2693469380243db8/base/files/file_descriptor_watcher_posix_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 27 2016

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

commit fc94b64028d13e4d54038a953038a1c7e3348ed5
Author: fdoray <fdoray@chromium.org>
Date: Tue Sep 27 13:29:35 2016

Enable the FileDescriptorWatcher API on MessageLoopForIO threads.

With this CL, a FileDescriptorWatcher is instantiated on every
base::Thread that runs a MessageLoopForIO. This allows
FileDescriptorWatcher::WatchReadable/WatchWritable to be used
on these threads.

BUG= 645114 

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

[modify] https://crrev.com/fc94b64028d13e4d54038a953038a1c7e3348ed5/base/threading/thread.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 30 2016

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

commit 968e64e3e26c7fa23b147572b04f5bf91c84a404
Author: fdoray <fdoray@chromium.org>
Date: Fri Sep 30 20:34:25 2016

Enable the FileDescriptorWatcher API on TYPE_IO AutoThreads.

With this CL, a FileDescriptorWatcher is instantiated on every
remoting::AutoThread that runs a MessageLoopForIO. This allows
FileDescriptorWatcher::WatchReadable/WatchWritable to be used
on these threads.

BUG= 645114 

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

[modify] https://crrev.com/968e64e3e26c7fa23b147572b04f5bf91c84a404/remoting/base/auto_thread.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 30 2016

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

commit a0e4a7ead3bae88770f4352e2dd927998238b9ee
Author: fdoray <fdoray@chromium.org>
Date: Fri Sep 30 22:27:32 2016

Use FileDescriptorWatcher in FilePathWatcherKQueue.

This allows FilePathWatcherKQueue to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/a0e4a7ead3bae88770f4352e2dd927998238b9ee/base/files/file_path_watcher_kqueue.cc
[modify] https://crrev.com/a0e4a7ead3bae88770f4352e2dd927998238b9ee/base/files/file_path_watcher_kqueue.h
[modify] https://crrev.com/a0e4a7ead3bae88770f4352e2dd927998238b9ee/components/policy/core/common/configuration_policy_provider_test.cc
[modify] https://crrev.com/a0e4a7ead3bae88770f4352e2dd927998238b9ee/components/policy/core/common/configuration_policy_provider_test.h

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 1 2016

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

commit 20e8000df85972c5fd53d287753a189c45b9408c
Author: fdoray <fdoray@chromium.org>
Date: Sat Oct 01 15:37:29 2016

Add comment about leaked FileDescriptorWatcher::Controller::Watcher.

If the MessageLoopForIO is deleted before Watcher::StartWatching() runs,
|watcher_| is leaked. If the MessageLoopForIO is deleted after
Watcher::StartWatching() runs but before the DeleteSoon task runs,
|watcher_| is deleted from Watcher::WillDestroyCurrentMessageLoop().

BUG= 645114 

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

[modify] https://crrev.com/20e8000df85972c5fd53d287753a189c45b9408c/base/files/file_descriptor_watcher_posix.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 3 2016

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

commit cd80c412bd3fce4aca1f9ea6a30030bc1b4a862e
Author: fdoray <fdoray@chromium.org>
Date: Mon Oct 03 17:22:41 2016

Use FileDescriptorWatcher in ClipboardX11.

This allows ClipboardX11 to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/cd80c412bd3fce4aca1f9ea6a30030bc1b4a862e/remoting/host/clipboard_x11.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 3 2016

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

commit 47b8d1585acb01b51f99ce4ab106eb123f9d2502
Author: fdoray <fdoray@chromium.org>
Date: Mon Oct 03 17:54:11 2016

Use FileDescriptorWatcher in DeviceMonitorLinux

This allows DeviceMonitorLinux to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/47b8d1585acb01b51f99ce4ab106eb123f9d2502/device/core/device_monitor_linux.cc
[modify] https://crrev.com/47b8d1585acb01b51f99ce4ab106eb123f9d2502/device/core/device_monitor_linux.h
[modify] https://crrev.com/47b8d1585acb01b51f99ce4ab106eb123f9d2502/device/hid/input_service_linux_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 3 2016

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

commit 92ebb94ad1bdf0d4aef4f74240d4933a945e14d8
Author: fdoray <fdoray@chromium.org>
Date: Mon Oct 03 20:20:35 2016

Use FileDescriptorWatcher in UdevLinux

This allows UdevLinux to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/92ebb94ad1bdf0d4aef4f74240d4933a945e14d8/device/udev_linux/udev_linux.cc
[modify] https://crrev.com/92ebb94ad1bdf0d4aef4f74240d4933a945e14d8/device/udev_linux/udev_linux.h

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 3 2016

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

commit 205d3f2d91dd335957d7dfdb89e1fca833053f38
Author: fdoray <fdoray@chromium.org>
Date: Mon Oct 03 21:29:54 2016

Use FileDescriptorWatcher in ServiceHelper.

FileDescriptorWatcher is a new API that should be used
instead of MessageLoopForIO::WatchFileDescriptor
when possible.

BUG= 645114 

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

[modify] https://crrev.com/205d3f2d91dd335957d7dfdb89e1fca833053f38/components/arc/standalone/service_helper.cc
[modify] https://crrev.com/205d3f2d91dd335957d7dfdb89e1fca833053f38/components/arc/standalone/service_helper.h
[modify] https://crrev.com/205d3f2d91dd335957d7dfdb89e1fca833053f38/components/arc/standalone/service_helper_unittest.cc

Blockedon: 652529
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 4 2016

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

commit 0bd9f1a1ba8ea5e03b189246691de08846ab0f9b
Author: fdoray <fdoray@chromium.org>
Date: Tue Oct 04 17:13:28 2016

Use FileDescriptorWatcher in ProcessOutputWatcher

This allows ProcessOutputWatcher to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/0bd9f1a1ba8ea5e03b189246691de08846ab0f9b/chromeos/process_proxy/process_output_watcher.cc
[modify] https://crrev.com/0bd9f1a1ba8ea5e03b189246691de08846ab0f9b/chromeos/process_proxy/process_output_watcher.h

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 4 2016

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

commit 80da60ef04918cb0a7f7cf3f53e6ab8be88b81c8
Author: fdoray <fdoray@chromium.org>
Date: Tue Oct 04 18:20:29 2016

Use FileDescriptorWatcher in UsbDeviceHandleUsbfs.

This allows UsbDeviceHandleUsbfs to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/80da60ef04918cb0a7f7cf3f53e6ab8be88b81c8/device/usb/usb_device_handle_usbfs.cc
[modify] https://crrev.com/80da60ef04918cb0a7f7cf3f53e6ab8be88b81c8/device/usb/usb_device_handle_usbfs.h

Project Member

Comment 13 by bugdroid1@chromium.org, Oct 4 2016

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

commit 8be7e5011d52cc9f1dbd55359e4bf417a73a1f89
Author: fdoray <fdoray@chromium.org>
Date: Tue Oct 04 20:25:42 2016

Use FileDescriptorWatcher in NativeMessageProcessHost.

This allows NativeMessageProcessHost to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/8be7e5011d52cc9f1dbd55359e4bf417a73a1f89/chrome/browser/extensions/api/messaging/native_message_process_host.cc
[modify] https://crrev.com/8be7e5011d52cc9f1dbd55359e4bf417a73a1f89/chrome/browser/extensions/api/messaging/native_message_process_host.h
[modify] https://crrev.com/8be7e5011d52cc9f1dbd55359e4bf417a73a1f89/chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Oct 5 2016

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

commit 1696fb9dfdc8095f7cd45239aa64b54603379322
Author: fdoray <fdoray@chromium.org>
Date: Wed Oct 05 07:29:32 2016

Build FileDescriptorWatcher for NaCl non-SFI

TestLauncherNonSfiMain instantiates a base::TestLauncher which needs
to watch a file descriptor.

BUG= 645114 

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

[modify] https://crrev.com/1696fb9dfdc8095f7cd45239aa64b54603379322/base/BUILD.gn

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 5 2016

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

commit 38bc36cf16d7a663619670854d0c3123a4302102
Author: fdoray <fdoray@chromium.org>
Date: Wed Oct 05 14:56:30 2016

Use FileDescriptorWatcher in UserInputMonitorLinuxCore.

This allows UserInputMonitorLinuxCore to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/38bc36cf16d7a663619670854d0c3123a4302102/media/base/user_input_monitor_linux.cc
[modify] https://crrev.com/38bc36cf16d7a663619670854d0c3123a4302102/media/base/user_input_monitor_unittest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Oct 5 2016

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

commit e7ee44e7643167612747acdd1bb081b4066ff89a
Author: fdoray <fdoray@chromium.org>
Date: Wed Oct 05 18:15:58 2016

Use FileDescriptorWatcher in tap_proxy.cc.

FileDescriptorWatcher is a new API that replaces
MessageLoopForIO::WatchFileDescriptor.

BUG= 645114 

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

[modify] https://crrev.com/e7ee44e7643167612747acdd1bb081b4066ff89a/media/cast/test/utility/tap_proxy.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Oct 6 2016

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

commit ddf00b80457978be7e72730b1f78d4c411572f6e
Author: fdoray <fdoray@chromium.org>
Date: Thu Oct 06 18:42:19 2016

Use FileDescriptorWatcher in BrlapiConnectionImpl.

This allows BrlapiConnectionImpl to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/ddf00b80457978be7e72730b1f78d4c411572f6e/chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Oct 6 2016

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

commit 8e17d7e9031b7e2e58473fe66a399943051357f0
Author: fdoray <fdoray@chromium.org>
Date: Thu Oct 06 19:03:34 2016

Use FileDescriptorWatcher in AlarmTimer.

FileDescriptorWatcher is a new API that replaces
MessageLoopForIO::WatchFileDescriptor.

This CL also gets rid of the base::Thread created in the anonymous
namespace of alarm_timer_chromeos.cc. The FileDescriptorWatcher
API is supported by every TaskScheduler thread. Very soon, most
tasks in Chrome will run in TaskScheduler. Therefore, there is no
reason to create a base::Thread to allow AlarmTimer to be used from
threads that don't support the FileDescriptorWatcher API.

BUG= 645114 

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

[modify] https://crrev.com/8e17d7e9031b7e2e58473fe66a399943051357f0/components/timers/alarm_timer_chromeos.cc
[modify] https://crrev.com/8e17d7e9031b7e2e58473fe66a399943051357f0/components/timers/alarm_timer_chromeos.h
[modify] https://crrev.com/8e17d7e9031b7e2e58473fe66a399943051357f0/components/timers/alarm_timer_unittest.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Oct 6 2016

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

commit 7a53efb2ae89077b62b7cc5ccbb11bf01246d20b
Author: nasko <nasko@chromium.org>
Date: Thu Oct 06 21:22:32 2016

Revert of Use FileDescriptorWatcher in AlarmTimer. (patchset #2 id:20001 of https://codereview.chromium.org/2398753003/ )

Reason for revert:
This CL is causing a memory leak in the unit tests:

https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/16728

failures:
AlarmTimerTest.RetainNonRepeatIsRunning
AlarmTimerTest.MessageLoopShutdown
AlarmTimerTest.NonRepeatIsRunning
AlarmTimerTest.RetainRepeatIsRunning

Sample report:
Direct leak of 152 byte(s) in 1 object(s) allocated from:
    #0 0xab8ebb in operator new(unsigned long) (/b/swarm_slave/w/irlnnUGq/out/Release/components_unittests+0xab8ebb)
    #1 0x9eba8fd in MakeUnique<base::FileDescriptorWatcher::Controller::Watcher, base::WeakPtr<base::FileDescriptorWatcher::Controller>, base::MessageLoopForIO::Mode &, int &> base/memory/ptr_util.h:56:29
    #2 0x9eba8fd in base::FileDescriptorWatcher::Controller::Controller(base::MessageLoopForIO::Mode, int, base::Callback<void (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/files/file_descriptor_watcher_posix.cc:159
    #3 0x9ebb293 in base::FileDescriptorWatcher::WatchReadable(int, base::Callback<void (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/files/file_descriptor_watcher_posix.cc:198:25
    #4 0xe0016a3 in timers::AlarmTimer::Reset() components/timers/alarm_timer_chromeos.cc:103:25
    #5 0x515b488 in timers::AlarmTimerTest_RetainRepeatIsRunning_Test::TestBody() components/timers/alarm_timer_unittest.cc:310:9

Original issue's description:
> Use FileDescriptorWatcher in AlarmTimer.
>
> FileDescriptorWatcher is a new API that replaces
> MessageLoopForIO::WatchFileDescriptor.
>
> This CL also gets rid of the base::Thread created in the anonymous
> namespace of alarm_timer_chromeos.cc. The FileDescriptorWatcher
> API is supported by every TaskScheduler thread. Very soon, most
> tasks in Chrome will run in TaskScheduler. Therefore, there is no
> reason to create a base::Thread to allow AlarmTimer to be used from
> threads that don't support the FileDescriptorWatcher API.
>
> BUG= 645114 
>
> Committed: https://crrev.com/8e17d7e9031b7e2e58473fe66a399943051357f0
> Cr-Commit-Position: refs/heads/master@{#423617}

TBR=chirantan@chromium.org,fdoray@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 645114 

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

[modify] https://crrev.com/7a53efb2ae89077b62b7cc5ccbb11bf01246d20b/components/timers/alarm_timer_chromeos.cc
[modify] https://crrev.com/7a53efb2ae89077b62b7cc5ccbb11bf01246d20b/components/timers/alarm_timer_chromeos.h
[modify] https://crrev.com/7a53efb2ae89077b62b7cc5ccbb11bf01246d20b/components/timers/alarm_timer_unittest.cc

Project Member

Comment 21 by bugdroid1@chromium.org, Oct 7 2016

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

commit fab0376d55393aabb6811c6cbdbe1f67975b13b4
Author: fdoray <fdoray@chromium.org>
Date: Fri Oct 07 16:43:06 2016

Use FileDescriptorWatcher in SerialIoHandlerPosix.

This allows SerialIoHandlerPosix to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/fab0376d55393aabb6811c6cbdbe1f67975b13b4/device/serial/serial_io_handler_posix.cc
[modify] https://crrev.com/fab0376d55393aabb6811c6cbdbe1f67975b13b4/device/serial/serial_io_handler_posix.h

Project Member

Comment 22 by bugdroid1@chromium.org, Oct 11 2016

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

commit 9f0e84139b0c8c3e40798e135f37bde5b0e3c7ad
Author: fdoray <fdoray@chromium.org>
Date: Tue Oct 11 17:19:08 2016

Use FileDescriptorWatcher in LocalInputMonitorX11.

This allows LocalInputMonitorX11 to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/9f0e84139b0c8c3e40798e135f37bde5b0e3c7ad/remoting/host/local_input_monitor_x11.cc

Project Member

Comment 23 by bugdroid1@chromium.org, Oct 11 2016

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

commit 51dbb9577b6a8194f025d790f581c10559f3ee2a
Author: fdoray <fdoray@chromium.org>
Date: Tue Oct 11 17:31:13 2016

Use FileDescriptorWatcher in AlarmTimer.

This CL is identical to https://codereview.chromium.org/2398753003/
which was reverted % fixes to avoid memory leaks in tests.

FileDescriptorWatcher is a new API that replaces
MessageLoopForIO::WatchFileDescriptor.

This CL also gets rid of the base::Thread created in the anonymous
namespace of alarm_timer_chromeos.cc. The FileDescriptorWatcher
API is supported by every TaskScheduler thread. Very soon, most
tasks in Chrome will run in TaskScheduler. Therefore, there is no
reason to create a base::Thread to allow AlarmTimer to be used from
threads that don't support the FileDescriptorWatcher API.

BUG= 645114 

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

[modify] https://crrev.com/51dbb9577b6a8194f025d790f581c10559f3ee2a/components/timers/alarm_timer_chromeos.cc
[modify] https://crrev.com/51dbb9577b6a8194f025d790f581c10559f3ee2a/components/timers/alarm_timer_chromeos.h
[modify] https://crrev.com/51dbb9577b6a8194f025d790f581c10559f3ee2a/components/timers/alarm_timer_unittest.cc

Project Member

Comment 24 by bugdroid1@chromium.org, Oct 11 2016

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

commit aae14a15971bc8f64e591593bbe626b44839993a
Author: fdoray <fdoray@chromium.org>
Date: Tue Oct 11 17:33:08 2016

Use FileDescriptorWatcher in signal_handler.cc.

This allows RegisterSignalHandler to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/aae14a15971bc8f64e591593bbe626b44839993a/remoting/host/posix/signal_handler.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Oct 11 2016

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

commit 150be4b2b4d10d0bd5a552ff76802b2d15950ec6
Author: fdoray <fdoray@chromium.org>
Date: Tue Oct 11 19:08:11 2016

Use FileDescriptorWatcher in HidConnectionLinux::FileThreadHelper.

This allows  HidConnectionLinux::FileThreadHelper to be used from
any thread that instantiates a FileDescriptorWatcher (not just threads
that run a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/150be4b2b4d10d0bd5a552ff76802b2d15950ec6/device/hid/hid_connection_linux.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Oct 11 2016

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

commit ac05888c1c2786d7910de47856ba831f3c5da827
Author: fdoray <fdoray@chromium.org>
Date: Tue Oct 11 19:35:06 2016

Use FileDescriptorWatcher in AudioPipeReader.

This allows AudioPipeReader to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/ac05888c1c2786d7910de47856ba831f3c5da827/remoting/host/linux/audio_pipe_reader.cc
[modify] https://crrev.com/ac05888c1c2786d7910de47856ba831f3c5da827/remoting/host/linux/audio_pipe_reader.h

Project Member

Comment 27 by bugdroid1@chromium.org, Oct 17 2016

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

commit 233655edccc560d98e739b48f16879df99d9c398
Author: fdoray <fdoray@chromium.org>
Date: Mon Oct 17 15:35:24 2016

Use FileDescriptorWatcher in wl::FakeServer.

FileDescriptorWatcher is a new API that replaces
MessageLoopForIO::WatchFileDescriptor.

BUG= 645114 

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

[modify] https://crrev.com/233655edccc560d98e739b48f16879df99d9c398/ui/ozone/platform/wayland/fake_server.cc
[modify] https://crrev.com/233655edccc560d98e739b48f16879df99d9c398/ui/ozone/platform/wayland/fake_server.h

Status: Fixed (was: Started)
The new API is now implemented and available on TYPE_IO base::Threads and on TaskScheduler threads.

Comment 29 by gab@chromium.org, Oct 25 2016

Woot :)!
Status: Started (was: Fixed)
Oops... My cl didn't land yet :(
Blocking: -553459 653916
Project Member

Comment 33 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38bc36cf16d7a663619670854d0c3123a4302102

commit 38bc36cf16d7a663619670854d0c3123a4302102
Author: fdoray <fdoray@chromium.org>
Date: Wed Oct 05 14:56:30 2016

Use FileDescriptorWatcher in UserInputMonitorLinuxCore.

This allows UserInputMonitorLinuxCore to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/38bc36cf16d7a663619670854d0c3123a4302102/media/base/user_input_monitor_linux.cc
[modify] https://crrev.com/38bc36cf16d7a663619670854d0c3123a4302102/media/base/user_input_monitor_unittest.cc

Project Member

Comment 34 by bugdroid1@chromium.org, Oct 27 2016

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

commit e7ee44e7643167612747acdd1bb081b4066ff89a
Author: fdoray <fdoray@chromium.org>
Date: Wed Oct 05 18:15:58 2016

Use FileDescriptorWatcher in tap_proxy.cc.

FileDescriptorWatcher is a new API that replaces
MessageLoopForIO::WatchFileDescriptor.

BUG= 645114 

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

[modify] https://crrev.com/e7ee44e7643167612747acdd1bb081b4066ff89a/media/cast/test/utility/tap_proxy.cc

Project Member

Comment 35 by bugdroid1@chromium.org, Oct 27 2016

Project Member

Comment 36 by bugdroid1@chromium.org, Oct 27 2016

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

commit ddf00b80457978be7e72730b1f78d4c411572f6e
Author: fdoray <fdoray@chromium.org>
Date: Thu Oct 06 18:42:19 2016

Use FileDescriptorWatcher in BrlapiConnectionImpl.

This allows BrlapiConnectionImpl to be used from any thread that
instantiates a FileDescriptorWatcher (not just threads that run
a MessageLoopForIO). This will facilitate the migration of
BrowserThreads to base/task_scheduler.

BUG= 645114 

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

[modify] https://crrev.com/ddf00b80457978be7e72730b1f78d4c411572f6e/chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc

Project Member

Comment 37 by bugdroid1@chromium.org, Oct 27 2016

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

commit 8e17d7e9031b7e2e58473fe66a399943051357f0
Author: fdoray <fdoray@chromium.org>
Date: Thu Oct 06 19:03:34 2016

Use FileDescriptorWatcher in AlarmTimer.

FileDescriptorWatcher is a new API that replaces
MessageLoopForIO::WatchFileDescriptor.

This CL also gets rid of the base::Thread created in the anonymous
namespace of alarm_timer_chromeos.cc. The FileDescriptorWatcher
API is supported by every TaskScheduler thread. Very soon, most
tasks in Chrome will run in TaskScheduler. Therefore, there is no
reason to create a base::Thread to allow AlarmTimer to be used from
threads that don't support the FileDescriptorWatcher API.

BUG= 645114 

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

[modify] https://crrev.com/8e17d7e9031b7e2e58473fe66a399943051357f0/components/timers/alarm_timer_chromeos.cc
[modify] https://crrev.com/8e17d7e9031b7e2e58473fe66a399943051357f0/components/timers/alarm_timer_chromeos.h
[modify] https://crrev.com/8e17d7e9031b7e2e58473fe66a399943051357f0/components/timers/alarm_timer_unittest.cc

Project Member

Comment 38 by bugdroid1@chromium.org, Oct 27 2016

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

commit 7a53efb2ae89077b62b7cc5ccbb11bf01246d20b
Author: nasko <nasko@chromium.org>
Date: Thu Oct 06 21:22:32 2016

Revert of Use FileDescriptorWatcher in AlarmTimer. (patchset #2 id:20001 of https://codereview.chromium.org/2398753003/ )

Reason for revert:
This CL is causing a memory leak in the unit tests:

https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/16728

failures:
AlarmTimerTest.RetainNonRepeatIsRunning
AlarmTimerTest.MessageLoopShutdown
AlarmTimerTest.NonRepeatIsRunning
AlarmTimerTest.RetainRepeatIsRunning

Sample report:
Direct leak of 152 byte(s) in 1 object(s) allocated from:
    #0 0xab8ebb in operator new(unsigned long) (/b/swarm_slave/w/irlnnUGq/out/Release/components_unittests+0xab8ebb)
    #1 0x9eba8fd in MakeUnique<base::FileDescriptorWatcher::Controller::Watcher, base::WeakPtr<base::FileDescriptorWatcher::Controller>, base::MessageLoopForIO::Mode &, int &> base/memory/ptr_util.h:56:29
    #2 0x9eba8fd in base::FileDescriptorWatcher::Controller::Controller(base::MessageLoopForIO::Mode, int, base::Callback<void (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/files/file_descriptor_watcher_posix.cc:159
    #3 0x9ebb293 in base::FileDescriptorWatcher::WatchReadable(int, base::Callback<void (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/files/file_descriptor_watcher_posix.cc:198:25
    #4 0xe0016a3 in timers::AlarmTimer::Reset() components/timers/alarm_timer_chromeos.cc:103:25
    #5 0x515b488 in timers::AlarmTimerTest_RetainRepeatIsRunning_Test::TestBody() components/timers/alarm_timer_unittest.cc:310:9

Original issue's description:
> Use FileDescriptorWatcher in AlarmTimer.
>
> FileDescriptorWatcher is a new API that replaces
> MessageLoopForIO::WatchFileDescriptor.
>
> This CL also gets rid of the base::Thread created in the anonymous
> namespace of alarm_timer_chromeos.cc. The FileDescriptorWatcher
> API is supported by every TaskScheduler thread. Very soon, most
> tasks in Chrome will run in TaskScheduler. Therefore, there is no
> reason to create a base::Thread to allow AlarmTimer to be used from
> threads that don't support the FileDescriptorWatcher API.
>
> BUG= 645114 
>
> Committed: https://crrev.com/8e17d7e9031b7e2e58473fe66a399943051357f0
> Cr-Commit-Position: refs/heads/master@{#423617}

TBR=chirantan@chromium.org,fdoray@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 645114 

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

[modify] https://crrev.com/7a53efb2ae89077b62b7cc5ccbb11bf01246d20b/components/timers/alarm_timer_chromeos.cc
[modify] https://crrev.com/7a53efb2ae89077b62b7cc5ccbb11bf01246d20b/components/timers/alarm_timer_chromeos.h
[modify] https://crrev.com/7a53efb2ae89077b62b7cc5ccbb11bf01246d20b/components/timers/alarm_timer_unittest.cc

Status: Fixed (was: Started)

Comment 40 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840

Comment 41 by gab@chromium.org, Nov 7 2016

Components: Internals>TaskScheduler

Comment 42 by gab@chromium.org, May 9 2017

@fdoray as discussed offline: please add a top-level example as documentation in file_descriptor_watcher_posix.h which uses an Unretained(this) callback and mentions why it's safe to do so.

Thanks :)!
Project Member

Comment 43 by bugdroid1@chromium.org, May 10 2017

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

commit f4f904edc4cbbb9ffd269cd6fd5d26d9925aee8c
Author: gab <gab@chromium.org>
Date: Wed May 10 20:55:02 2017

Use base::FileDescriptorWatcher instead of MessageLoopForIO::WatchFileDescriptor in proxy_config_service_linux.cc

base::FileDescriptorWatcher is the equivalent MessageLoop independent API
(the only difference is an extra PostTask hop on notification which is irrelevant
for low volume handles)

This MessageLoopForIO::WatchFileDescriptor is causing problems @ https://bugs.chromium.org/p/chromium/issues/detail?id=653916#c47
per the FILE thread's MessageLoop going away.

BUG= 645114 ,  653916 

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

[modify] https://crrev.com/f4f904edc4cbbb9ffd269cd6fd5d26d9925aee8c/net/proxy/proxy_config_service_linux.cc

Project Member

Comment 44 by bugdroid1@chromium.org, Apr 6 2018

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

commit 95807795f9c89841d28734d3305a3488e7455343
Author: Gabriel Charette <gab@chromium.org>
Date: Fri Apr 06 18:22:50 2018

Clarify base::FileDescriptorWatcher's API to mention that the task is always posted.

(as opposed to invoked on a delegate right away)

Prerequisite for a note I'm about to add on Posix MessagePumps'
WatchFileDescriptor() API.

R=fdoray@chromium.org

Bug:  645114 , 825327
Change-Id: I5ead76650bd7a605c7bd19648b10daf2195e052c
Reviewed-on: https://chromium-review.googlesource.com/999792
Commit-Queue: Gabriel Charette <gab@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548864}
[modify] https://crrev.com/95807795f9c89841d28734d3305a3488e7455343/base/files/file_descriptor_watcher_posix.h

Sign in to add a comment