Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2195953002:
Adds sync message support to Channel-associated interfaces. (Closed)
|
Created:
4 years, 4 months ago by Ken Rockot(use gerrit already) Modified:
4 years, 4 months ago Reviewers:
yzshen1 CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds sync message support to Channel-associated interfaces.
BUG=612500
Committed: https://crrev.com/9abe09b7586865302936e289200a464118cb9c59
Cr-Commit-Position: refs/heads/master@{#409320}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 14
Patch Set 4 : . #
Total comments: 2
Patch Set 5 : nits #
Dependent Patchsets: Messages
Total messages: 29 (21 generated)
The CQ bit was checked by rockot@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rockot@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...
rockot@chromium.org changed reviewers: + yzshen@chromium.org
Please take a look
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rockot@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.
https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_channel_mojo_un... File ipc/ipc_channel_mojo_unittest.cc (right): https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_channel_mojo_un... ipc/ipc_channel_mojo_unittest.cc:660: int32_t next_expected_value_; nit: please init this value. (here and other classes below) https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_mojo_bootstrap.cc File ipc/ipc_mojo_bootstrap.cc (right): https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:617: this, id, raw_message)); Raw message pointer may not be an appropriate ID: it is possible to create a message at the same address as a message that is already gone. https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:643: scoped_refptr<ChannelAssociatedGroupController> keepalive(this); I think this line is not needed because the base::Callback holds on to a ref of |this|? (and also line 672) https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:679: if (!client) If there is not client, what should we do here? Should we drop the message? Returning directly will leave the sync message in the queue. Do we allow this endpoint to re-attach to a client again? (I guess we don't) If we don't, then this shouldn't be an issue except that it takes up some space for longer. Otherwise, we may not be able to process the messages in the sync message queue. https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_sync_channel.cc File ipc/ipc_sync_channel.cc (right): https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_sync_channel.cc... ipc/ipc_sync_channel.cc:107: void BlockDispatch(bool *dispatch_flag) { dispatch_flag_ = dispatch_flag; } style nit: "*" should be next to "bool" instead of the variable name. https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_sync_channel.cc... ipc/ipc_sync_channel.cc:694: // immediately if woken up by the same the comment doesn't seem to be complete.
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_channel_mojo_un... File ipc/ipc_channel_mojo_unittest.cc (right): https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_channel_mojo_un... ipc/ipc_channel_mojo_unittest.cc:660: int32_t next_expected_value_; On 2016/08/02 at 17:38:37, yzshen1 wrote: > nit: please init this value. (here and other classes below) Done (in all 3 places) https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_mojo_bootstrap.cc File ipc/ipc_mojo_bootstrap.cc (right): https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:617: this, id, raw_message)); On 2016/08/02 at 17:38:37, yzshen1 wrote: > Raw message pointer may not be an appropriate ID: it is possible to create a message at the same address as a message that is already gone. I've switched to using sequential integers. Overflow shouldn't be a problem since it's only possible if we accumulate very long message/task queues (and then we have other problems!) WDYT? https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:643: scoped_refptr<ChannelAssociatedGroupController> keepalive(this); On 2016/08/02 at 17:38:37, yzshen1 wrote: > I think this line is not needed because the base::Callback holds on to a ref of |this|? (and also line 672) Done https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:679: if (!client) On 2016/08/02 at 17:38:37, yzshen1 wrote: > If there is not client, what should we do here? Should we drop the message? Returning directly will leave the sync message in the queue. > > Do we allow this endpoint to re-attach to a client again? (I guess we don't) If we don't, then this shouldn't be an issue except that it takes up some space for longer. Otherwise, we may not be able to process the messages in the sync message queue. Fixed to drop the message in this case
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM (In addition to these new comments below, there are also two nits in ipc_sync_channel.cc that haven't been replied.) https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_mojo_bootstrap.cc File ipc/ipc_mojo_bootstrap.cc (right): https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:617: this, id, raw_message)); On 2016/08/02 18:21:24, Ken Rockot wrote: > On 2016/08/02 at 17:38:37, yzshen1 wrote: > > Raw message pointer may not be an appropriate ID: it is possible to create a > message at the same address as a message that is already gone. > > I've switched to using sequential integers. Overflow shouldn't be a problem > since it's only possible if we accumulate very long message/task queues (and > then we have other problems!) WDYT? SGTM https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_mojo_bootstrap.... ipc/ipc_mojo_bootstrap.cc:679: if (!client) On 2016/08/02 18:21:24, Ken Rockot wrote: > On 2016/08/02 at 17:38:37, yzshen1 wrote: > > If there is not client, what should we do here? Should we drop the message? > Returning directly will leave the sync message in the queue. > > > > Do we allow this endpoint to re-attach to a client again? (I guess we don't) > If we don't, then this shouldn't be an issue except that it takes up some space > for longer. Otherwise, we may not be able to process the messages in the sync > message queue. > > Fixed to drop the message in this case Btw, I think we don't have code to disallow re-attaching an endpoint client currently. I think we should add such code, because we want to guarantee FIFO-ness and we also drop messages in AcceptOnProxyThread/AcceptSyncMessage. (This doesn't block landing this CL.) https://codereview.chromium.org/2195953002/diff/60001/ipc/ipc_channel_mojo_un... File ipc/ipc_channel_mojo_unittest.cc (right): https://codereview.chromium.org/2195953002/diff/60001/ipc/ipc_channel_mojo_un... ipc/ipc_channel_mojo_unittest.cc:994: int32_t response_value_; nit: please init this variable too.
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Thanks https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_sync_channel.cc File ipc/ipc_sync_channel.cc (right): https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_sync_channel.cc... ipc/ipc_sync_channel.cc:107: void BlockDispatch(bool *dispatch_flag) { dispatch_flag_ = dispatch_flag; } On 2016/08/02 at 17:38:37, yzshen1 wrote: > style nit: "*" should be next to "bool" instead of the variable name. Done https://codereview.chromium.org/2195953002/diff/40001/ipc/ipc_sync_channel.cc... ipc/ipc_sync_channel.cc:694: // immediately if woken up by the same On 2016/08/02 at 17:38:37, yzshen1 wrote: > the comment doesn't seem to be complete. Yikes - Done https://codereview.chromium.org/2195953002/diff/60001/ipc/ipc_channel_mojo_un... File ipc/ipc_channel_mojo_unittest.cc (right): https://codereview.chromium.org/2195953002/diff/60001/ipc/ipc_channel_mojo_un... ipc/ipc_channel_mojo_unittest.cc:994: int32_t response_value_; On 2016/08/02 at 18:34:11, yzshen1 wrote: > nit: please init this variable too. 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 unchecked by rockot@chromium.org
The CQ bit was checked by rockot@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2195953002/#ps80001 (title: "nits")
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 #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Adds sync message support to Channel-associated interfaces. BUG=612500 ========== to ========== Adds sync message support to Channel-associated interfaces. BUG=612500 Committed: https://crrev.com/9abe09b7586865302936e289200a464118cb9c59 Cr-Commit-Position: refs/heads/master@{#409320} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9abe09b7586865302936e289200a464118cb9c59 Cr-Commit-Position: refs/heads/master@{#409320} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
