Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808983002:
Migrate to base::FileDescriptionWatcher in dbus/bus.cc. (Closed)
DescriptionMigrate to base::FileDescriptionWatcher in dbus/bus.cc.
Before this CL, dbus::Bus::OnAddWatch called
MessageLoopForIO::current()->WatchFileDescriptor(), which is invalid from a
non-MessageLoopForIO thread. This caused a crash when dbus::Bus::OnAddWatch
was called from a TaskScheduler thread.
With this CL, dbus::Bus::OnAddWatch uses
base::FileDescriptorWatcher::WatchReadable/Writable instead of
MessageLoopForIO::current()->WatchFileDescriptor(). This works from
base::Threads that run a MessageLoopForIO and TaskScheduler threads.
This CL also instantiates a base::FileDescriptorWatcher in tests that use
dbus::Bus::OnAddWatch. This is required to allow usage of
base::FileDescriptorWatcher::WatchReadable/Writable on the main thread
of a test.
BUG=708142
Review-Url: https://codereview.chromium.org/2808983002
Cr-Commit-Position: refs/heads/master@{#465215}
Committed: https://chromium.googlesource.com/chromium/src/+/ebc379c0d84a4c0447bcc95317c85ffee5c9025e
Patch Set 1 #
Total comments: 5
Patch Set 2 : CR #Patch Set 3 : add-FileDescriptorWatcher #Patch Set 4 : self-review #Patch Set 5 : self-review #
Total comments: 4
Patch Set 6 : CR-daniel-erat #
Total comments: 4
Patch Set 7 : fix-build-error-check-single-line #
Total comments: 4
Patch Set 8 : CR-satorux1-48-update-comment #
Messages
Total messages: 60 (34 generated)
Description was changed from ========== dbus BUG= ========== to ========== Migrate to base::FileDescriptionWatcher in dbus/bus.cc. Before this CL, dbus::Bus::OnAddWatch called MessageLoopForIO::current(), which is illegal from a non-MessageLoopForIO thread. This caused a crash. With this CL, dbus::Bus::OnAddWatch uses base::FileDescriptorWatcher, which works from MessageLoopForIO and TaskScheduler threads. BUG=708142 ==========
fdoray@chromium.org changed reviewers: + satorux@chromium.org
PTAL
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
satorux@chromium.org changed reviewers: + derat@chromium.org, hashimoto@chromium.org
https://codereview.chromium.org/2808983002/diff/1/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/2808983002/diff/1/dbus/bus.cc#newcode68 dbus/bus.cc:68: base::Unretained(this))); Is unretained safe here? https://codereview.chromium.org/2808983002/diff/1/dbus/bus.cc#newcode71 dbus/bus.cc:71: read_watcher_ = base::FileDescriptorWatcher::WatchReadable( WatchWritable?
it's good that the linux bots failed, but i'm surprised that the chrome os tests didn't. do we not exercise this code there for some reason? https://codereview.chromium.org/2808983002/diff/1/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/2808983002/diff/1/dbus/bus.cc#newcode71 dbus/bus.cc:71: read_watcher_ = base::FileDescriptorWatcher::WatchReadable( On 2017/04/11 03:47:21, satorux1 wrote: > WatchWritable? and this line should also assign to write_watcher_, not read_watcher_.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
PTAnL https://codereview.chromium.org/2808983002/diff/1/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/2808983002/diff/1/dbus/bus.cc#newcode68 dbus/bus.cc:68: base::Unretained(this))); On 2017/04/11 03:47:21, satorux1 wrote: > Is unretained safe here? Added comment. https://codereview.chromium.org/2808983002/diff/1/dbus/bus.cc#newcode71 dbus/bus.cc:71: read_watcher_ = base::FileDescriptorWatcher::WatchReadable( On 2017/04/11 05:14:15, Daniel Erat wrote: > On 2017/04/11 03:47:21, satorux1 wrote: > > WatchWritable? > > and this line should also assign to write_watcher_, not read_watcher_. oops... Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2808983002/diff/80001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/2808983002/diff/80001/dbus/bus.cc#newcode86 dbus/bus.cc:86: void OnFileCanReadWithoutBlocking() { i think you could simplify this further by just having a single OnFileReady method that takes either DBUS_WATCH_READABLE or DBUS_WATCH_WRITABLE as a parameter. https://codereview.chromium.org/2808983002/diff/80001/dbus/object_proxy_unitt... File dbus/object_proxy_unittest.cc (right): https://codereview.chromium.org/2808983002/diff/80001/dbus/object_proxy_unitt... dbus/object_proxy_unittest.cc:32: // Enable FileDescriptorWatcher. Needed dbus::Watch. nit: "Needed by ..."? i don't understand the "Enable" part of this either; maybe just delete it?
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAnL https://codereview.chromium.org/2808983002/diff/80001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/2808983002/diff/80001/dbus/bus.cc#newcode86 dbus/bus.cc:86: void OnFileCanReadWithoutBlocking() { On 2017/04/11 16:36:19, Daniel Erat wrote: > i think you could simplify this further by just having a single OnFileReady > method that takes either DBUS_WATCH_READABLE or DBUS_WATCH_WRITABLE as a > parameter. Done. https://codereview.chromium.org/2808983002/diff/80001/dbus/object_proxy_unitt... File dbus/object_proxy_unittest.cc (right): https://codereview.chromium.org/2808983002/diff/80001/dbus/object_proxy_unitt... dbus/object_proxy_unittest.cc:32: // Enable FileDescriptorWatcher. Needed dbus::Watch. On 2017/04/11 16:36:19, Daniel Erat wrote: > nit: "Needed by ..."? i don't understand the "Enable" part of this either; maybe > just delete it? Done. You can't call FileDescriptorWatcher::WatchReadable/Writable outside the scope of a FileDescriptorWatcher. This is documented here https://cs.chromium.org/chromium/src/base/files/file_descriptor_watcher_posix...
https://codereview.chromium.org/2808983002/diff/100001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/2808983002/diff/100001/dbus/bus.cc#newcode70 dbus/bus.cc:70: base::Bind(&Watch::OnFileCanReadWithoutBlocking, i don't think this compiles; please test changes locally before uploading https://codereview.chromium.org/2808983002/diff/100001/dbus/bus.cc#newcode90 dbus/bus.cc:90: CHECK(success) << "Unable to allocate memory"; nit: just CHECK() the dbus_watch_handle call on a single line
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/11 05:14:15, Daniel Erat wrote: > it's good that the linux bots failed, but i'm surprised that the chrome os tests > didn't. do we not exercise this code there for some reason? I think we used to run dbus_unittests run on linux_chromium_chromeos_* try bots. Maybe the configurations for the bots were changed somehow, or the bot that runs the test was removed from the default set of try bots. Let me file a bug.
lgtm (i didn't notice there was a new version until now. codereview.chromium.org doesn't automatically email reviewers when a new patchset is uploaded; you need to do it manually.)
On 2017/04/12 00:22:41, satorux1 wrote: > On 2017/04/11 05:14:15, Daniel Erat wrote: > > it's good that the linux bots failed, but i'm surprised that the chrome os > tests > > didn't. do we not exercise this code there for some reason? > > I think we used to run dbus_unittests run on linux_chromium_chromeos_* try bots. > Maybe the configurations for the bots were changed somehow, or the bot that runs > the test was removed from the default set of try bots. Let me file a bug. Filed: crbug.com/710686
Could you addd TEST= line? fdoray: were you able to reproduce this bug locally and confirmed that the patch fixed the crash?
> With this CL, dbus::Bus::OnAddWatch uses > base::FileDescriptorWatcher, which works from > MessageLoopForIO and TaskScheduler threads. I wanted to know how base::FileDescriptorWatcher works on "TaskScheduler threads" but I failed to understand: https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc#newcode68 dbus/bus.cc:68: read_watcher_ = base::FileDescriptorWatcher::WatchReadable( The function comment for WatchReadable() says: // To call these methods, a FileDescriptorWatcher must have been // instantiated on the current thread and SequencedTaskRunnerHandle::IsSet() // must return true. It wasn't clear to me where FileDescriptorWatcher is instantiated. I took a quick look and found that a thread with MessageLoopForIO has an instance: https://cs.chromium.org/chromium/src/base/threading/thread.cc?q=new%5C+Filede... Is FileDescriptorWatcher also instantiated in "TaskScheduler threads"?
On 2017/04/12 01:02:30, satorux1 wrote: > > With this CL, dbus::Bus::OnAddWatch uses > > base::FileDescriptorWatcher, which works from > > MessageLoopForIO and TaskScheduler threads. > > I wanted to know how base::FileDescriptorWatcher works on "TaskScheduler > threads" but I failed to understand: > > https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc > File dbus/bus.cc (right): > > https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc#newcode68 > dbus/bus.cc:68: read_watcher_ = base::FileDescriptorWatcher::WatchReadable( > The function comment for WatchReadable() says: > > > // To call these methods, a FileDescriptorWatcher must have been > // instantiated on the current thread and SequencedTaskRunnerHandle::IsSet() > // must return true. > > > It wasn't clear to me where FileDescriptorWatcher is instantiated. I took a > quick look and found that a thread with MessageLoopForIO has an instance: > > https://cs.chromium.org/chromium/src/base/threading/thread.cc?q=new%5C+Filede... > > Is FileDescriptorWatcher also instantiated in "TaskScheduler threads"? Hmm, I'm more confused as there are two FileDescriptorWatcher: - base::FileDescriptorWatcher - MessageLoopForIO::FileDescriptorWatcher "a FileDescriptorWatcher" in the comment refers to which one?
On 2017/04/12 00:25:22, Daniel Erat wrote: > lgtm > > (i didn't notice there was a new version until now. http://codereview.chromium.org > doesn't automatically email reviewers when a new patchset is uploaded; you need > to do it manually.) I wanted to wait for all bots to be green before sending a message :)
https://codereview.chromium.org/2808983002/diff/100001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/2808983002/diff/100001/dbus/bus.cc#newcode70 dbus/bus.cc:70: base::Bind(&Watch::OnFileCanReadWithoutBlocking, On 2017/04/11 17:05:54, Daniel Erat wrote: > i don't think this compiles; please test changes locally before uploading Done. https://codereview.chromium.org/2808983002/diff/100001/dbus/bus.cc#newcode90 dbus/bus.cc:90: CHECK(success) << "Unable to allocate memory"; On 2017/04/11 17:05:54, Daniel Erat wrote: > nit: just CHECK() the dbus_watch_handle call on a single line Done.
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc#newcode68 dbus/bus.cc:68: read_watcher_ = base::FileDescriptorWatcher::WatchReadable( On 2017/04/12 01:02:30, satorux1 wrote: > The function comment for WatchReadable() says: > > > // To call these methods, a FileDescriptorWatcher must have been > // instantiated on the current thread and SequencedTaskRunnerHandle::IsSet() > // must return true. > > > It wasn't clear to me where FileDescriptorWatcher is instantiated. I took a > quick look and found that a thread with MessageLoopForIO has an instance: > > https://cs.chromium.org/chromium/src/base/threading/thread.cc?q=new%5C+Filede... > > Is FileDescriptorWatcher also instantiated in "TaskScheduler threads"? > > Hmm, I'm more confused as there are two FileDescriptorWatcher: > > - base::FileDescriptorWatcher > - MessageLoopForIO::FileDescriptorWatcher MessageLoopForIO and TaskScheduler threads instantiate a base::FileDescriptorWatcher and hence allow calls to base::FileDescriptorWatcher::WatchReadable/Writable. "FileDescriptorWatcher" in base/files/file_descriptor_watcher_posix.h refers to base::FileDescriptorWatcher. I want to make base::FileDescriptorWatcher work everywhere and to remove MessageLoopForIO::FileDescriptorWatcher, but there's a lot of work to do before we get there.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/04/12 12:39:48, fdoray wrote: > https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc > File dbus/bus.cc (right): > > https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc#newcode68 > dbus/bus.cc:68: read_watcher_ = base::FileDescriptorWatcher::WatchReadable( > On 2017/04/12 01:02:30, satorux1 wrote: > > The function comment for WatchReadable() says: > > > > > > // To call these methods, a FileDescriptorWatcher must have been > > // instantiated on the current thread and SequencedTaskRunnerHandle::IsSet() > > // must return true. > > > > > > > It wasn't clear to me where FileDescriptorWatcher is instantiated. I took a > > quick look and found that a thread with MessageLoopForIO has an instance: > > > > > https://cs.chromium.org/chromium/src/base/threading/thread.cc?q=new%5C+Filede... > > > > Is FileDescriptorWatcher also instantiated in "TaskScheduler threads"? > > > > Hmm, I'm more confused as there are two FileDescriptorWatcher: > > > > - base::FileDescriptorWatcher > > - MessageLoopForIO::FileDescriptorWatcher > > MessageLoopForIO and TaskScheduler threads instantiate a > base::FileDescriptorWatcher Thanks. Maybe I was just overlooking something but it wasn't clear to me. How about documenting here as a comment, or in the patch description? Would be good to document this in base::FileDescriptorWatcher's header file as well?
Description was changed from ========== Migrate to base::FileDescriptionWatcher in dbus/bus.cc. Before this CL, dbus::Bus::OnAddWatch called MessageLoopForIO::current(), which is illegal from a non-MessageLoopForIO thread. This caused a crash. With this CL, dbus::Bus::OnAddWatch uses base::FileDescriptorWatcher, which works from MessageLoopForIO and TaskScheduler threads. BUG=708142 ========== to ========== Migrate to base::FileDescriptionWatcher in dbus/bus.cc. Before this CL, dbus::Bus::OnAddWatch called MessageLoopForIO::current(), which is illegal from a non-MessageLoopForIO thread. This caused a crash when dbus::Bus::OnAddWatch was called from a TaskScheduler thread. With this CL, dbus::Bus::OnAddWatch uses base::FileDescriptorWatcher, which works from MessageLoopForIO and TaskScheduler threads. BUG=708142 ==========
Description was changed from ========== Migrate to base::FileDescriptionWatcher in dbus/bus.cc. Before this CL, dbus::Bus::OnAddWatch called MessageLoopForIO::current(), which is illegal from a non-MessageLoopForIO thread. This caused a crash when dbus::Bus::OnAddWatch was called from a TaskScheduler thread. With this CL, dbus::Bus::OnAddWatch uses base::FileDescriptorWatcher, which works from MessageLoopForIO and TaskScheduler threads. BUG=708142 ========== to ========== Migrate to base::FileDescriptionWatcher in dbus/bus.cc. Before this CL, dbus::Bus::OnAddWatch called MessageLoopForIO::current()->WatchFileDescriptor(), which is illegal from a non-MessageLoopForIO thread. This caused a crash when dbus::Bus::OnAddWatch was called from a TaskScheduler thread. With this CL, dbus::Bus::OnAddWatch uses base::FileDescriptorWatcher::WatchReadable/Writable instead of MessageLoopForIO::current()->WatchFileDescriptor(). This works from base::Threads that run a MessageLoopForIO and TaskScheduler threads. This CL also instantiates a base::FileDescriptorWatcher in tests that use dbus::Bus::OnAddWatch. This is required to allow usage of base::FileDescriptorWatcher::WatchReadable/Writable on the main thread of a test. BUG=708142 ==========
On 2017/04/12 13:07:30, satorux1 wrote: > On 2017/04/12 12:39:48, fdoray wrote: > > https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc > > File dbus/bus.cc (right): > > > > https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc#newcode68 > > dbus/bus.cc:68: read_watcher_ = base::FileDescriptorWatcher::WatchReadable( > > On 2017/04/12 01:02:30, satorux1 wrote: > > > The function comment for WatchReadable() says: > > > > > > > > > // To call these methods, a FileDescriptorWatcher must have been > > > // instantiated on the current thread and > SequencedTaskRunnerHandle::IsSet() > > > // must return true. > > > > > > > > > > > > It wasn't clear to me where FileDescriptorWatcher is instantiated. I took a > > > quick look and found that a thread with MessageLoopForIO has an instance: > > > > > > > > > https://cs.chromium.org/chromium/src/base/threading/thread.cc?q=new%5C+Filede... > > > > > > Is FileDescriptorWatcher also instantiated in "TaskScheduler threads"? > > > > > > Hmm, I'm more confused as there are two FileDescriptorWatcher: > > > > > > - base::FileDescriptorWatcher > > > - MessageLoopForIO::FileDescriptorWatcher > > > > MessageLoopForIO and TaskScheduler threads instantiate a > > base::FileDescriptorWatcher > > Thanks. Maybe I was just overlooking something but it wasn't clear to me. How > about documenting here as a comment, or in the patch description? Would be good > to document this in base::FileDescriptorWatcher's header file as well? PTAnL I clarified the CL description. I'm working on making FileDescriptorWatcher work from everywhere, so I'd prefer not spending too much time on improving its current documentation.
lgtm with a nit: https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc#newcode1050 dbus/bus.cc:1050: // message_pump_libevent.h. update this comment?
nit: illegal-> invalid in the patch description in favor of not using legal terms? :)
lgtm
Description was changed from ========== Migrate to base::FileDescriptionWatcher in dbus/bus.cc. Before this CL, dbus::Bus::OnAddWatch called MessageLoopForIO::current()->WatchFileDescriptor(), which is illegal from a non-MessageLoopForIO thread. This caused a crash when dbus::Bus::OnAddWatch was called from a TaskScheduler thread. With this CL, dbus::Bus::OnAddWatch uses base::FileDescriptorWatcher::WatchReadable/Writable instead of MessageLoopForIO::current()->WatchFileDescriptor(). This works from base::Threads that run a MessageLoopForIO and TaskScheduler threads. This CL also instantiates a base::FileDescriptorWatcher in tests that use dbus::Bus::OnAddWatch. This is required to allow usage of base::FileDescriptorWatcher::WatchReadable/Writable on the main thread of a test. BUG=708142 ========== to ========== Migrate to base::FileDescriptionWatcher in dbus/bus.cc. Before this CL, dbus::Bus::OnAddWatch called MessageLoopForIO::current()->WatchFileDescriptor(), which is invalid from a non-MessageLoopForIO thread. This caused a crash when dbus::Bus::OnAddWatch was called from a TaskScheduler thread. With this CL, dbus::Bus::OnAddWatch uses base::FileDescriptorWatcher::WatchReadable/Writable instead of MessageLoopForIO::current()->WatchFileDescriptor(). This works from base::Threads that run a MessageLoopForIO and TaskScheduler threads. This CL also instantiates a base::FileDescriptorWatcher in tests that use dbus::Bus::OnAddWatch. This is required to allow usage of base::FileDescriptorWatcher::WatchReadable/Writable on the main thread of a test. BUG=708142 ==========
On 2017/04/14 01:26:48, satorux1 wrote: > nit: illegal-> invalid in the patch description in favor of not using legal > terms? :) Done https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/2808983002/diff/120001/dbus/bus.cc#newcode1050 dbus/bus.cc:1050: // message_pump_libevent.h. On 2017/04/14 01:25:33, satorux1 wrote: > update this comment? Done.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, satorux@chromium.org, hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2808983002/#ps140001 (title: "CR-satorux1-48-update-comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492519436995460,
"parent_rev": "f8ebda9548f2a4bfc00d414f48512327b2d656d7", "commit_rev":
"ebc379c0d84a4c0447bcc95317c85ffee5c9025e"}
Message was sent while issue was closed.
Description was changed from ========== Migrate to base::FileDescriptionWatcher in dbus/bus.cc. Before this CL, dbus::Bus::OnAddWatch called MessageLoopForIO::current()->WatchFileDescriptor(), which is invalid from a non-MessageLoopForIO thread. This caused a crash when dbus::Bus::OnAddWatch was called from a TaskScheduler thread. With this CL, dbus::Bus::OnAddWatch uses base::FileDescriptorWatcher::WatchReadable/Writable instead of MessageLoopForIO::current()->WatchFileDescriptor(). This works from base::Threads that run a MessageLoopForIO and TaskScheduler threads. This CL also instantiates a base::FileDescriptorWatcher in tests that use dbus::Bus::OnAddWatch. This is required to allow usage of base::FileDescriptorWatcher::WatchReadable/Writable on the main thread of a test. BUG=708142 ========== to ========== Migrate to base::FileDescriptionWatcher in dbus/bus.cc. Before this CL, dbus::Bus::OnAddWatch called MessageLoopForIO::current()->WatchFileDescriptor(), which is invalid from a non-MessageLoopForIO thread. This caused a crash when dbus::Bus::OnAddWatch was called from a TaskScheduler thread. With this CL, dbus::Bus::OnAddWatch uses base::FileDescriptorWatcher::WatchReadable/Writable instead of MessageLoopForIO::current()->WatchFileDescriptor(). This works from base::Threads that run a MessageLoopForIO and TaskScheduler threads. This CL also instantiates a base::FileDescriptorWatcher in tests that use dbus::Bus::OnAddWatch. This is required to allow usage of base::FileDescriptorWatcher::WatchReadable/Writable on the main thread of a test. BUG=708142 Review-Url: https://codereview.chromium.org/2808983002 Cr-Commit-Position: refs/heads/master@{#465215} Committed: https://chromium.googlesource.com/chromium/src/+/ebc379c0d84a4c0447bcc95317c8... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/ebc379c0d84a4c0447bcc95317c8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
