Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1820233002:
[WebSocket] Incoming close frame should not overtake data frames (Closed)
Description[WebSocket] Incoming close frame should not overtake data frames
Incoming data frames are throttled by quota maintained by both the browser side
and the renderer side websocket implementations. On the other hand, when a
CLOSE frame arrives, it is notified immediately to the renderer side. As a
result, a CLOSE frame overtakes data frames, which is bad.
BUG=596761
Committed: https://crrev.com/d2727dfa142f063944cd57e1de9900609ff2ec32
Cr-Commit-Position: refs/heads/master@{#386337}
Patch Set 1 : #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : rebase #
Messages
Total messages: 32 (15 generated)
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
yhirano@chromium.org changed reviewers: + tyoshino@chromium.org
ping
https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... net/websockets/websocket_channel.cc:532: return CHANNEL_ALIVE; CHANNEL_DELETED here and insert "return CHANNEL_ALIVE" between L530 and L531 https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... net/websockets/websocket_channel.cc:538: return CHANNEL_ALIVE; CHANNEL_DELETED? https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... net/websockets/websocket_channel.cc:973: // We have some data to be sent to the renderer befor sending this before https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... net/websockets/websocket_channel.cc:977: return RespondToClosingHandshake(); blank line https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... net/websockets/websocket_channel.h:92: // send up to |quota| units of data. Calling this function may break the line at the end of the sentence and have a blank line?
https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... net/websockets/websocket_channel.cc:532: return CHANNEL_ALIVE; On 2016/03/29 11:18:56, tyoshino wrote: > CHANNEL_DELETED here and insert "return CHANNEL_ALIVE" between L530 and L531 Done. https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... net/websockets/websocket_channel.cc:538: return CHANNEL_ALIVE; On 2016/03/29 11:18:56, tyoshino wrote: > CHANNEL_DELETED? Done. https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... net/websockets/websocket_channel.cc:973: // We have some data to be sent to the renderer befor sending this On 2016/03/29 11:18:56, tyoshino wrote: > before Done. https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... net/websockets/websocket_channel.cc:977: return RespondToClosingHandshake(); On 2016/03/29 11:18:56, tyoshino wrote: > blank line Done. https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/1820233002/diff/100001/net/websockets/websock... net/websockets/websocket_channel.h:92: // send up to |quota| units of data. Calling this function may On 2016/03/29 11:18:56, tyoshino wrote: > break the line at the end of the sentence and have a blank line? Done.
lgtm https://codereview.chromium.org/1820233002/diff/120001/net/websockets/websock... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/1820233002/diff/120001/net/websockets/websock... net/websockets/websocket_channel.cc:504: return CHANNEL_ALIVE; CHANNEL_DELETED? or just return the result of DoDropChannel?
https://codereview.chromium.org/1820233002/diff/120001/net/websockets/websock... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/1820233002/diff/120001/net/websockets/websock... net/websockets/websocket_channel.cc:504: return CHANNEL_ALIVE; On 2016/04/05 06:13:56, tyoshino wrote: > CHANNEL_DELETED? or just return the result of DoDropChannel? Thanks, you're right. Done.
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tyoshino@chromium.org Link to the patchset: https://codereview.chromium.org/1820233002/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1820233002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1820233002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
yhirano@chromium.org changed reviewers: + kinuko@chromium.org
+kinuko@ for content/ changes.
On 2016/04/06 07:46:33, yhirano wrote: > +kinuko@ for content/ changes. lgtm
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1820233002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1820233002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
yhirano@chromium.org changed reviewers: + rockot@chromium.org
+rockot@ for mojo/ changes.
On 2016/04/06 14:45:25, yhirano wrote: > +rockot@ for mojo/ changes. mojo/services/network is deleted, so I don't need an approval for the directory any more.
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tyoshino@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/1820233002/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1820233002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1820233002/180001
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [WebSocket] Incoming close frame should not overtake data frames Incoming data frames are throttled by quota maintained by both the browser side and the renderer side websocket implementations. On the other hand, when a CLOSE frame arrives, it is notified immediately to the renderer side. As a result, a CLOSE frame overtakes data frames, which is bad. BUG=596761 ========== to ========== [WebSocket] Incoming close frame should not overtake data frames Incoming data frames are throttled by quota maintained by both the browser side and the renderer side websocket implementations. On the other hand, when a CLOSE frame arrives, it is notified immediately to the renderer side. As a result, a CLOSE frame overtakes data frames, which is bad. BUG=596761 Committed: https://crrev.com/d2727dfa142f063944cd57e1de9900609ff2ec32 Cr-Commit-Position: refs/heads/master@{#386337} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d2727dfa142f063944cd57e1de9900609ff2ec32 Cr-Commit-Position: refs/heads/master@{#386337} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
