Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2532873003:
Fix hitting a DCHECK in FtpNetworkTransaction on extra responses. (Closed)
DescriptionFix hitting a DCHECK in FtpNetworkTransaction on extra responses.
The DCHECK was mostly benign, though Chrome would close the socket
without waiting for a response to its QUIT messages, which has unclear
consequences.
BUG=668759
Committed: https://crrev.com/d94cd17eb70ca6099f9eeeaf307962624534884b
Cr-Commit-Position: refs/heads/master@{#434727}
Patch Set 1 #Patch Set 2 : Add bonus test #
Total comments: 4
Messages
Total messages: 19 (10 generated)
The CQ bit was checked by mmenke@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...
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
mmenke@chromium.org changed reviewers: + eroman@chromium.org
Another silly FTP fix. https://codereview.chromium.org/2532873003/diff/20001/net/ftp/ftp_network_tra... File net/ftp/ftp_network_transaction.cc (right): https://codereview.chromium.org/2532873003/diff/20001/net/ftp/ftp_network_tra... net/ftp/ftp_network_transaction.cc:422: // to have already been consumed, even when sending the QUIT command. Another option would be to skip that DCHECK in case of a quit message, which would mean we wouldn't wait for the response for the quit command, but only if we received 2 or more unexpected messages, which seems weird (We should either always wait for the quit response or never do it, when we get bonus messages, regardless of how many we get).
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2532873003/diff/20001/net/ftp/ftp_network_tra... File net/ftp/ftp_network_transaction_unittest.cc (right): https://codereview.chromium.org/2532873003/diff/20001/net/ftp/ftp_network_tra... net/ftp/ftp_network_transaction_unittest.cc:1747: TEST_P(FtpNetworkTransactionTest, ExtraQuitResponses) { This one would have passed before, since Stop() has logic to prevent multiple QUIT messages just seemed worth adding out of paranoia.
https://codereview.chromium.org/2532873003/diff/20001/net/ftp/ftp_network_tra... File net/ftp/ftp_network_transaction.cc (right): https://codereview.chromium.org/2532873003/diff/20001/net/ftp/ftp_network_tra... net/ftp/ftp_network_transaction.cc:435: rv = Stop(ERR_INVALID_RESPONSE); I am not sure I understand this code flow. rv will be OK in every case except when the sent command was COMMAND_QUIT. But maybe I am misunderstanding the role of Stop()
https://codereview.chromium.org/2532873003/diff/20001/net/ftp/ftp_network_tra... File net/ftp/ftp_network_transaction.cc (right): https://codereview.chromium.org/2532873003/diff/20001/net/ftp/ftp_network_tra... net/ftp/ftp_network_transaction.cc:435: rv = Stop(ERR_INVALID_RESPONSE); On 2016/11/28 17:51:24, eroman (slow) wrote: > I am not sure I understand this code flow. > > rv will be OK in every case except when the sent command was COMMAND_QUIT. > But maybe I am misunderstanding the role of Stop() Stop sets the next state to send the quit message, so multiple calls don't cause problems. If we've already sent a quit messages, it just returns the error synchronously, and we exit the loop, but don't DCHECK, because we don't send the quit message again.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2016/11/28 19:54:51, eroman (slow) wrote: > lgtm Thanks! Looks like we have at least 3 more FTP issues to fix (Others look more real-ish, at least - port overflows, time overflow).
The CQ bit was checked by mmenke@chromium.org
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": 20001, "attempt_start_ts": 1480363097185000,
"parent_rev": "6f6492ea5666f57e7d0c3d2b4a5c74a756b6a6b5", "commit_rev":
"3eec1b0353a7c4c6b266bd6e38383b1b534e167d"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix hitting a DCHECK in FtpNetworkTransaction on extra responses. The DCHECK was mostly benign, though Chrome would close the socket without waiting for a response to its QUIT messages, which has unclear consequences. BUG=668759 ========== to ========== Fix hitting a DCHECK in FtpNetworkTransaction on extra responses. The DCHECK was mostly benign, though Chrome would close the socket without waiting for a response to its QUIT messages, which has unclear consequences. BUG=668759 Committed: https://crrev.com/d94cd17eb70ca6099f9eeeaf307962624534884b Cr-Commit-Position: refs/heads/master@{#434727} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d94cd17eb70ca6099f9eeeaf307962624534884b Cr-Commit-Position: refs/heads/master@{#434727} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
