New issue
Advanced search Search tips

Issue 650318 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 653916



Sign in to add a comment

Remove MessageLoop destruction observers

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

Issue description

TaskScheduler doesn't support destruction observers. Removing MessageLoop destruction observers will facilitate the migration of BrowserThreads to TaskScheduler.
 
Project Member

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

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

commit 7b67e033842715d4941d46e726df8ca18d94d768
Author: fdoray <fdoray@chromium.org>
Date: Tue Sep 27 12:23:33 2016

Remove MessageLoop::current() from base::win::ObjectWatcher (reland).

---
This is a reland of https://codereview.chromium.org/2125763003 which
was reverted because it broke the Win10 builder. It should not break
anything now that TestSimpleTaskRunner is thread-safe
(https://codereview.chromium.org/2296923003)
---

Why? The fact that there's a MessageLoop on the thread is an
unnecessary implementation detail. When browser threads are migrated to
base/task_scheduler, tasks will no longer have access to a MessageLoop
but they will be able to get the current ThreadTaskRunnerHandle.

Before this CL, ObjectWatcher implemented
WillDestroyCurrentMessageLoop() to stop the watch when the
MessageLoop responsible for running the callback was destroyed.
This prevented the watch callback from being sent to a destroyed
MessageLoop.

Now that ObjectWatcher keeps a reference to a TaskRunner, this is
no longer required. The TaskRunner can't be deleted while there are
references to it. If the underlying MessageLoop has been deleted
when the watch callback is posted, the callback will simply not run.

Note that the watch will still be stopped when the ObjectWatcher is
deleted.

TBR=ben@chromium.org
BUG= 650318 

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

[modify] https://crrev.com/7b67e033842715d4941d46e726df8ca18d94d768/base/files/file_path_watcher_win.cc
[modify] https://crrev.com/7b67e033842715d4941d46e726df8ca18d94d768/base/process/kill_win.cc
[modify] https://crrev.com/7b67e033842715d4941d46e726df8ca18d94d768/base/win/object_watcher.cc
[modify] https://crrev.com/7b67e033842715d4941d46e726df8ca18d94d768/base/win/object_watcher.h
[modify] https://crrev.com/7b67e033842715d4941d46e726df8ca18d94d768/chrome/common/service_process_util_win.cc
[modify] https://crrev.com/7b67e033842715d4941d46e726df8ca18d94d768/ui/gfx/font_fallback_win.cc

Project Member

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

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

commit cc6bd5c938f5d4dfa33ed1bbe962a2effc12d597
Author: fdoray <fdoray@chromium.org>
Date: Tue Oct 04 18:03:17 2016

Make WaitableEventWatcher TaskScheduler-friendly.

With this CL, WaitableEventWatcher uses SequencedTaskRunnerHandle
instead of MessageLoop::current() to post back to the sequence that
called StartWatching().

Also, WaitableEventWatcher no longer registers itself as a destruction
observer of the MessageLoop from which StartWatching() is called. If
the watched WaitableEvent is signaled after the MessageLoop is
destroyed, the task posted by WaitableEventWatcher to the
SequencedTaskRunner will simply not run (no crash).

MessageLoop::current() and destruction observer do not work with
TaskScheduler.

BUG= 650318 

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

[modify] https://crrev.com/cc6bd5c938f5d4dfa33ed1bbe962a2effc12d597/base/synchronization/waitable_event_watcher.h
[modify] https://crrev.com/cc6bd5c938f5d4dfa33ed1bbe962a2effc12d597/base/synchronization/waitable_event_watcher_posix.cc
[modify] https://crrev.com/cc6bd5c938f5d4dfa33ed1bbe962a2effc12d597/base/synchronization/waitable_event_watcher_unittest.cc
[modify] https://crrev.com/cc6bd5c938f5d4dfa33ed1bbe962a2effc12d597/base/synchronization/waitable_event_watcher_win.cc

Project Member

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

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

commit 7a9ea7caa267b3779d7984a6834e2d35b723cefb
Author: fdoray <fdoray@chromium.org>
Date: Thu Oct 20 17:22:27 2016

Leak DeviceMonitorLinux on shutdown.

In preparation for the migration of many Chrome threads to
TaskScheduler, we want to get rid of MessageLoop destruction
observers.

This CL removes the code that deletes DeviceMonitorLinux from
a destruction observer and uses a LazyInstance::Leaky to manage
the lifetime of DeviceMonitorLinux.

This CL also helps reducing the amount of work done during
shutdown
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/AMB2Y7Sqj48/QltA07wUCQAJ

BUG= 650318 

Review-Url: https://chromiumcodereview.appspot.com/2433933004
Cr-Commit-Position: refs/heads/master@{#426518}

[modify] https://crrev.com/7a9ea7caa267b3779d7984a6834e2d35b723cefb/device/base/device_monitor_linux.cc

Comment 5 by fdoray@chromium.org, Oct 25 2016

Blocking: -553459 653916

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

Components: Internals>TaskScheduler
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 23 2016

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

commit 1e1feb846042722a8df671de070a32374221e499
Author: fdoray <fdoray@chromium.org>
Date: Wed Nov 23 12:29:59 2016

Remove destruction observer from file_path_watcher_win.cc

Now that the FilePathWatcher::~FilePathWatcher() is called on the same
sequence as FilePathWatcher::Watch() (see
https://codereview.chromium.org/2438913003/), the work done in
FilePathWatcherImpl::WillDestroyCurrentMessageLoop() can safely be
moved to FilePathWatcherImpl::Cancel().

BUG= 650318 

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

[modify] https://crrev.com/1e1feb846042722a8df671de070a32374221e499/base/files/file_path_watcher_win.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 29 2016

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

commit 52e11d48575954e29aa9407c4343d76f0475327a
Author: fdoray <fdoray@chromium.org>
Date: Tue Nov 29 22:19:58 2016

Remove destruction observer from base::FilePathWatcherKQueue.

Now that FilePathWatcher::~FilePathWatcher() is called on the same
sequence as FilePathWatcher::Watch() (see
https://codereview.chromium.org/2438913003/), the work done in
FilePathWatcherKQueue::WillDestroyCurrentMessageLoop() can safely be
moved to FilePathWatcherKQueue::Cancel().

BUG= 650318 

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

[modify] https://crrev.com/52e11d48575954e29aa9407c4343d76f0475327a/base/files/file_path_watcher.h
[modify] https://crrev.com/52e11d48575954e29aa9407c4343d76f0475327a/base/files/file_path_watcher_kqueue.cc
[modify] https://crrev.com/52e11d48575954e29aa9407c4343d76f0475327a/base/files/file_path_watcher_kqueue.h

Status: Fixed (was: Started)
Removed all destruction observers from all browser threads that will be migrated to TaskScheduler.

Sign in to add a comment