Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
759923003:
Detect situation when there is no missing send completion signal in P2PSocket implementation. (Closed)
|
Created:
6 years ago by guoweis_left_chromium Modified:
5 years, 11 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, jam, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded a CHECK assert which will cause crash when missing send completion signal is detected.
1. change in_fligth_packets_size_ to not only tracking size but also packet_id
2. pass back the packet_id in the path of completion signal such that it could
be matched with what's been tracked in in_flight_packets_size
BUG=427555
Committed: https://crrev.com/467bab46baa301ea4d21097c3214f44fd184871a
Cr-Commit-Position: refs/heads/master@{#310844}
Patch Set 1 : #Patch Set 2 : #Patch Set 3 : #
Total comments: 9
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Total comments: 20
Patch Set 7 : #
Total comments: 10
Patch Set 8 : #Messages
Total messages: 51 (23 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
guoweis@chromium.org changed reviewers: + juberti@google.com
1. change in_fligth_packets_size_ to not only tracking size but also packet_id 2. pass back the packet_id in the path of completion signal such that it could be matched with what's been tracked in in_flight_packets_size 3. add a heuristic to detect when a packet doesn't have it completion signal returned and report UMA for that.
guoweis@chromium.org changed reviewers: + juberti@chromium.org - juberti@google.com
+juberti@chromium as well.
guoweis@chromium.org changed reviewers: + rkaplow@chromium.org
+rkaplow for histograms.xml
rkaplow@chromium.org changed reviewers: + asvitkine@google.com
https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:653: // rely on the destructor since if user closes the tab, destructor is not if a user https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:653: // rely on the destructor since if user closes the tab, destructor is not the destructor https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:654: // invoked. Or if user just inputs another url, destructor will be called a/the user https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:661: UMA_HISTOGRAM_COUNTS("WebRTC.ApplicationSocketMismatchPacketsDetected_UDP", instead of using a numeric histogram, it seems to me you want to see when completion signal was not returned. You can use UMA_HISTOGRAM_BOOLEAN for something like this: ie socket_mismatch = send_metrics.packet_id != 0 && in_flight_packet_records_.front().packet_id + 100 < send_metrics.packet_id and then UMA_HISTOGRAM_BOOLEAN("WebRTC.ApplicationSocketMismatchPacketsDetected_UDP", socket_mismatch) or something like that. The advantage (other than allocated less space) is you will actually mark down how many times it did not occur. If you're not interested in that case then you can just log true inside your if clause. https://codereview.chromium.org/759923003/diff/180001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/759923003/diff/180001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40197: + This counter detects the condition when a packet is sent from application Reword this a bit? when a packet is sent from *the* application later to *the* system layer but has *not* received the completion signal for the packet to replenished the *sent* quota. Also not sure what replenished the sent quota but I guess someone looking at this histogram would understand that better.
Patchset #4 (id:200001) has been deleted
Patchset #4 (id:220001) has been deleted
On 2014/12/03 22:30:27, rkaplow wrote: > https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... > File content/renderer/p2p/ipc_socket_factory.cc (right): > > https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... > content/renderer/p2p/ipc_socket_factory.cc:653: // rely on the destructor since > if user closes the tab, destructor is not > if a user > > https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... > content/renderer/p2p/ipc_socket_factory.cc:653: // rely on the destructor since > if user closes the tab, destructor is not > the destructor > > https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... > content/renderer/p2p/ipc_socket_factory.cc:654: // invoked. Or if user just > inputs another url, destructor will be called > a/the user > > https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... > content/renderer/p2p/ipc_socket_factory.cc:661: > UMA_HISTOGRAM_COUNTS("WebRTC.ApplicationSocketMismatchPacketsDetected_UDP", > instead of using a numeric histogram, it seems to me you want to see when > completion signal was not returned. > You can use UMA_HISTOGRAM_BOOLEAN for something like this: > > ie socket_mismatch = send_metrics.packet_id != 0 && > in_flight_packet_records_.front().packet_id + 100 < > send_metrics.packet_id > and then > UMA_HISTOGRAM_BOOLEAN("WebRTC.ApplicationSocketMismatchPacketsDetected_UDP", > socket_mismatch) > > or something like that. The advantage (other than allocated less space) is you > will actually mark down how many times it did not occur. > If you're not interested in that case then you can just log true inside your if > clause. > > https://codereview.chromium.org/759923003/diff/180001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/759923003/diff/180001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:40197: + This counter detects the > condition when a packet is sent from application > Reword this a bit? > > when a packet is sent from *the* application later to *the* system layer but has > *not* received the completion signal for the packet to replenished the *sent* > quota. > > Also not sure what replenished the sent quota but I guess someone looking at > this histogram would understand that better. PTAL.
PTAL. https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:653: // rely on the destructor since if user closes the tab, destructor is not On 2014/12/03 22:30:26, rkaplow wrote: > if a user Done. https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:653: // rely on the destructor since if user closes the tab, destructor is not On 2014/12/03 22:30:26, rkaplow wrote: > the destructor Done. https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:654: // invoked. Or if user just inputs another url, destructor will be called On 2014/12/03 22:30:26, rkaplow wrote: > a/the user Done. https://codereview.chromium.org/759923003/diff/180001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:661: UMA_HISTOGRAM_COUNTS("WebRTC.ApplicationSocketMismatchPacketsDetected_UDP", On 2014/12/03 22:30:26, rkaplow wrote: > instead of using a numeric histogram, it seems to me you want to see when > completion signal was not returned. > You can use UMA_HISTOGRAM_BOOLEAN for something like this: > > ie socket_mismatch = send_metrics.packet_id != 0 && > in_flight_packet_records_.front().packet_id + 100 < > send_metrics.packet_id > and then > UMA_HISTOGRAM_BOOLEAN("WebRTC.ApplicationSocketMismatchPacketsDetected_UDP", > socket_mismatch) > > or something like that. The advantage (other than allocated less space) is you > will actually mark down how many times it did not occur. > If you're not interested in that case then you can just log true inside your if > clause. Done.
histogram part seems good to me. I'm not actually an owner of histograms.xml, I can ping avitkine to get a look as well. https://codereview.chromium.org/759923003/diff/240001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/759923003/diff/240001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:653: // rely on the destructor since if a user close the tab, destructor is not well now that you've switched to 'a user', it should be if a user closes. Also, you're missing "the" destructor is not invoked
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/759923003/diff/240001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/759923003/diff/240001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40194: +<histogram name="WebRTC.ApplicationSocketMismatchPacketsDetected"> Please specify an enum="" tag here. It sounds like the best one to use is "BooleanDetected". If it doesn't exist, you can add the <enum> entry for it (i.e. similar to existing ones like BooleanMatched).
PTAL https://codereview.chromium.org/759923003/diff/240001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/759923003/diff/240001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:653: // rely on the destructor since if a user close the tab, destructor is not On 2014/12/09 20:19:37, rkaplow wrote: > well now that you've switched to 'a user', it should be > if a user closes. > Also, you're missing "the" destructor is not invoked > Ah, sorry about this. Done. https://codereview.chromium.org/759923003/diff/240001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/759923003/diff/240001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40194: +<histogram name="WebRTC.ApplicationSocketMismatchPacketsDetected"> On 2014/12/09 20:22:55, Alexei Svitkine wrote: > Please specify an enum="" tag here. It sounds like the best one to use is > "BooleanDetected". If it doesn't exist, you can add the <enum> entry for it > (i.e. similar to existing ones like BooleanMatched). Done.
lgtm
guoweis@chromium.org changed reviewers: + sergeyu@chromium.org - juberti@chromium.org
Sergey, could you take a look at this change? This is to rule out any possibility that we might get a leak situation on send_bytes_available if OS doesn't call back to us.
https://codereview.chromium.org/759923003/diff/260001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/759923003/diff/260001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:346: id_, P2PSendPacketMetrics(packet_id /* packet_id */))); Don't need the comment. https://codereview.chromium.org/759923003/diff/260001/content/common/p2p_sock... File content/common/p2p_socket_type.h (right): https://codereview.chromium.org/759923003/diff/260001/content/common/p2p_sock... content/common/p2p_socket_type.h:53: // timestamps when packet arrives at various points. Are you actually planning to add other fields in this struct, or are you adding it just in case? If you don't currently plan to add other fields here then I don't think you need it. Just pass packet_id. https://codereview.chromium.org/759923003/diff/260001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/759923003/diff/260001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:195: // In normal case without reordering, the first packet should always be the I do not understand how reordering might happen. Shouldn't browser always send OnSendComplete() in the same order it received data from the renderer? And if it doesn't reorder then you don't need to pass packet_id back to the renderer.
PTAL. https://codereview.chromium.org/759923003/diff/260001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/759923003/diff/260001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:346: id_, P2PSendPacketMetrics(packet_id /* packet_id */))); On 2014/12/16 00:21:43, Sergey Ulanov wrote: > Don't need the comment. Done. https://codereview.chromium.org/759923003/diff/260001/content/common/p2p_sock... File content/common/p2p_socket_type.h (right): https://codereview.chromium.org/759923003/diff/260001/content/common/p2p_sock... content/common/p2p_socket_type.h:53: // timestamps when packet arrives at various points. On 2014/12/16 00:21:43, Sergey Ulanov wrote: > Are you actually planning to add other fields in this struct, or are you adding > it just in case? If you don't currently plan to add other fields here then I > don't think you need it. Just pass packet_id. Yes, one other thing I'd like to track in the future is where time spent. https://codereview.chromium.org/759923003/diff/260001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/759923003/diff/260001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:195: // In normal case without reordering, the first packet should always be the On 2014/12/16 00:21:43, Sergey Ulanov wrote: > I do not understand how reordering might happen. Shouldn't browser always send > OnSendComplete() in the same order it received data from the renderer? > > And if it doesn't reorder then you don't need to pass packet_id back to the > renderer. Yes, reordering probably shouldn't happen. I was concerned about on windows, when doing overlapped IO, I'm not 100% sure whether the call back will be in sequence or not. However, with more understanding, I realized that can't happen. I'm not sure I understand your commetn about not passing packet_id, even if reordering is not possible. Without the packet_id, how do we detect a callback is skipped?
https://codereview.chromium.org/759923003/diff/280001/content/common/p2p_sock... File content/common/p2p_socket_type.h (right): https://codereview.chromium.org/759923003/diff/280001/content/common/p2p_sock... content/common/p2p_socket_type.h:58: uint64 packet_id; please use uint64_t instead of uint64 here and everywhere else in this CL. (uint64_t is standard, but in the past it wasn't supported in some toolchains. It's not a problem anymore so it's better to use the standard types in new code.) https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:93: uint64 timeticks_received_by_socket; use base::TimeTicks type for this. Is this used anywhere? https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:194: // This is a sorted linked list of all InFlightPacketRecord by its packet_id. You are not actually sorting them by packet_id. They are in the order in which they were sent. It's a coincidence that it happens to be sorted by packet_id because of the way P2PSocketClientImpl() allocates packet IDs. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:197: // packets that we never receive its completion signal from OS. If I understand correctly the purpose of this CL is to be able to detect bugs in IPC layer, i.e. when the browser process doesn't send SendComplete notification back to the renderer process. And if that's right, then the last sentence of this comment doesn't make much sense. Suggest rewording "Used to detect when browser doesn't send SendComplete message for some packets." https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:455: // Since OnSendComplete happens on the same thread, there is no chance that nit: I don't think you really need this comment. It should be obvious. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:631: if (send_metrics.packet_id == 0) { When does this happen? Why do you need this? https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:636: it = FindInFlightRecord(send_metrics.packet_id); I don't think you need this because we never expect out-of-order delivery of IPC messages. Just get the first packet and DCHECK() that packet_id is the same. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:657: UMA_HISTOGRAM_BOOLEAN("WebRTC.ApplicationSocketMismatchPacketsDetected_UDP", I don't think you need UMA metric for this. Just CHECK() here and see if we get crash reports. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... File content/renderer/p2p/socket_client.h (right): https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... content/renderer/p2p/socket_client.h:40: virtual void Send(const net::IPEndPoint& address, This method is no longer used anywhere. Can you please remove it and rename SendWithDscp() to Send()? Otherwise it makes no sense that SendWithDscp() returns id and this method doesn't https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... File content/renderer/p2p/socket_client_impl.cc (right): https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... content/renderer/p2p/socket_client_impl.cc:97: DCHECK_EQ(state_, STATE_OPEN); I don't think this DCHECK is valid. SendWithDscp() posts the task without verifying the state and it may change before this task is executed.
Patchset #7 (id:300001) has been deleted
Patchset #7 (id:320001) has been deleted
Patchset #7 (id:340001) has been deleted
On 2014/12/17 20:05:30, Sergey Ulanov wrote: > https://codereview.chromium.org/759923003/diff/280001/content/common/p2p_sock... > File content/common/p2p_socket_type.h (right): > > https://codereview.chromium.org/759923003/diff/280001/content/common/p2p_sock... > content/common/p2p_socket_type.h:58: uint64 packet_id; > please use uint64_t instead of uint64 here and everywhere else in this CL. > > (uint64_t is standard, but in the past it wasn't supported in some toolchains. > It's not a problem anymore so it's better to use the standard types in new > code.) > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... > File content/renderer/p2p/ipc_socket_factory.cc (right): > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... > content/renderer/p2p/ipc_socket_factory.cc:93: uint64 > timeticks_received_by_socket; > use base::TimeTicks type for this. Is this used anywhere? > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... > content/renderer/p2p/ipc_socket_factory.cc:194: // This is a sorted linked list > of all InFlightPacketRecord by its packet_id. > You are not actually sorting them by packet_id. They are in the order in which > they were sent. It's a coincidence that it happens to be sorted by packet_id > because of the way P2PSocketClientImpl() allocates packet IDs. > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... > content/renderer/p2p/ipc_socket_factory.cc:197: // packets that we never receive > its completion signal from OS. > If I understand correctly the purpose of this CL is to be able to detect bugs in > IPC layer, i.e. when the browser process doesn't send SendComplete notification > back to the renderer process. And if that's right, then the last sentence of > this comment doesn't make much sense. > Suggest rewording "Used to detect when browser doesn't send SendComplete message > for some packets." > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... > content/renderer/p2p/ipc_socket_factory.cc:455: // Since OnSendComplete happens > on the same thread, there is no chance that > nit: I don't think you really need this comment. It should be obvious. > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... > content/renderer/p2p/ipc_socket_factory.cc:631: if (send_metrics.packet_id == 0) > { > When does this happen? Why do you need this? > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... > content/renderer/p2p/ipc_socket_factory.cc:636: it = > FindInFlightRecord(send_metrics.packet_id); > I don't think you need this because we never expect out-of-order delivery of IPC > messages. Just get the first packet and DCHECK() that packet_id is the same. > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... > content/renderer/p2p/ipc_socket_factory.cc:657: > UMA_HISTOGRAM_BOOLEAN("WebRTC.ApplicationSocketMismatchPacketsDetected_UDP", > I don't think you need UMA metric for this. Just CHECK() here and see if we get > crash reports. > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... > File content/renderer/p2p/socket_client.h (right): > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... > content/renderer/p2p/socket_client.h:40: virtual void Send(const > net::IPEndPoint& address, > This method is no longer used anywhere. Can you please remove it and rename > SendWithDscp() to Send()? Otherwise it makes no sense that SendWithDscp() > returns id and this method doesn't > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... > File content/renderer/p2p/socket_client_impl.cc (right): > > https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... > content/renderer/p2p/socket_client_impl.cc:97: DCHECK_EQ(state_, STATE_OPEN); > I don't think this DCHECK is valid. SendWithDscp() posts the task without > verifying the state and it may change before this task is executed. PTAL.
publish the comments. https://codereview.chromium.org/759923003/diff/280001/content/common/p2p_sock... File content/common/p2p_socket_type.h (right): https://codereview.chromium.org/759923003/diff/280001/content/common/p2p_sock... content/common/p2p_socket_type.h:58: uint64 packet_id; On 2014/12/17 20:05:30, Sergey Ulanov wrote: > please use uint64_t instead of uint64 here and everywhere else in this CL. > > (uint64_t is standard, but in the past it wasn't supported in some toolchains. > It's not a problem anymore so it's better to use the standard types in new > code.) Done. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:93: uint64 timeticks_received_by_socket; On 2014/12/17 20:05:30, Sergey Ulanov wrote: > use base::TimeTicks type for this. Is this used anywhere? Removed. This should been another change which tracks the time, not this one. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:194: // This is a sorted linked list of all InFlightPacketRecord by its packet_id. On 2014/12/17 20:05:30, Sergey Ulanov wrote: > You are not actually sorting them by packet_id. They are in the order in which > they were sent. It's a coincidence that it happens to be sorted by packet_id > because of the way P2PSocketClientImpl() allocates packet IDs. > Removed that comment. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:197: // packets that we never receive its completion signal from OS. On 2014/12/17 20:05:30, Sergey Ulanov wrote: > If I understand correctly the purpose of this CL is to be able to detect bugs in > IPC layer, i.e. when the browser process doesn't send SendComplete notification > back to the renderer process. And if that's right, then the last sentence of > this comment doesn't make much sense. > Suggest rewording "Used to detect when browser doesn't send SendComplete message > for some packets." Done. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:455: // Since OnSendComplete happens on the same thread, there is no chance that On 2014/12/17 20:05:30, Sergey Ulanov wrote: > nit: I don't think you really need this comment. It should be obvious. Done. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:631: if (send_metrics.packet_id == 0) { On 2014/12/17 20:05:30, Sergey Ulanov wrote: > When does this happen? Why do you need this? TCP. We're not tracking tcp's packet id so this is needed. Added a comment for that. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:636: it = FindInFlightRecord(send_metrics.packet_id); On 2014/12/17 20:05:30, Sergey Ulanov wrote: > I don't think you need this because we never expect out-of-order delivery of IPC > messages. Just get the first packet and DCHECK() that packet_id is the same. Done. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:657: UMA_HISTOGRAM_BOOLEAN("WebRTC.ApplicationSocketMismatchPacketsDetected_UDP", On 2014/12/17 20:05:30, Sergey Ulanov wrote: > I don't think you need UMA metric for this. Just CHECK() here and see if we get > crash reports. Done. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... File content/renderer/p2p/socket_client.h (right): https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... content/renderer/p2p/socket_client.h:40: virtual void Send(const net::IPEndPoint& address, On 2014/12/17 20:05:30, Sergey Ulanov wrote: > This method is no longer used anywhere. Can you please remove it and rename > SendWithDscp() to Send()? Otherwise it makes no sense that SendWithDscp() > returns id and this method doesn't Done. https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... File content/renderer/p2p/socket_client_impl.cc (right): https://codereview.chromium.org/759923003/diff/280001/content/renderer/p2p/so... content/renderer/p2p/socket_client_impl.cc:97: DCHECK_EQ(state_, STATE_OPEN); On 2014/12/17 20:05:30, Sergey Ulanov wrote: > I don't think this DCHECK is valid. SendWithDscp() posts the task without > verifying the state and it may change before this task is executed. Done.
lgtm when my comments are addressed https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:120: nit: Don't need this empty line. https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:592: InFlightPacketList::iterator it = in_flight_packet_records_.begin(); nit: this can be: const InFlightPacketRecord& packet_record = in_flight_packet_records_.front(); https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:602: in_flight_packet_records_.erase(it); pop_front()? https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/so... File content/renderer/p2p/socket_client.h (right): https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/so... content/renderer/p2p/socket_client.h:41: virtual uint64_t SendWithDscp(const net::IPEndPoint& address, rename this to Send()? https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/so... File content/renderer/p2p/socket_client_impl.h (right): https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/so... content/renderer/p2p/socket_client_impl.h:43: void Send(const net::IPEndPoint& address, Remove this? It no longer overrides anything, so it shouldn't even compile.
brettw@chromium.org: Please review changes in content/common/p2p_socket_type.h wfh@chromium.org: Please review changes in content/common/p2p_messages.h https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:120: On 2014/12/23 01:11:02, Sergey Ulanov wrote: > nit: Don't need this empty line. Done. https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:592: InFlightPacketList::iterator it = in_flight_packet_records_.begin(); On 2014/12/23 01:11:02, Sergey Ulanov wrote: > nit: this can be: > const InFlightPacketRecord& packet_record = in_flight_packet_records_.front(); Done. https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:602: in_flight_packet_records_.erase(it); On 2014/12/23 01:11:02, Sergey Ulanov wrote: > pop_front()? Done.
guoweis@chromium.org changed reviewers: + sergeyu@chromium.org
all comments from sergeyu@ addressed. https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/so... File content/renderer/p2p/socket_client.h (right): https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/so... content/renderer/p2p/socket_client.h:41: virtual uint64_t SendWithDscp(const net::IPEndPoint& address, On 2014/12/23 01:11:02, Sergey Ulanov wrote: > rename this to Send()? Done. https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/so... File content/renderer/p2p/socket_client_impl.h (right): https://codereview.chromium.org/759923003/diff/360001/content/renderer/p2p/so... content/renderer/p2p/socket_client_impl.h:43: void Send(const net::IPEndPoint& address, On 2014/12/23 01:11:02, Sergey Ulanov wrote: > Remove this? It no longer overrides anything, so it shouldn't even compile. Done.
ipc content/common/p2p_messages.h lgtm
guoweis@chromium.org changed reviewers: + jln@chromium.org - brettw@chromium.org, sergeyu@chromium.org, wfh@chromium.org
jln, please owner review p2p_socket_type.h
On 2014/12/23 21:49:37, guoweis_chromium wrote: > jln, please owner review p2p_socket_type.h I'm not a correct owner for p2p_socket_type.h. My understanding is that a p2p/ owner review + a security review (here from wfh) is what you really need, so you just need a content/ owner to rubberstamp this.
On 2015/01/05 18:39:22, jln (OOO til 5th) wrote: > On 2014/12/23 21:49:37, guoweis_chromium wrote: > > jln, please owner review p2p_socket_type.h > > I'm not a correct owner for p2p_socket_type.h. My understanding is that a p2p/ > owner review + a security review (here from wfh) is what you really need, so you > just need a content/ owner to rubberstamp this. yes, you will need a content/ OWNER to rubberstamp content/common/p2p_socket_type.h
guoweis@chromium.org changed reviewers: + avi@chromium.org - jln@chromium.org
avi@, could you rubber lgtm on p2p_socket_types.h? It has been reviewed by wfh@ for security perspective.
On 2015/01/05 20:46:13, guoweis_chromium wrote: > avi@, could you rubber lgtm on p2p_socket_types.h? It has been reviewed by wfh@ > for security perspective. p2p_socket_types.h LGTM
The CQ bit was checked by guoweis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/759923003/380001
Message was sent while issue was closed.
Committed patchset #8 (id:380001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/467bab46baa301ea4d21097c3214f44fd184871a Cr-Commit-Position: refs/heads/master@{#310844} |
