Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
554363004:
ChannelMojo: Handle errors in pending message processing. (Closed)
|
Created:
6 years, 3 months ago by Hajime Morrita Modified:
6 years, 3 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionChannelMojo: Handle errors in pending message processing.
ChannelMojo::OnConnect() ignores errors in Send() but the error
results deleting |message_readrer_| which causes null access.
This CL add an error check for that.
This also adds some hooks to make this testable by
faking lower level API.
TEST=ipc_channel_mojo_unittest.cc
BUG=410813
R=yzshen@chromium.org, viettrungluu@chromium.org
Committed: https://crrev.com/0a24cfc91484cc092603abf3863690dafe61311e
Cr-Commit-Position: refs/heads/master@{#294997}
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Patch Set 3 : #
Total comments: 7
Patch Set 4 : #
Messages
Total messages: 19 (5 generated)
Trung, Yuzhu, Could you take a look at this crash fix? The fix itself is trivial. I added some quirks for testing though.
Trung, could you take a look at this crash fix? On 2014/09/12 20:21:58, morrita wrote: > Trung, Yuzhu, Could you take a look at this crash fix? > The fix itself is trivial. I added some quirks for > testing though.
https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo.cc File ipc/mojo/ipc_channel_mojo.cc (right): https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo.cc... ipc/mojo/ipc_channel_mojo.cc:228: ChannelMojo::CreateMessageReader( nit: you could merge this line onto the previous one. https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo_re... File ipc/mojo/ipc_channel_mojo_readers.h (right): https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo_re... ipc/mojo/ipc_channel_mojo_readers.h:42: virtual MojoResult WriteMessageToPipe( Is it possible to do it in a different way? For example, forcefully close the handle to trigger a write error? I am a little afraid making it virtual is more expensive than before, and this is on the performance critical path. What do you think? https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo_un... File ipc/mojo/ipc_channel_mojo_unittest.cc (right): https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo_un... ipc/mojo/ipc_channel_mojo_unittest.cc:166: unnecessary empty line https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo_un... ipc/mojo/ipc_channel_mojo_unittest.cc:169: // Exists to create ErraticMessageReader nit: trailing '.', please. (and line 188, 259) https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo_un... ipc/mojo/ipc_channel_mojo_unittest.cc:278: // This messages are queued as pending. This -> These
Thanks for the review Yuzhu! PTAL? https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo.cc File ipc/mojo/ipc_channel_mojo.cc (right): https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo.cc... ipc/mojo/ipc_channel_mojo.cc:228: ChannelMojo::CreateMessageReader( On 2014/09/15 17:26:07, yzshen1 wrote: > nit: you could merge this line onto the previous one. Done. https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo_re... File ipc/mojo/ipc_channel_mojo_readers.h (right): https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo_re... ipc/mojo/ipc_channel_mojo_readers.h:42: virtual MojoResult WriteMessageToPipe( On 2014/09/15 17:26:07, yzshen1 wrote: > Is it possible to do it in a different way? For example, forcefully close the > handle to trigger a write error? I am a little afraid making it virtual is more > expensive than before, and this is on the performance critical path. What do you > think? Sounds good. I made change and things got much simpler. Thanks for the suggestion! https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo_un... File ipc/mojo/ipc_channel_mojo_unittest.cc (right): https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo_un... ipc/mojo/ipc_channel_mojo_unittest.cc:166: On 2014/09/15 17:26:08, yzshen1 wrote: > unnecessary empty line Done. https://codereview.chromium.org/554363004/diff/1/ipc/mojo/ipc_channel_mojo_un... ipc/mojo/ipc_channel_mojo_unittest.cc:169: // Exists to create ErraticMessageReader On 2014/09/15 17:26:08, yzshen1 wrote: > nit: trailing '.', please. (and line 188, 259) Done.
Thanks! Only a few more nits. https://codereview.chromium.org/554363004/diff/40001/ipc/mojo/ipc_channel_moj... File ipc/mojo/ipc_channel_mojo_unittest.cc (right): https://codereview.chromium.org/554363004/diff/40001/ipc/mojo/ipc_channel_moj... ipc/mojo/ipc_channel_mojo_unittest.cc:153: const IPC::ChannelHandle &channel_handle, nit: '&' should be adjacent to the type name. https://codereview.chromium.org/554363004/diff/40001/ipc/mojo/ipc_channel_moj... ipc/mojo/ipc_channel_mojo_unittest.cc:166: // Exists to create ErraticChannelMojo trailing '.', please. https://codereview.chromium.org/554363004/diff/40001/ipc/mojo/ipc_channel_moj... ipc/mojo/ipc_channel_mojo_unittest.cc:237: // A long running process that connects to us trailing '.', please. https://codereview.chromium.org/554363004/diff/40001/ipc/mojo/ipc_channel_moj... ipc/mojo/ipc_channel_mojo_unittest.cc:263: // crbug.com/410813 I think if this CL has fixed the bug, you don't need to mention it. Instead, you comment about the expected behavior.
Thanks for helping me polish it! PTAL? https://codereview.chromium.org/554363004/diff/40001/ipc/mojo/ipc_channel_moj... File ipc/mojo/ipc_channel_mojo_unittest.cc (right): https://codereview.chromium.org/554363004/diff/40001/ipc/mojo/ipc_channel_moj... ipc/mojo/ipc_channel_mojo_unittest.cc:153: const IPC::ChannelHandle &channel_handle, On 2014/09/15 18:26:22, yzshen1 wrote: > nit: '&' should be adjacent to the type name. Done. https://codereview.chromium.org/554363004/diff/40001/ipc/mojo/ipc_channel_moj... ipc/mojo/ipc_channel_mojo_unittest.cc:166: // Exists to create ErraticChannelMojo On 2014/09/15 18:26:22, yzshen1 wrote: > trailing '.', please. Oops. I overlooked this one :-( Fixed. https://codereview.chromium.org/554363004/diff/40001/ipc/mojo/ipc_channel_moj... ipc/mojo/ipc_channel_mojo_unittest.cc:263: // crbug.com/410813 On 2014/09/15 18:26:22, yzshen1 wrote: > I think if this CL has fixed the bug, you don't need to mention it. Instead, you > comment about the expected behavior. Right. Removed.
lgtm
The CQ bit was checked by morrita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/554363004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/56111) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by morrita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/554363004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by morrita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/554363004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 72f8c225ea39c0743a9665441c92aa0678bf3bd7
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0a24cfc91484cc092603abf3863690dafe61311e Cr-Commit-Position: refs/heads/master@{#294997} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
