Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2207153004:
Fixing a hang in the native messaging hosts on Windows (Closed)
|
Created:
4 years, 4 months ago by joedow Modified:
4 years, 4 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixing a hang in the native messaging hosts on Windows
The It2Me and Me2Me native messaging host processes would hang after reporting
an error and attempting to terminate the process. This appeared to be caused
by the NativeMessageReader component which was blocked on a read operation on
one thread while another thread was waiting for it to exit. The fix is to use
a Windows API to cancel synchronous IO on the blocked thread and then
destroying the inner message reader class.
Also, we should treat an elevate request cancellation the same as a failure to
launch the elevated process. The binary cannot accomplish its task either way
so there isn't any benefit to treating them differently, it's better to allow
the caller to decide what to do in that case (restart the NMH or send another
message to it).
BUG=634218
Committed: https://crrev.com/0edf3b33e71c5cd32e6fe02497ee4f17152a9772
Cr-Commit-Position: refs/heads/master@{#411125}
Patch Set 1 #Patch Set 2 : Merging with ToT #Patch Set 3 : Fixed a problem where member variables were being dereferenced after OnError() was called. #Patch Set 4 : Preventing error logging for expected/ok error condition #
Total comments: 2
Patch Set 5 : Updating unit tests #Patch Set 6 : Fixing posix unittest failures #Patch Set 7 : Adding a comment describing why we need to cancel the pending read operation on Windows. #
Messages
Total messages: 41 (33 generated)
The CQ bit was checked by joedow@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.
The CQ bit was checked by joedow@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.
Description was changed from ========== Fixing a hang in the native messaging hosts on Windows The It2Me and Me2Me native messaging host processes would hang after reporting an error and attempting to terminate the process. This appeared to be caused by the NativeMessageReader component which was blocked on a read operation on one thread while another thread was waiting for it to exit. The fix is to use a Windows API to cancel synchronous IO on the blocked thread and then destroying the inner message reader class. BUG=634218 ========== to ========== Fixing a hang in the native messaging hosts on Windows The It2Me and Me2Me native messaging host processes would hang after reporting an error and attempting to terminate the process. This appeared to be caused by the NativeMessageReader component which was blocked on a read operation on one thread while another thread was waiting for it to exit. The fix is to use a Windows API to cancel synchronous IO on the blocked thread and then destroying the inner message reader class. Also, we should treat an elevate request cancellation the same as a failure to launch the elevated process. The binary cannot accomplish its task either way so there isn't any benefit to treating them differently, it's better to allow the caller to decide what to do in that case (restart the NMH or send another message to it). BUG=634218 ==========
The CQ bit was checked by joedow@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.
The CQ bit was checked by joedow@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...
joedow@chromium.org changed reviewers: + sergeyu@chromium.org
PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you please add a unittest for this case? https://codereview.chromium.org/2207153004/diff/60001/remoting/host/native_me... File remoting/host/native_messaging/native_messaging_reader.cc (right): https://codereview.chromium.org/2207153004/diff/60001/remoting/host/native_me... remoting/host/native_messaging/native_messaging_reader.cc:147: base::Thread::Options options; base::Thread::Options thread_options(base::MessageLoop::TYPE_IO, 0); and remove the line below. Or you can construct Options when calling StartWithOptions: reader_thread_.StartWithOptions( base::Thread::Options(base::MessageLoop::TYPE_IO, 0));
The CQ bit was checked by joedow@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_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 joedow@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.
Added a unit test for the new scenario (also refactored and cleaned up the existing unit tests a bit). One note is that the new unit test only applies to Windows as destroying the Reader class on posix will still hang. I did some testing on Linux and this does not seem to be a problem like it was on Windows (i.e. the NMH process is restarted correctly when it closes its write pipe). I can see a method for applying a similar fix to Linux, but it actually requires a fair amount of work to make it possible to cancel the blocking read. Let me know if you think that is worth doing as well. https://codereview.chromium.org/2207153004/diff/60001/remoting/host/native_me... File remoting/host/native_messaging/native_messaging_reader.cc (right): https://codereview.chromium.org/2207153004/diff/60001/remoting/host/native_me... remoting/host/native_messaging/native_messaging_reader.cc:147: base::Thread::Options options; On 2016/08/09 15:39:58, Sergey Ulanov wrote: > base::Thread::Options thread_options(base::MessageLoop::TYPE_IO, 0); > and remove the line below. Or you can construct Options when calling > StartWithOptions: > reader_thread_.StartWithOptions( > base::Thread::Options(base::MessageLoop::TYPE_IO, 0)); Done.
I don't see a good reason to implement the same fix for posix. Maybe add comment in ~NativeMessagingReader() to make it clear why we cancel IO only on Windows. LGTM
The CQ bit was checked by joedow@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...
Added a comment block in the d'tor. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2207153004/#ps120001 (title: "Adding a comment describing why we need to cancel the pending read operation on Windows.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fixing a hang in the native messaging hosts on Windows The It2Me and Me2Me native messaging host processes would hang after reporting an error and attempting to terminate the process. This appeared to be caused by the NativeMessageReader component which was blocked on a read operation on one thread while another thread was waiting for it to exit. The fix is to use a Windows API to cancel synchronous IO on the blocked thread and then destroying the inner message reader class. Also, we should treat an elevate request cancellation the same as a failure to launch the elevated process. The binary cannot accomplish its task either way so there isn't any benefit to treating them differently, it's better to allow the caller to decide what to do in that case (restart the NMH or send another message to it). BUG=634218 ========== to ========== Fixing a hang in the native messaging hosts on Windows The It2Me and Me2Me native messaging host processes would hang after reporting an error and attempting to terminate the process. This appeared to be caused by the NativeMessageReader component which was blocked on a read operation on one thread while another thread was waiting for it to exit. The fix is to use a Windows API to cancel synchronous IO on the blocked thread and then destroying the inner message reader class. Also, we should treat an elevate request cancellation the same as a failure to launch the elevated process. The binary cannot accomplish its task either way so there isn't any benefit to treating them differently, it's better to allow the caller to decide what to do in that case (restart the NMH or send another message to it). BUG=634218 Committed: https://crrev.com/0edf3b33e71c5cd32e6fe02497ee4f17152a9772 Cr-Commit-Position: refs/heads/master@{#411125} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0edf3b33e71c5cd32e6fe02497ee4f17152a9772 Cr-Commit-Position: refs/heads/master@{#411125} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
