Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2040353002:
[Chromoting] Fix DCHECK when shutting down It2Me host from client side (Closed)
|
Created:
4 years, 6 months ago by Hzj_jie Modified:
4 years, 6 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. |
Description[Chromoting] Fix DCHECK when shutting down It2Me host from client side
This change is to fix a DCHECK issue in It2Me scenario. remote_assistance_host
triggers a DCHECK failure if client disconnects the connection. So this change
modified It2MeHost::Shutdown function to return directly if current state is
kDisconnected. See bug for details.
BUG=617474
Committed: https://crrev.com/f36aac5517dc426a97f23531fd5291aff982dc50
Cr-Commit-Position: refs/heads/master@{#398783}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Resolve review comments #
Total comments: 1
Patch Set 3 : Resolve review comments #
Messages
Total messages: 23 (12 generated)
Description was changed from ========== [Chromogin] Returns directly in Shutdown if current state is kDisconnected This change is to fix a crash issue in It2Me scenario. remote_assistance_host crashes if client disconnects the connection. See http://crbug.com/617474 for details. BUG=617474 ========== to ========== [Chromogin] Returns directly in Shutdown if current state is kDisconnected This change is to fix a crash issue in It2Me scenario. remote_assistance_host crashes if client disconnects the connection. See http://crbug.com/617474 for details. BUG=617474 ==========
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
tow typos in the cl description: - Chromogin - returns (remove s?) The description says that the CL fixes crash, but it's a DCHECK, not crash. Also I suggest changing the title to something like "Fix DCHECK when shutting down It2Me host." Otherwise LGTM https://codereview.chromium.org/2040353002/diff/1/remoting/host/it2me/it2me_h... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2040353002/diff/1/remoting/host/it2me/it2me_h... remoting/host/it2me/it2me_host.cc:106: // called. See http://crbug.com/617474 for details. The link to the bug is not useful here. I would just remove the comment - the code is self-explanatory.
Also you have BUG= in the description. No need to duplicate it with the crbug.com/617474 link
Description was changed from ========== [Chromogin] Returns directly in Shutdown if current state is kDisconnected This change is to fix a crash issue in It2Me scenario. remote_assistance_host crashes if client disconnects the connection. See http://crbug.com/617474 for details. BUG=617474 ========== to ========== [Chromoing] Return directly in Shutdown if current state is kDisconnected This change is to fix a DCHECK issue in It2Me scenario. remote_assistance_host crashes if client disconnects the connection. See bug for details. BUG=617474 ==========
Description was changed from ========== [Chromoing] Return directly in Shutdown if current state is kDisconnected This change is to fix a DCHECK issue in It2Me scenario. remote_assistance_host crashes if client disconnects the connection. See bug for details. BUG=617474 ========== to ========== [Chromoing] Fix DCHECK when shutting down It2Me host from client side This change is to fix a DCHECK issue in It2Me scenario. remote_assistance_host triggers a DCHECK failure if client disconnects the connection. So this change modified It2MeHost::Shutdown function to return directly if current state is kDisconnected. See bug for details. BUG=617474 ==========
https://codereview.chromium.org/2040353002/diff/1/remoting/host/it2me/it2me_h... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2040353002/diff/1/remoting/host/it2me/it2me_h... remoting/host/it2me/it2me_host.cc:106: // called. See http://crbug.com/617474 for details. On 2016/06/08 21:54:04, Sergey Ulanov wrote: > The link to the bug is not useful here. I would just remove the comment - the > code is self-explanatory. I would prefer to keep it, since the condition of returning is a little bit magic to me. i.e. One may need to search for the references of Shutdown function to understand why the Shutdown function may be called twice in some scenarios.
https://codereview.chromium.org/2040353002/diff/1/remoting/host/it2me/it2me_h... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2040353002/diff/1/remoting/host/it2me/it2me_h... remoting/host/it2me/it2me_host.cc:106: // called. See http://crbug.com/617474 for details. On 2016/06/08 23:12:20, Hzj_jie wrote: > On 2016/06/08 21:54:04, Sergey Ulanov wrote: > > The link to the bug is not useful here. I would just remove the comment - the > > code is self-explanatory. > > I would prefer to keep it, since the condition of returning is a little bit > magic to me. i.e. One may need to search for the references of Shutdown function > to understand why the Shutdown function may be called twice in some scenarios. Ok, then I suggest rewording: // Disconnect() may be called even when after the host been // already stopped. Ignore repeated calls. Also it may be useful to rename Shutdown() to DisconnectOnNetworkThread().
The CQ bit was checked by zijiehe@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/2040353002/#ps20001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2040353002/20001
https://codereview.chromium.org/2040353002/diff/20001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2040353002/diff/20001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:106: // Ignore repeated calls. See http://crbug.com/617474 for details. I still think that the bug link is only a distraction here and is not useful. Even if the purpose of this code is still not clear from the comment it can always be traced to this CL and the bug.
The CQ bit was unchecked by zijiehe@chromium.org
On 2016/06/09 01:04:10, Sergey Ulanov wrote: > https://codereview.chromium.org/2040353002/diff/20001/remoting/host/it2me/it2... > File remoting/host/it2me/it2me_host.cc (right): > > https://codereview.chromium.org/2040353002/diff/20001/remoting/host/it2me/it2... > remoting/host/it2me/it2me_host.cc:106: // Ignore repeated calls. See > http://crbug.com/617474 for details. > I still think that the bug link is only a distraction here and is not useful. > Even if the purpose of this code is still not clear from the comment it can > always be traced to this CL and the bug. Sure, updated.
The CQ bit was checked by zijiehe@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/2040353002/#ps40001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2040353002/40001
Message was sent while issue was closed.
Description was changed from ========== [Chromoing] Fix DCHECK when shutting down It2Me host from client side This change is to fix a DCHECK issue in It2Me scenario. remote_assistance_host triggers a DCHECK failure if client disconnects the connection. So this change modified It2MeHost::Shutdown function to return directly if current state is kDisconnected. See bug for details. BUG=617474 ========== to ========== [Chromoing] Fix DCHECK when shutting down It2Me host from client side This change is to fix a DCHECK issue in It2Me scenario. remote_assistance_host triggers a DCHECK failure if client disconnects the connection. So this change modified It2MeHost::Shutdown function to return directly if current state is kDisconnected. See bug for details. BUG=617474 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Chromoing] Fix DCHECK when shutting down It2Me host from client side This change is to fix a DCHECK issue in It2Me scenario. remote_assistance_host triggers a DCHECK failure if client disconnects the connection. So this change modified It2MeHost::Shutdown function to return directly if current state is kDisconnected. See bug for details. BUG=617474 ========== to ========== [Chromoing] Fix DCHECK when shutting down It2Me host from client side This change is to fix a DCHECK issue in It2Me scenario. remote_assistance_host triggers a DCHECK failure if client disconnects the connection. So this change modified It2MeHost::Shutdown function to return directly if current state is kDisconnected. See bug for details. BUG=617474 Committed: https://crrev.com/f36aac5517dc426a97f23531fd5291aff982dc50 Cr-Commit-Position: refs/heads/master@{#398783} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f36aac5517dc426a97f23531fd5291aff982dc50 Cr-Commit-Position: refs/heads/master@{#398783}
Message was sent while issue was closed.
Description was changed from ========== [Chromoing] Fix DCHECK when shutting down It2Me host from client side This change is to fix a DCHECK issue in It2Me scenario. remote_assistance_host triggers a DCHECK failure if client disconnects the connection. So this change modified It2MeHost::Shutdown function to return directly if current state is kDisconnected. See bug for details. BUG=617474 Committed: https://crrev.com/f36aac5517dc426a97f23531fd5291aff982dc50 Cr-Commit-Position: refs/heads/master@{#398783} ========== to ========== [Chromoting] Fix DCHECK when shutting down It2Me host from client side This change is to fix a DCHECK issue in It2Me scenario. remote_assistance_host triggers a DCHECK failure if client disconnects the connection. So this change modified It2MeHost::Shutdown function to return directly if current state is kDisconnected. See bug for details. BUG=617474 Committed: https://crrev.com/f36aac5517dc426a97f23531fd5291aff982dc50 Cr-Commit-Position: refs/heads/master@{#398783} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
