Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2700533002:
CopyOutputRequest must have a working result_callback_ when received over mojo (Closed)
|
Created:
3 years, 10 months ago by Saman Sami Modified:
3 years, 10 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, cc-bugs_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCopyOutputRequest must have a working result_callback_ when received over mojo
BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2700533002
Cr-Commit-Position: refs/heads/master@{#451064}
Committed: https://chromium.googlesource.com/chromium/src/+/4b0ea82167e72d7f04154a60714ff18ba8b65179
Patch Set 1 #
Total comments: 10
Patch Set 2 : c #
Total comments: 6
Patch Set 3 : Addressed Dana's comments #
Total comments: 4
Patch Set 4 : c #
Messages
Total messages: 40 (23 generated)
Description was changed from ========== CopyOutputRequest must have a working result_callback_ when received over mojo BUG=672071 ========== to ========== CopyOutputRequest must have a working result_callback_ when received over mojo BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@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...
Description was changed from ========== CopyOutputRequest must have a working result_callback_ when received over mojo BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== CopyOutputRequest must have a working result_callback_ when received over mojo BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
samans@chromium.org changed reviewers: + danakj@chromium.org, fsamuel@chromium.org, tsepez@chromium.org
samans@chromium.org changed reviewers: - danakj@chromium.org
tsepez@: Please review mojo fsamuel@: Please review all files
lgtm
samans@chromium.org changed reviewers: + danakj@chromium.org
danakj@: Please review cc/ipc/struct_traits_unittest.cc
https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... File cc/ipc/copy_output_request.mojom (right): https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... cc/ipc/copy_output_request.mojom:16: gfx.mojom.Rect? area; btw Rect has no IsNull either https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request_... File cc/ipc/copy_output_request_struct_traits.cc (right): https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request_... cc/ipc/copy_output_request_struct_traits.cc:22: result_callback_.Run(std::move(result)); Can you also make sure this is called only once and called when the pipe breaks with an empty result? That is what the copy request destructor would do. Can you also test those cases?
The CQ bit was checked by samans@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...
PTAL https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... File cc/ipc/copy_output_request.mojom (right): https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... cc/ipc/copy_output_request.mojom:16: gfx.mojom.Rect? area; On 2017/02/15 17:19:57, danakj wrote: > btw Rect has no IsNull either We are using base::Optional<gfx::Rect> in CopyOutputRequest. https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request_... File cc/ipc/copy_output_request_struct_traits.cc (right): https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request_... cc/ipc/copy_output_request_struct_traits.cc:22: result_callback_.Run(std::move(result)); On 2017/02/15 17:19:57, danakj wrote: > Can you also make sure this is called only once and called when the pipe breaks > with an empty result? That is what the copy request destructor would do. > > Can you also test those cases? Will do. It's hard to test though because I have no way of accessing the interface pointer in the unit tests. It's wrapped in a base::Callback and and the callback is hidden inside a CopyOutputRequest, and CopyOutputRequest automatically takes care of everything you said.
https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... File cc/ipc/copy_output_request.mojom (right): https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... cc/ipc/copy_output_request.mojom:16: gfx.mojom.Rect? area; On 2017/02/15 17:36:50, Saman Sami wrote: > On 2017/02/15 17:19:57, danakj wrote: > > btw Rect has no IsNull either > > We are using base::Optional<gfx::Rect> in CopyOutputRequest. Hm, I mean to say that the ? here isn't doing anything though, right? If I learnt how it works correctly last night. Cuz there's no null state for Rect anyhow. If the Rect is optionally present then it should be an Optional<Rect> here? https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request_... File cc/ipc/copy_output_request_struct_traits.cc (right): https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request_... cc/ipc/copy_output_request_struct_traits.cc:22: result_callback_.Run(std::move(result)); On 2017/02/15 17:36:50, Saman Sami wrote: > On 2017/02/15 17:19:57, danakj wrote: > > Can you also make sure this is called only once and called when the pipe > breaks > > with an empty result? That is what the copy request destructor would do. > > > > Can you also test those cases? > > Will do. It's hard to test though because I have no way of accessing the > interface pointer in the unit tests. It's wrapped in a base::Callback and and > the callback is hidden inside a CopyOutputRequest, and CopyOutputRequest > automatically takes care of everything you said. Can you instantiate the CopyOutputResultSenderPtr from result_sender() directly in the test to use it? https://codereview.chromium.org/2700533002/diff/20001/cc/ipc/copy_output_requ... File cc/ipc/copy_output_request_struct_traits.cc (right): https://codereview.chromium.org/2700533002/diff/20001/cc/ipc/copy_output_requ... cc/ipc/copy_output_request_struct_traits.cc:19: : result_callback_(result_callback) {} dcheck the callback isnt null here? https://codereview.chromium.org/2700533002/diff/20001/cc/ipc/copy_output_requ... cc/ipc/copy_output_request_struct_traits.cc:28: base::ResetAndReturn(&result_callback_).Run(std::move(result)); needs to early out if null right?
https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... File cc/ipc/copy_output_request.mojom (right): https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... cc/ipc/copy_output_request.mojom:16: gfx.mojom.Rect? area; On 2017/02/15 17:41:59, danakj wrote: > On 2017/02/15 17:36:50, Saman Sami wrote: > > On 2017/02/15 17:19:57, danakj wrote: > > > btw Rect has no IsNull either > > > > We are using base::Optional<gfx::Rect> in CopyOutputRequest. > > Hm, I mean to say that the ? here isn't doing anything though, right? If I > learnt how it works correctly last night. Cuz there's no null state for Rect > anyhow. > > If the Rect is optionally present then it should be an Optional<Rect> here? Since base::Optional<gfx::Rect> is nullable, it does do something. Without ?, it would crash if the rect does not exist. https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request_... File cc/ipc/copy_output_request_struct_traits.cc (right): https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request_... cc/ipc/copy_output_request_struct_traits.cc:22: result_callback_.Run(std::move(result)); On 2017/02/15 17:41:59, danakj wrote: > On 2017/02/15 17:36:50, Saman Sami wrote: > > On 2017/02/15 17:19:57, danakj wrote: > > > Can you also make sure this is called only once and called when the pipe > > breaks > > > with an empty result? That is what the copy request destructor would do. > > > > > > Can you also test those cases? > > > > Will do. It's hard to test though because I have no way of accessing the > > interface pointer in the unit tests. It's wrapped in a base::Callback and and > > the callback is hidden inside a CopyOutputRequest, and CopyOutputRequest > > automatically takes care of everything you said. > > Can you instantiate the CopyOutputResultSenderPtr from result_sender() directly > in the test to use it? Yeah I think you're actually right. https://codereview.chromium.org/2700533002/diff/20001/cc/ipc/copy_output_requ... File cc/ipc/copy_output_request_struct_traits.cc (right): https://codereview.chromium.org/2700533002/diff/20001/cc/ipc/copy_output_requ... cc/ipc/copy_output_request_struct_traits.cc:19: : result_callback_(result_callback) {} On 2017/02/15 17:41:59, danakj wrote: > dcheck the callback isnt null here? CopyOutputRequest also checks it but ok. https://codereview.chromium.org/2700533002/diff/20001/cc/ipc/copy_output_requ... cc/ipc/copy_output_request_struct_traits.cc:28: base::ResetAndReturn(&result_callback_).Run(std::move(result)); On 2017/02/15 17:41:59, danakj wrote: > needs to early out if null right? There was no early out in CopyOutputRequest::SendResult so I thought this would magically work? https://cs.chromium.org/chromium/src/cc/output/copy_output_request.cc?rcl=806...
https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... File cc/ipc/copy_output_request.mojom (right): https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... cc/ipc/copy_output_request.mojom:16: gfx.mojom.Rect? area; On 2017/02/15 17:51:42, Saman Sami wrote: > On 2017/02/15 17:41:59, danakj wrote: > > On 2017/02/15 17:36:50, Saman Sami wrote: > > > On 2017/02/15 17:19:57, danakj wrote: > > > > btw Rect has no IsNull either > > > > > > We are using base::Optional<gfx::Rect> in CopyOutputRequest. > > > > Hm, I mean to say that the ? here isn't doing anything though, right? If I > > learnt how it works correctly last night. Cuz there's no null state for Rect > > anyhow. > > > > If the Rect is optionally present then it should be an Optional<Rect> here? > > Since base::Optional<gfx::Rect> is nullable, it does do something. Without ?, it > would crash if the rect does not exist. I guess I still don't understand mojo nullable at all. :/ https://codereview.chromium.org/2700533002/diff/20001/cc/ipc/copy_output_requ... File cc/ipc/copy_output_request_struct_traits.cc (right): https://codereview.chromium.org/2700533002/diff/20001/cc/ipc/copy_output_requ... cc/ipc/copy_output_request_struct_traits.cc:28: base::ResetAndReturn(&result_callback_).Run(std::move(result)); On 2017/02/15 17:51:42, Saman Sami wrote: > On 2017/02/15 17:41:59, danakj wrote: > > needs to early out if null right? > > There was no early out in CopyOutputRequest::SendResult so I thought this would > magically work? > https://cs.chromium.org/chromium/src/cc/output/copy_output_request.cc?rcl=806... It would crash, but you don't want bad ipc to cause a crash.
https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... File cc/ipc/copy_output_request.mojom (right): https://codereview.chromium.org/2700533002/diff/1/cc/ipc/copy_output_request.... cc/ipc/copy_output_request.mojom:16: gfx.mojom.Rect? area; On 2017/02/15 17:53:17, danakj wrote: > On 2017/02/15 17:51:42, Saman Sami wrote: > > On 2017/02/15 17:41:59, danakj wrote: > > > On 2017/02/15 17:36:50, Saman Sami wrote: > > > > On 2017/02/15 17:19:57, danakj wrote: > > > > > btw Rect has no IsNull either > > > > > > > > We are using base::Optional<gfx::Rect> in CopyOutputRequest. > > > > > > Hm, I mean to say that the ? here isn't doing anything though, right? If I > > > learnt how it works correctly last night. Cuz there's no null state for Rect > > > anyhow. > > > > > > If the Rect is optionally present then it should be an Optional<Rect> here? > > > > Since base::Optional<gfx::Rect> is nullable, it does do something. Without ?, > it > > would crash if the rect does not exist. > > I guess I still don't understand mojo nullable at all. :/ If CopyOutputRequest had a gfx::Rect there would be no need for ?. But it has a base::Optional<gfx::Rect> which is nullable and area() returns a base::Optional<gfx::Rect> so we need to use ? otherwise when the base::Optional does not have a gfx::Rect the message will be rejected. https://codereview.chromium.org/2700533002/diff/20001/cc/ipc/copy_output_requ... File cc/ipc/copy_output_request_struct_traits.cc (right): https://codereview.chromium.org/2700533002/diff/20001/cc/ipc/copy_output_requ... cc/ipc/copy_output_request_struct_traits.cc:28: base::ResetAndReturn(&result_callback_).Run(std::move(result)); On 2017/02/15 17:53:18, danakj wrote: > On 2017/02/15 17:51:42, Saman Sami wrote: > > On 2017/02/15 17:41:59, danakj wrote: > > > needs to early out if null right? > > > > There was no early out in CopyOutputRequest::SendResult so I thought this > would > > magically work? > > > https://cs.chromium.org/chromium/src/cc/output/copy_output_request.cc?rcl=806... > > It would crash, but you don't want bad ipc to cause a crash. Right. I didn't see any DCHECKs or early outs in CopyOutputRequest so I assumed it won't crash.
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 samans@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...
PTAL
https://codereview.chromium.org/2700533002/diff/40001/cc/ipc/copy_output_requ... File cc/ipc/copy_output_request.mojom (right): https://codereview.chromium.org/2700533002/diff/40001/cc/ipc/copy_output_requ... cc/ipc/copy_output_request.mojom:18: CopyOutputResultSender? result_sender; How come this is nullable? (Sorry for being forever confused about this) https://codereview.chromium.org/2700533002/diff/40001/cc/ipc/copy_output_requ... File cc/ipc/copy_output_request_struct_traits.cc (right): https://codereview.chromium.org/2700533002/diff/40001/cc/ipc/copy_output_requ... cc/ipc/copy_output_request_struct_traits.cc:54: if (request->result_callback_) { This one shouldn't be needed. As you said cc::CopyOutputRequest always has a callback. (https://cs.chromium.org/chromium/src/cc/output/copy_output_request.cc?rcl=064...) But the mojo interface may be used badly. That's why we need the checks above.
mojom itself LGTM, but resolve the usage issues.
https://codereview.chromium.org/2700533002/diff/40001/cc/ipc/copy_output_requ... File cc/ipc/copy_output_request_struct_traits.cc (right): https://codereview.chromium.org/2700533002/diff/40001/cc/ipc/copy_output_requ... cc/ipc/copy_output_request_struct_traits.cc:54: if (request->result_callback_) { On 2017/02/15 19:48:19, danakj wrote: > This one shouldn't be needed. As you said cc::CopyOutputRequest always has a > callback. > (https://cs.chromium.org/chromium/src/cc/output/copy_output_request.cc?rcl=064...) > But the mojo interface may be used badly. That's why we need the checks above. Oh. Uh, I guess there is CreateEmptyRequest() which some unit tests use. I don't think we need to support that with mojo right now. I was just worried about the IPC interface being maliciously used and guarding the receiving side.
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 samans@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2700533002/diff/40001/cc/ipc/copy_output_requ... File cc/ipc/copy_output_request_struct_traits.cc (right): https://codereview.chromium.org/2700533002/diff/40001/cc/ipc/copy_output_requ... cc/ipc/copy_output_request_struct_traits.cc:54: if (request->result_callback_) { On 2017/02/15 19:50:26, danakj wrote: > On 2017/02/15 19:48:19, danakj wrote: > > This one shouldn't be needed. As you said cc::CopyOutputRequest always has a > > callback. > > > (https://cs.chromium.org/chromium/src/cc/output/copy_output_request.cc?rcl=064...) > > But the mojo interface may be used badly. That's why we need the checks above. > > Oh. Uh, I guess there is CreateEmptyRequest() which some unit tests use. I don't > think we need to support that with mojo right now. I was just worried about the > IPC interface being maliciously used and guarding the receiving side. Right. We won't support empty requests.
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
LGTM thanks for the tests!
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2700533002/#ps60001 (title: "c")
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": 60001, "attempt_start_ts": 1487269142281290,
"parent_rev": "1fcb25c7728a2b6a9c8d0454010810484ba13106", "commit_rev":
"4b0ea82167e72d7f04154a60714ff18ba8b65179"}
Message was sent while issue was closed.
Description was changed from ========== CopyOutputRequest must have a working result_callback_ when received over mojo BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== CopyOutputRequest must have a working result_callback_ when received over mojo BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2700533002 Cr-Commit-Position: refs/heads/master@{#451064} Committed: https://chromium.googlesource.com/chromium/src/+/4b0ea82167e72d7f04154a60714f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/4b0ea82167e72d7f04154a60714f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
