Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2516243002:
CastTransportIPC can send messages to uninitialized channel id. (Closed)
|
Created:
4 years, 1 month ago by dhna Modified:
4 years, 1 month ago CC:
chromium-reviews, imcheng+watch_chromium.org, jasonroberts+watch_google.com, avayvod+watch_chromium.org, feature-media-reviews_chromium.org, xjz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCastTransportIPC can send messages to uninitialized channel id.
BUG=660661
Committed: https://crrev.com/851a15bc4a14c8fc9867cf9a29da6e7b1acb79ee
Cr-Commit-Position: refs/heads/master@{#433732}
Patch Set 1 #
Total comments: 2
Patch Set 2 : miu comment #
Total comments: 2
Patch Set 3 : nit fix #Messages
Total messages: 28 (18 generated)
The CQ bit was checked by jinho.bang@samsung.com 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...
corona10@gmail.com changed reviewers: + xhwang@chromium.org - jinho.bang@samsung.com
corona10@gmail.com changed reviewers: + jinho.bang@samsung.com
xhwang@ PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== CastTransportIPC can send messages to uninitialized channel id. BUG=660661 ========== to ========== CastTransportIPC can send messages to uninitialized channel id. BUG=660661 ==========
xhwang@chromium.org changed reviewers: + miu@chromium.org, zino@chromium.org - jinho.bang@samsung.com, xhwang@chromium.org
xhwang -> miu, a much better owner for this file miu: PTAL
Thanks for catching and fixing this issue! :) Comments on Patch Set 1: https://codereview.chromium.org/2516243002/diff/1/chrome/renderer/media/cast_... File chrome/renderer/media/cast_transport_ipc.cc (right): https://codereview.chromium.org/2516243002/diff/1/chrome/renderer/media/cast_... chrome/renderer/media/cast_transport_ipc.cc:28: if (CastIPCDispatcher::Get()) { Note: It's unfortunate that the original author of the code short-cutted this here. (Really, CastIPCDispatcher should be provided as a ctor argument.) Could you add a TODO comment here for future clean-up? https://codereview.chromium.org/2516243002/diff/1/chrome/renderer/media/cast_... chrome/renderer/media/cast_transport_ipc.cc:194: if (CastIPCDispatcher::Get()) { Suggestion for simplifying: If you change this to the following, you can remove all the checks for "channel_id_ != -1" in the dtor and methods above: if (CastIPCDispatcher::Get() && channel_id_ != -1) { ...
corona10@gmail.com changed reviewers: + jinho.bang@samsung.com - zino@chromium.org
On 2016/11/21 20:21:50, miu wrote: > Thanks for catching and fixing this issue! :) > > Comments on Patch Set 1: > > https://codereview.chromium.org/2516243002/diff/1/chrome/renderer/media/cast_... > File chrome/renderer/media/cast_transport_ipc.cc (right): > > https://codereview.chromium.org/2516243002/diff/1/chrome/renderer/media/cast_... > chrome/renderer/media/cast_transport_ipc.cc:28: if (CastIPCDispatcher::Get()) { > Note: It's unfortunate that the original author of the code short-cutted this > here. (Really, CastIPCDispatcher should be provided as a ctor argument.) Could > you add a TODO comment here for future clean-up? > > https://codereview.chromium.org/2516243002/diff/1/chrome/renderer/media/cast_... > chrome/renderer/media/cast_transport_ipc.cc:194: if (CastIPCDispatcher::Get()) { > Suggestion for simplifying: If you change this to the following, you can remove > all the checks for "channel_id_ != -1" in the dtor and methods above: > > if (CastIPCDispatcher::Get() && channel_id_ != -1) { > ... miu@ Thanks for review my code. Done it as your comment. PTAL :-)
PS2 lgtm % nits: https://codereview.chromium.org/2516243002/diff/20001/chrome/renderer/media/c... File chrome/renderer/media/cast_transport_ipc.cc (right): https://codereview.chromium.org/2516243002/diff/20001/chrome/renderer/media/c... chrome/renderer/media/cast_transport_ipc.cc:29: // TODO: CastIPCDispatcher should be provided as a ctor argument. nit: In Chromium, TODO comments should have author logins (so we know who to poke). Since I asked for this, feel free to make this: // TODO(miu): CastIPCDispatcher should be provided as a ctor argument. https://codereview.chromium.org/2516243002/diff/20001/chrome/renderer/media/c... chrome/renderer/media/cast_transport_ipc.cc:37: if (channel_id_ != -1) { nit: This check isn't needed either (since Send() checks for the -1 value).
The CQ bit was checked by corona10@gmail.com to run a CQ dry run
On 2016/11/21 21:11:54, miu wrote: > PS2 lgtm % nits: > > https://codereview.chromium.org/2516243002/diff/20001/chrome/renderer/media/c... > File chrome/renderer/media/cast_transport_ipc.cc (right): > > https://codereview.chromium.org/2516243002/diff/20001/chrome/renderer/media/c... > chrome/renderer/media/cast_transport_ipc.cc:29: // TODO: CastIPCDispatcher > should be provided as a ctor argument. > nit: In Chromium, TODO comments should have author logins (so we know who to > poke). Since I asked for this, feel free to make this: > > // TODO(miu): CastIPCDispatcher should be provided as a ctor argument. > > https://codereview.chromium.org/2516243002/diff/20001/chrome/renderer/media/c... > chrome/renderer/media/cast_transport_ipc.cc:37: if (channel_id_ != -1) { > nit: This check isn't needed either (since Send() checks for the -1 value). miu@ Thanks I fix it :-).
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 corona10@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org Link to the patchset: https://codereview.chromium.org/2516243002/#ps40001 (title: "nit fix")
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": 40001, "attempt_start_ts": 1479778335021590,
"parent_rev": "49b2217f1f0cebf3aed78890744f91156bc6be5f", "commit_rev":
"bcf49f03cca104c930deadd3134f63b88723cd48"}
Message was sent while issue was closed.
Description was changed from ========== CastTransportIPC can send messages to uninitialized channel id. BUG=660661 ========== to ========== CastTransportIPC can send messages to uninitialized channel id. BUG=660661 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== CastTransportIPC can send messages to uninitialized channel id. BUG=660661 ========== to ========== CastTransportIPC can send messages to uninitialized channel id. BUG=660661 Committed: https://crrev.com/851a15bc4a14c8fc9867cf9a29da6e7b1acb79ee Cr-Commit-Position: refs/heads/master@{#433732} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/851a15bc4a14c8fc9867cf9a29da6e7b1acb79ee Cr-Commit-Position: refs/heads/master@{#433732} |
