Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11366155:
SSLClientSocket::IsConnected should care for internal buffers (Closed)
|
Created:
8 years, 1 month ago by Takashi Toyoshima Modified:
7 years, 11 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionSSLClientSocket::IsConnected should care for internal buffers
SSLClientSocket::IsConnected() and SSLClientSocket::IsConnectedAndIdle()
may return false though it has buffered data. They should care for internally
processing buffer.
There are various implementation, for NSS, OpenSSL, and platform dependent
system libraries. This CL fix the issue on NSS. Fix for others will follow.
BUG=160033
TEST=browser_tests, net_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178775
Patch Set 1 #Patch Set 2 : plan 2 #Patch Set 3 : same change on IsConnectedAndIdle #Patch Set 4 : same change on nss #
Total comments: 14
Patch Set 5 : fix #Patch Set 6 : add unit test #
Total comments: 25
Patch Set 7 : rebase only #Patch Set 8 : reflect #9 #Patch Set 9 : reflects #10 comments on test #Patch Set 10 : Ryan's alternative fix #Patch Set 11 : rebase to new rev. #
Total comments: 10
Patch Set 12 : fix closure handling #Patch Set 13 : Split _win fix to another CL #
Total comments: 29
Patch Set 14 : reflect comments except for CloseNotify handling #Patch Set 15 : rebase #Patch Set 16 : handle logical EOFs #Patch Set 17 : remove comments #
Total comments: 13
Patch Set 18 : review #30 #
Total comments: 4
Patch Set 19 : replace sync EOF check into core #Patch Set 20 : rebase (for try) #
Total comments: 20
Patch Set 21 : fix DCHECK errors #Patch Set 22 : one more pitfall #
Total comments: 10
Patch Set 23 : reflect obvious comments #Patch Set 24 : fix IsConnectedAndIdle #Patch Set 25 : clarify function assignment #
Total comments: 2
Patch Set 26 : for submit #Patch Set 27 : rebase #
Messages
Total messages: 44 (0 generated)
Hi Wan-Teh, I landed a CL on tcp_client_socket_win.cc before. http://codereview.chromium.org/8083031 We need a similar CL on ssl_client_socket_win.cc. We don't have clear definition on IsConnected() method especially on the case of closed but having unread data. Maybe right direction could be having two interface like CanRead() and CanWrite(). The previous change, we decide to return true before reading EOF. So, I follow this policy again in this Patch Set 3. Patch Set 1 also works. But it is not consistent with previous CL.
Hi Wan-Tech, Sorry, Patch Set 3 is not enough. I'll request review again once I'm ready again. Thanks
I revised my CL to apply same modification on nss because Win uses nss by default and problem seemingly happens with nss. Could you review Patch Set 4?
Review comments on patch set 4: Thank you for the CL. This is an interesting bug. High-level comments: 1. I noticed that you use different solutions for ssl_client_socket_nss and ssl_client_socket_win. I think your solution for ssl_client_socket_win is better. Can you use the same solution for ssl_client_socket_nss? I think it will require checking if nss_bufs_ contains received data. 2. Your current solution for ssl_client_socket_nss seems to miss two places where you should also set core_recv_eof_ to true. I pointed out the two places below. 3. When you and I have finished one round of code review, we should ask rsleevi to take a look at this. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... File net/socket/ssl_client_socket_nss.cc (right): https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:1242: PostOrRunCallback(FROM_HERE, base::Bind(callback, rv)); If rv is 0 here, we should also set core_recv_eof_ to true. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:2357: rv); If rv is 0 here, we should also set core_recv_eof_ to true. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:2979: bool ret = completed_handshake_ & !core_recv_eof_; Why didn't you add the following code to this function? if (ret) ret = transport_->socket()->IsConnected(); See lines 2995-2996 below. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:3068: if (!rv) Please write this as if (rv == 0) https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... File net/socket/ssl_client_socket_win.cc (right): https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_win.cc:709: if (bytes_received_ || bytes_decrypted_) I think the changes to this file are correct. However, it is not obvious. I wonder if we can come up with a better test.
I just found that in patch sets 2 and 3, you only modified ssl_client_socket_win.cc. But that file is not used by default. It is used only if Chrome is run with the --use-system-ssl command-line option. Why did you fix that file first? Are you running Chrome with --use-system-ssl?
Thank you for reviewing this. > Why did you fix that file first? Are you running Chrome with --use-system-ssl? Firstly, I just mistakenly thought that ssl_client_socket_win is used by default, but after that I noticed that ssl_client_socket_ssl is used by default. That's the reason why I canceled my first review request at comment #3. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... File net/socket/ssl_client_socket_nss.cc (right): https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:1242: PostOrRunCallback(FROM_HERE, base::Bind(callback, rv)); This function is an internal Core class and it may run in another thread. This is the reason why I add a new flag, core_recv_eof_, in its owner class SSLClientSocketNSS. I think line 3069 catches this case. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:2357: rv); ditto. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:2979: bool ret = completed_handshake_ & !core_recv_eof_; Firstly, I think calling socket()->IsConnected() is redundant. But, as you said, it should be done. I'll add. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:3068: if (!rv) On 2012/11/13 20:03:57, wtc wrote: > > Please write this as > if (rv == 0) Done. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... File net/socket/ssl_client_socket_win.cc (right): https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_win.cc:709: if (bytes_received_ || bytes_decrypted_) Hum... it depends on packet fragmentation, or virtually timing. In our case, the problem happens only when a WebSocket packet is splitted into two small SSL frames. We can reproduce this situation with the trunk version of pywebsocket. But, I think it's not enough... I have no good idea now.
+tyoshino Can you look net/data/websocket/* expecially on _wsh.py? +wtc I added browser tests for ws:// and wss://. The test using wss:// scheme can reproduce Issue 160033, one of w3c test contributed by Microsoft. Can you take another look on this CL?
On 2012/12/06 12:28:44, Takashi Toyoshima (chromium) wrote: > > I added browser tests for ws:// and wss://. > The test using wss:// scheme can reproduce Issue 160033, one of w3c test > contributed by Microsoft. > Can you take another look on this CL? Yes, I will review this CL again. Thanks.
LGTM https://codereview.chromium.org/11366155/diff/19001/net/data/websocket/README File net/data/websocket/README (right): https://codereview.chromium.org/11366155/diff/19001/net/data/websocket/README... net/data/websocket/README:11: change title to "PASS" to notify content::TitleWatcher. changes the https://codereview.chromium.org/11366155/diff/19001/net/data/websocket/close-... File net/data/websocket/close-with-split-packet_wsh.py (right): https://codereview.chromium.org/11366155/diff/19001/net/data/websocket/close-... net/data/websocket/close-with-split-packet_wsh.py:26: code = struct.pack('!H', int(1000)) int() is unnecessary https://codereview.chromium.org/11366155/diff/19001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/19001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2874: bool ret = completed_handshake_ & !core_recv_eof_; why &? https://codereview.chromium.org/11366155/diff/19001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2891: bool ret = completed_handshake_ & !core_recv_eof_; ditto https://codereview.chromium.org/11366155/diff/19001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.h (right): https://codereview.chromium.org/11366155/diff/19001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.h:174: // True if |core_| detects EOF. has detected https://codereview.chromium.org/11366155/diff/19001/net/socket_stream/socket_... File net/socket_stream/socket_stream.cc (right): https://codereview.chromium.org/11366155/diff/19001/net/socket_stream/socket_... net/socket_stream/socket_stream.cc:1070: //if (!socket_.get() || !socket_->IsConnected()) { To be removed before submission?
Review comments on patch set 6: rsleevi: It's now time to have you involved in the code review. I'm sorry my comments may be hard to understand. I'll be happy to explain the problem to you. toyoshim: Thank you for updating the patch and adding unit tests! I reviewed the CL carefully because I am not sure if I fully understand the problem. I didn't review ssl_client_socket_win.cc because that file isn't used by default. I will review that when I am happy with the fix for ssl_client_socket_nss.cc. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... File net/socket/ssl_client_socket_nss.cc (right): https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:1242: PostOrRunCallback(FROM_HERE, base::Bind(callback, rv)); On 2012/11/15 11:10:24, Takashi Toyoshima (chromium) wrote: > > I think line 3069 catches this case. In this case, Core::Read() returns ERR_IO_PENDING. But line 3069 checks if |rv| is 0. So it won't catch this case. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:2357: rv); On 2012/11/15 11:10:24, Takashi Toyoshima (chromium) wrote: > ditto. Core::DoReadCallback() is not called when Core::Read() returns 0. So line 3069 won't catch this case, either. https://chromiumcodereview.appspot.com/11366155/diff/19001/chrome/browser/net... File chrome/browser/net/websocket_browsertest.cc (right): https://chromiumcodereview.appspot.com/11366155/diff/19001/chrome/browser/net... chrome/browser/net/websocket_browsertest.cc:39: // frames. splitted => split ("split" is an irregular verb.) In TCP, these are called "packets" or "segments" (I don't know which one), not "frames". I suggest just using "packets". Make the same changes to the comment on lines 62-63. https://chromiumcodereview.appspot.com/11366155/diff/19001/chrome/browser/net... chrome/browser/net/websocket_browsertest.cc:63: // or SSL frames. In SSL, these are called "records". But informally many people call them "packets". https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket/ssl_cli... File net/socket/ssl_client_socket_nss.cc (right): https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket/ssl_cli... net/socket/ssl_client_socket_nss.cc:1113: user_read_callback_ = callback; Note that there are three cases. This is case 1, and this will be handled by the code I suggested in Core::DoReadCallback(). Case 2 is lines 1119-1120. This still needs to be handled. Case 3 is line 1124. It is handled by your new code on lines 2965-2966 in SSLClientSocketNSS::Read(). https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket/ssl_cli... net/socket/ssl_client_socket_nss.cc:2234: rv); Core::Read() may be completed asynchronously and report the read result |rv| using user_read_callback_ here. If |rv| is 0, we should also set core_recv_eof_ to true, in a thread-safe way. https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket/ssl_cli... net/socket/ssl_client_socket_nss.cc:2621: memcpy(buf, read_buffer->data(), result); IMPORTANT: here we copy data received from transport_socket() to the nss_bufs_ memio buffer. Then we call OnRecvComplete, which will only process the data in nss_bufs_ if there is a pending Read(). So, if there is unprocessed received data in nss_bufs_, SSLClientSocketNSS::IsConnected() also needs to return true without calling transport_socket()->IsConnected(). https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket/ssl_cli... net/socket/ssl_client_socket_nss.cc:2868: // message from the server, and return false in that case. We're not doing Note this comment. I believe your CL is trying to fix this false positive in one case: the server has sent the close_notify alert, we have processed it (so core_recv_eof_ is true), but the server has not closed the TCP connection (so transport_->socket()->IsConnected() will return true). https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket/ssl_cli... net/socket/ssl_client_socket_nss.cc:2876: ret = transport_->socket()->IsConnected(); 1. Please define |ret| using a single expression: bool ret = completed_handshake_ && !core_recv_eof_ && transport_->socket()->IsConnected(); 2. Make the same change to IsConnectedAndIdle(). IMPORTANT: I think this function is the root of the problem. I believe that whenever there is a Read() that is still in progress, IsConnected() must return true without calling transport_->socket()->IsConnected(). The reason is that there may be received but not yet processed data in flight. Unfortunately I don't see a way to detect whether there is a pending Read(). Only the Core object knows that, indicated by a non-null Core::user_read_buf_ member. It seems that we need to add a 'bool waiting_read_' member to SSLClientSocketNSS. Then we can use a fix similar to your fix to TCPClientSocketWin::IsConnected. By the way, do you know why TCPClientSocketLibevent::IsConnected doesn't need this fix? Is it because TCPClientSocketLibevent does non-blocking socket I/O as opposed to async socket I/O? I'm sorry if I have asked this question before. https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket/ssl_cli... File net/socket/ssl_client_socket_nss.h (right): https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket/ssl_cli... net/socket/ssl_client_socket_nss.h:175: bool core_recv_eof_; I suggest we name this member ssl_recv_eof_, to contrast with the transport_recv_eof_ member. This member means the SSL read (PR_Read on the NSS SSL socket) returns 0. In contrast, transport_recv_eof_ means transport_socket_->Read() returns 0 (maybe asynchronously).
https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket/ssl_cli... File net/socket/ssl_client_socket_nss.cc (right): https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket/ssl_cli... net/socket/ssl_client_socket_nss.cc:2876: ret = transport_->socket()->IsConnected(); I discussed with Wan-Teh and would propose an alternative solution here. With SSL sockets, we have two buffers at play - we have the underlying transport socket's buffers (which may be operated by the kernel) and we have the NSS socket's buffers (reflected in the NSS MemIO, which is owned by the ::Core and can only be accessed on the NSS task runner) The current IsConnected/IsConnectedAndIdle TCP implementation uses MSG_PEEK, which allows it to determine if the OS buffers contain any data. If they're empty / socket is disconnected, it returns true. There is a legitimate bug here, in that we should be considering the state of the NSS internal buffers before communicating with the underlying socket. SSL::IsConnected() { if (!completed_handshake_) return false; if (waiting_read_ || waiting_write) return true; // An async read/write is still pending. if (ssl_buffers_have_data()) return true; // It may still be possible to read data // if (received_close_notify_) // return true; return transport_->socket()->IsConnected(); } SSL::IsConnectedAndIdle() { if (!completed_handshake_) return false; if (waiting_read_ || waiting_write_) return true; // This is probably wrong, but mirrors TCP if (ssl_buffers_have_data()) return false; // If there is data buffered, we are NOT idle // if (received_close_notify_) // return false; return transport_->socket()->IsConnectedAndIdle(); } The trick here is how to implement "ssl_buffers_have_data()" call, since as I mentioned, memio lives on the NSS task runner, not the socket's task runner. The NSS memio only receives data in response to a Read() or Write() call [or technically a Connect() call]. In order to satisfy a Read/Write call, the Core will post back to the Network Task Runner (via PostOrRunCallback) to run the Read callback. The way to do this is to have ::Core::DoReadCallback/::Core::DoWriteCallback/::Core::DoConnectCallback curry the state of the NSS memio buffers back to the Network Task Runner, and then have the Network Task Runner execute the actual callback Instead of base::Closure c = base::Bind( base::ResetAndReturn(&some_callback_here_), rv); PostOrRunCallback(FROM_HERE, c); It would be DCHECK(OnNSSTaskRunner()); int amount_in_read_buffer = ... // This will invoke the user callback with |rv| as result base::Closure user_cb = base::Bind( base::ResetAndReturn(&...), rv); // This is used to curry the amount_in_read_buffer and |post_or_run_cb| back to the network task runner base::Closure task = base::Bind( &Core::UpdateBufferState, this, amount_in_read_buffer, user_cb); PostOrRunCallback(FROM_HERE, task); void ...::Core::UpdateBufferState(int pending_data, const base::Closure& task) { DCHECK(OnNetworkTaskRunner()); available_read_data_ = pending_data; task.Run(); } https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket_stream/... File net/socket_stream/socket_stream.cc (left): https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket_stream/... net/socket_stream/socket_stream.cc:1070: if (!socket_.get() || !socket_->IsConnected()) { I have to agree with Wan-Teh: I think this function is the root of the bug. You should not check IsConnected() - you should attempt to read or write and let the EOF signalling from those methods indicate success or failure. Could you explain more about this?
I resume this CL. Firstly, I reflect tyoshino's comments though I'll change direction as Wan-Teh and Ryan suggested. https://codereview.chromium.org/11366155/diff/19001/net/data/websocket/README File net/data/websocket/README (right): https://codereview.chromium.org/11366155/diff/19001/net/data/websocket/README... net/data/websocket/README:11: change title to "PASS" to notify content::TitleWatcher. On 2012/12/14 15:13:54, tyoshino wrote: > changes the Done. https://codereview.chromium.org/11366155/diff/19001/net/data/websocket/close-... File net/data/websocket/close-with-split-packet_wsh.py (right): https://codereview.chromium.org/11366155/diff/19001/net/data/websocket/close-... net/data/websocket/close-with-split-packet_wsh.py:26: code = struct.pack('!H', int(1000)) On 2012/12/14 15:13:54, tyoshino wrote: > int() is unnecessary Done. https://codereview.chromium.org/11366155/diff/19001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/19001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2874: bool ret = completed_handshake_ & !core_recv_eof_; Oops... https://codereview.chromium.org/11366155/diff/19001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2891: bool ret = completed_handshake_ & !core_recv_eof_; On 2012/12/14 15:13:54, tyoshino wrote: > ditto Done. https://codereview.chromium.org/11366155/diff/19001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.h (right): https://codereview.chromium.org/11366155/diff/19001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.h:174: // True if |core_| detects EOF. On 2012/12/14 15:13:54, tyoshino wrote: > has detected Done. https://codereview.chromium.org/11366155/diff/19001/net/socket_stream/socket_... File net/socket_stream/socket_stream.cc (right): https://codereview.chromium.org/11366155/diff/19001/net/socket_stream/socket_... net/socket_stream/socket_stream.cc:1070: //if (!socket_.get() || !socket_->IsConnected()) { Oops, this change is not expected, but just for test. But, As Wan-Teh and Ryan said, I'll change direction.
Sorry for split responses. I'll try to do essential fix in the next patch set. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... File net/socket/ssl_client_socket_nss.cc (right): https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:1242: PostOrRunCallback(FROM_HERE, base::Bind(callback, rv)); Ah, you are right! Sorry, I missed the point this function returns ERR_IO_PENDING for rv == 0. https://chromiumcodereview.appspot.com/11366155/diff/9001/net/socket/ssl_clie... net/socket/ssl_client_socket_nss.cc:2357: rv); Agreed now. https://chromiumcodereview.appspot.com/11366155/diff/19001/chrome/browser/net... File chrome/browser/net/websocket_browsertest.cc (right): https://chromiumcodereview.appspot.com/11366155/diff/19001/chrome/browser/net... chrome/browser/net/websocket_browsertest.cc:39: // frames. Thanks! https://chromiumcodereview.appspot.com/11366155/diff/19001/chrome/browser/net... chrome/browser/net/websocket_browsertest.cc:63: // or SSL frames. Thanks again. I prefer to use "records" here. I want use these terms properly from now on :-) https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket_stream/... File net/socket_stream/socket_stream.cc (left): https://chromiumcodereview.appspot.com/11366155/diff/19001/net/socket_stream/... net/socket_stream/socket_stream.cc:1070: if (!socket_.get() || !socket_->IsConnected()) { Yes. Removing IsConnected() check is easy and right fix for this bug. I tried it as Patch Set 1 and confirmed that it fixed the problem. This code was written by ukai@ as the initial commit for SocketStream. I guess he aimed early close detection here, or just followed other implementations, e.g., ftp_network_transaction.cc. It means that without fixing IsConnected() behavior, other protocol may keep on having the same problem.
A happy new year, and sorry for lazy update on this. I implement Ryan's plan at Patch Set 10. So could you review it again? Patch Set 11 is the one to which I just apply a rebase up to the r175328. Thanks.
https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:576: // UpdateReadStatus() NACK on this. The intention of PostOrRunCallback is to force synchronous continuation in situations where there is not a dedicated SSL thread. This change introduces an extra yield for the single-threaded case, which we definitely don't want. https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2304: rv)); NACK: See above comments re: PostOrRunCallback. You should not be directly calling PostTask here, since in the single-threaded case, OnNSSTaskRunner == OnNetworkTaskRunner() == true https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2329: rv)); Also here https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2336: int rv) { If you're going to post a callback, you should post it as a base::Closure, pre-bound to rv. The existing code (DoWriteCallback/DoReadCallback) already handles the DCHECKs on |rv| https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2348: PostOrRunCallback(FROM_HERE, base::Bind(callback, rv)); At this point, you're guaranteed to be OnNetworkTaskRunner (line 2337), so there's no point in calling PostOrRunCallback. Presuming the updated type to base::Closure callback.Run()
Thank you, Ryan. Patch Set 12 fixes thread and closure problems. Could you review it again? https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:576: // UpdateReadStatus() Oh, I miss the case of single-threaded model and my understanding on PostOrRunCallback method is not clear. Please let me reconsider this change from the viewpoint of threads and follow your original suggestion carefully. https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2304: rv)); On 2013/01/07 18:59:04, Ryan Sleevi wrote: > NACK: See above comments re: PostOrRunCallback. You should not be directly > calling PostTask here, since in the single-threaded case, OnNSSTaskRunner == > OnNetworkTaskRunner() == true Done. https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2329: rv)); On 2013/01/07 18:59:04, Ryan Sleevi wrote: > Also here Done. https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2336: int rv) { On 2013/01/07 18:59:04, Ryan Sleevi wrote: > If you're going to post a callback, you should post it as a base::Closure, > pre-bound to rv. The existing code (DoWriteCallback/DoReadCallback) already > handles the DCHECKs on |rv| Done. https://codereview.chromium.org/11366155/diff/48004/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2348: PostOrRunCallback(FROM_HERE, base::Bind(callback, rv)); On 2013/01/07 18:59:04, Ryan Sleevi wrote: > At this point, you're guaranteed to be OnNetworkTaskRunner (line 2337), so > there's no point in calling PostOrRunCallback. > > Presuming the updated type to base::Closure > > callback.Run() Done.
the _nss and _win changes look good to me, but is this not also an issue for _openssl? Also, I have not reviewed the Python/WS test code. Assuming tyoshino will do that?
Thanks. Other owners on net/data/websocket have taken long leave now. tyoshino is TL of our team and originally worked on Chromium/WebSocket with ukai. Also he is responsible for pywebsocket development. I think he is good person to review them. I'll look on _openssl, and report it soon. Thanks.
Hi, Ryan. Currently I have no clear idea to have an equivalent test for openssl because my new test is in browsertest. Android port is the only port which use openssl by default and I think Android doesn't have a browsertest, right? I may confirm this fix in local build with special build flag on linux or something, but is it enough to land the fix on openssl? Anyway, I'll continue to work on this tomorrow. Thanks!
On 2013/01/08 14:43:27, Takashi Toyoshima (chromium) wrote: > Hi, Ryan. > > Currently I have no clear idea to have an equivalent test for openssl because my > new test is in browsertest. Android port is the only port which use openssl by > default and I think Android doesn't have a browsertest, right? > I may confirm this fix in local build with special build flag on linux or > something, but is it enough to land the fix on openssl? You should be able to use the linux_redux trybot, which will use OpenSSL on Linux. (GYP_DEFINES=use_openssl=1) That said, because this is a browser_test, you'd need to run locally on a Linux bot. > > Anyway, I'll continue to work on this tomorrow. > > Thanks!
Hi, Similar fix should be applied to _mac and _openssl. And I should add another test for _win and _mac with --use-system-ssl flag. So, I'll split _win and other fixes to another CLs. I add TODO comments to tests and disable it for OpenSSL. Can you permit me to land this CL firstly. I'll upload other CLs very soon.
On 2013/01/09 09:56:12, Takashi Toyoshima (chromium) wrote: > Hi, > > Similar fix should be applied to _mac and _openssl. And I should add another > test for > _win and _mac with --use-system-ssl flag. So, I'll split _win and other fixes to > another CLs. > > I add TODO comments to tests and disable it for OpenSSL. Correct. If you're not going to implement OpenSSL fix in this CL, you should make sure all tests are disabled. That said, the OpenSSL fix should come immediately after, and I'm happy to help, because it is used by a core platform (Android). > > Can you permit me to land this CL firstly. I'll upload other CLs very soon.
https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2977: else if (core_->HasUnhandledData()) BUG: I think there's a bug here. This fails to consider receiving a CloseNotify, which causes a weird situation in which Read() will return EOF, but then IsConnected() will continue returning true. It's not enough to check that there's unhandled data. You need to check that there's unhandled data and there hasn't been an exceptional return from (Read, Write) that should cause such data to be ignored. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3002: else if (core_->HasUnhandledData()) Same here. Design wise, you can probably simplify this: if (!IsConnected()) return false if (core_->HasUnhandledData()) return false; return transport_->socket()->IsConnectedAndIdle();
I split the change to three CLs, this CL, for --use-system-ssl, and for OpenSSL. Now I'm preparing a CL for OpenSSL too. Then, firstly I'll post replies on this CL. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2977: else if (core_->HasUnhandledData()) OK, I'll restore ssl_recv_eof_, then remove above comments. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3002: else if (core_->HasUnhandledData()) I tried to keep EnterFunction() and LeaveFunction() to work here. But can I remove them? If so, I can simplify it as you suggested.
https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3002: else if (core_->HasUnhandledData()) On 2013/01/10 07:46:29, Takashi Toyoshima (chromium) wrote: > I tried to keep EnterFunction() and LeaveFunction() to work here. But can I > remove them? If so, I can simplify it as you suggested. Just substitute the return for ret = ... & elseifs, like you have. Sorry for being shorthand lazy here :)
https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3002: else if (core_->HasUnhandledData()) Ah, I see. You mean that I should reuse IsConnected() function here. I agreed.
Review comments on patch set 13: Thank you for the new patch sets. I suggest some changes below. I haven't looked at this CL for three weeks, so I am not confident that I have regained full understanding of the problem and the best solution. Patch set 13 seems correct to me, but I may have missed some subtle issues. https://codereview.chromium.org/11366155/diff/60005/chrome/browser/net/websoc... File chrome/browser/net/websocket_browsertest.cc (right): https://codereview.chromium.org/11366155/diff/60005/chrome/browser/net/websoc... chrome/browser/net/websocket_browsertest.cc:92: // TODO(toyoshim): Add test for --use-system-ssl and fix issues on Win and Mac. I think you can ignore the --use-system-ssl code. https://codereview.chromium.org/11366155/diff/60005/net/data/websocket/README File net/data/websocket/README (right): https://codereview.chromium.org/11366155/diff/60005/net/data/websocket/README... net/data/websocket/README:39: packet handling. Nit: did you mean "split packet" or "split frame" here? https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:642: bool HasUnhandledData(); It may be good to stress that this is unhandled received data (as opposed to the data that we are trying to send). Perhaps the word "unhandled" already implies received data. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:822: // The information about NSS task runner status. Nit: I think "status" can be removed from this comment without losing much. Or the comment can say "The NSS task runner status". It seems important to note that these members are accessed on the network task runner. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:825: bool waiting_write_; It may be a good idea to name these members nss_waiting_read_ and nss_waiting_write_ to stress the fact that they represent the NSS task runner status. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:1130: user_read_callback_ = callback; It may be clearer to first set waiting_read_ to false (or assume it is false), and set waiting_read_ to true here. This way, waiting_read_=true matches rv=ERR_IO_PENDING. This comment also applies to the setting of waiting_write_ in SSLClientSocketNSS::Core::Write(). https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2334: void SSLClientSocketNSS::Core::UpdateNSSStatus( Document this method either here or in the declaration. I think this method could have a more precise name. I don't have a good suggestion, but even "UpdateNSSTaskRunnerStatus" would be clearer than "UpdateNSSStatus". https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2980: ret = transport_->socket()->IsConnected(); I would probably write this as a single conditional expression, or like this: if (!completed_handshake_) { ret = false; } else { ret = core_->HasPendingAsyncOperation() || core_->HasUnhandledData() || transport_->socket()->IsConnected(); } This comment also applies to the SSLClientSocketNSS::IsConnectedAndIdle() method.
Thank you, Wan and Ryan. Yeah, SSLClientSocketNSS is complicated more than expected... Anyway, I reflect your comments in the patch set 14. But, handling CloseNotify() is not implemented yet. It seemingly requires to check EOF here and there as I did in an early patch set. I'll upload patch set 15 which is just rebased version of patch set 14 because current baseline doesn't work in trybots. https://codereview.chromium.org/11366155/diff/60005/chrome/browser/net/websoc... File chrome/browser/net/websocket_browsertest.cc (right): https://codereview.chromium.org/11366155/diff/60005/chrome/browser/net/websoc... chrome/browser/net/websocket_browsertest.cc:92: // TODO(toyoshim): Add test for --use-system-ssl and fix issues on Win and Mac. This is a TODO for fixing the same bug on system libraries on Win and Mac. I revised this comment. https://codereview.chromium.org/11366155/diff/60005/net/data/websocket/README File net/data/websocket/README (right): https://codereview.chromium.org/11366155/diff/60005/net/data/websocket/README... net/data/websocket/README:39: packet handling. Oops. The name is not consistent. New test is used in ws and wss. So the right term must be packet and record. The file name close-with-split-packet-or-record_wsh.py looks redundant. But at least the name of html and _wsh.py must be consistent. I'd like to use packet for the file name here and clarify what does the packet mean in this document. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:642: bool HasUnhandledData(); On 2013/01/14 20:50:08, wtc wrote: > > It may be good to stress that this is unhandled received data > (as opposed to the data that we are trying to send). > > Perhaps the word "unhandled" already implies received data. Done. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:804: //////////////////////////////////////////////////////////////////////////// From line 802 to 804. This comment declares that following NSS task runner status can be accessed on the network task runner. See also my comments at line 822. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:822: // The information about NSS task runner status. OK, I remove the last "status". We have a comment at line 802 to 804 and I think it means that following members (including these status variables) can be accessed on the network task runner. So I omitted the comment and just placed them here. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:825: bool waiting_write_; On 2013/01/14 20:50:08, wtc wrote: > > It may be a good idea to name these members nss_waiting_read_ > and nss_waiting_write_ to stress the fact that they represent > the NSS task runner status. Done. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:1130: user_read_callback_ = callback; On 2013/01/14 20:50:08, wtc wrote: > > It may be clearer to first set waiting_read_ to false > (or assume it is false), and set waiting_read_ to true here. > This way, waiting_read_=true matches rv=ERR_IO_PENDING. > > This comment also applies to the setting of waiting_write_ > in SSLClientSocketNSS::Core::Write(). Done. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2199: if (OnNetworkTaskRunner()) { This comment is not related to this CL. At the head of this function, we use DCHECK(OnNSSTaskRunner()). So, I guess following else is not needed. Ditto on BufferRecv() function above. If so, I'll upload another CL to remove following else. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2334: void SSLClientSocketNSS::Core::UpdateNSSStatus( OK. I use "UpdateNSSTaskRunnerStatus" for now. I may change it again if I find better name. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2977: else if (core_->HasUnhandledData()) Try to fix this in the next CL. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2980: ret = transport_->socket()->IsConnected(); On 2013/01/14 20:50:08, wtc wrote: > > I would probably write this as a single conditional expression, > or like this: > if (!completed_handshake_) { > ret = false; > } else { > ret = core_->HasPendingAsyncOperation() || > core_->HasUnhandledData() || > transport_->socket()->IsConnected(); > } > > This comment also applies to the SSLClientSocketNSS::IsConnectedAndIdle() > method. Done. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2980: ret = transport_->socket()->IsConnected(); On 2013/01/14 20:50:08, wtc wrote: > > I would probably write this as a single conditional expression, > or like this: > if (!completed_handshake_) { > ret = false; > } else { > ret = core_->HasPendingAsyncOperation() || > core_->HasUnhandledData() || > transport_->socket()->IsConnected(); > } > > This comment also applies to the SSLClientSocketNSS::IsConnectedAndIdle() > method. Done. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3002: else if (core_->HasUnhandledData()) I applied Wan-Teh's suggestion after applying Ryan's idea.
OK. Patch set 16 handles CloseNotify, unexpected close by errors, and so on. I think this is the simplest way to catch various kinds of close. I post try jobs in patch set 16 and upload patch set 17 as an alternative version which doesn't have any comments on IsConnected() and IsConnectedAndIdle() because the last change will fix the point this comment described.
Review comments on patch set 17: I believe the code related to unhandled_buffer_size_ has bugs. 1. It seems that unhandled_buffer_size_ is actually the "complement" of the correct value. See my comment marked with "BUG(?)" below. 2. unhandled_buffer_size_ also needs to be updated when unhandled data is consumed. I am not sure if we are doing that correctly. In particular, if a PR_Read call consumes unhandled data but does not result in a complete SSL record that can be decrypted, I believe we are not updating unhandled_buffer_size_ in that case. See my comment about PR_Read below. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2199: if (OnNetworkTaskRunner()) { On 2013/01/15 13:46:03, Takashi Toyoshima (chromium) wrote: > > At the head of this function, we use DCHECK(OnNSSTaskRunner()). > So, I guess following else is not needed. The OnNetworkTaskRunner() test here means "if the network task runner is the same as the NSS task runner". This test should not be removed. https://codereview.chromium.org/11366155/diff/96005/chrome/browser/net/websoc... File chrome/browser/net/websocket_browsertest.cc (right): https://codereview.chromium.org/11366155/diff/96005/chrome/browser/net/websoc... chrome/browser/net/websocket_browsertest.cc:95: // libraries on Win and Mac once http://crbug.com/160033 is fixed against them. Delete this comment. We are going to remove the --use-system-ssl code. https://codereview.chromium.org/11366155/diff/96005/net/data/websocket/README File net/data/websocket/README (right): https://codereview.chromium.org/11366155/diff/96005/net/data/websocket/README... net/data/websocket/README:11: packets mean segments for WebSocket, or records for secure WebSocket. segments => TCP segments records => SSL records Make the same changes to line 41. https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2083: int rv = PR_Read(nss_fd_, user_read_buf_->data(), user_read_buf_len_); PR_Read may consume the unhandled data in the memio read buffer. So it seems that after the PR_Read call, we should also update unhandled_buffer_size_. Note: PR_Write could potentially also consume the unhandled data in the memio read buffer if a renegotiation is in progress. https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2328: int amount_in_read_buffer = memio_GetReadParams(nss_bufs_, &buf); BUG(?): I believe this is wrong. The return value of memio_GetReadParams() is the number of bytes available to write into the read buffer, i.e., the free space in the read buffer. So the amount of unhandled data should be approximately kRecvBufferSize - memio_GetReadParams(nss_bufs_, &buf) The reason I said "approximately" is that we may need to subtract 1 because the read buffer is a circular buffer. There is an internal function memio_buffer_used_contiguous(&secret->readbuf) defined in net/base/nss_memio.c that we can use to get the right value. We will need to expose it. https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3010: !core_->IsClosed() && Why do we need both ssl_is_closed_ and core_->IsClosed()? https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.h (right): https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.h:174: // True if the SSL connection is closed due to CloseNotify or errors. CloseNotify => close_notify
Thanks. Also I abandoned the CL for system libraries. https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/60005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2199: if (OnNetworkTaskRunner()) { Ah, sorry I mistakenly read this line 2199 as On*NSSTask*Runner(). You are right. Sorry for bothering. https://codereview.chromium.org/11366155/diff/96005/chrome/browser/net/websoc... File chrome/browser/net/websocket_browsertest.cc (right): https://codereview.chromium.org/11366155/diff/96005/chrome/browser/net/websoc... chrome/browser/net/websocket_browsertest.cc:95: // libraries on Win and Mac once http://crbug.com/160033 is fixed against them. Oh, I see. Done! https://codereview.chromium.org/11366155/diff/96005/net/data/websocket/README File net/data/websocket/README (right): https://codereview.chromium.org/11366155/diff/96005/net/data/websocket/README... net/data/websocket/README:11: packets mean segments for WebSocket, or records for secure WebSocket. On 2013/01/16 00:34:16, wtc wrote: > > segments => TCP segments > records => SSL records > > Make the same changes to line 41. Done. https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2083: int rv = PR_Read(nss_fd_, user_read_buf_->data(), user_read_buf_len_); I define enum Operation type and use it in UpdateNSSTaskRunnerStatus. PR_Read and PR_Write will use OPERATION_UPDATE to just update unhandled_buffer_size_. I'm not sure when a renegotiation could happen. So, I check the size before and after calling PR_Write, and notify the latest size iff the size is changed. https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2328: int amount_in_read_buffer = memio_GetReadParams(nss_bufs_, &buf); Done. I expose it as memio_GetReadableBufferSize(). https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3010: !core_->IsClosed() && ssl_is_closed_ is for synchronous completion case, core_->IsClosed() is for asynchronous completion case. Synchronous case should be handled for single thread model. But... these condition looks wrong? Now I think HasPendingAsyncOperation should be handled at higher priority than IsClosed() because pending async operation should be finished before the detected EOF.
https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3010: !core_->IsClosed() && On 2013/01/16 07:16:03, Takashi Toyoshima (chromium) wrote: > ssl_is_closed_ is for synchronous completion case, core_->IsClosed() is for > asynchronous completion case. Synchronous case should be handled for single I'm not sure I understand this. Both synchronous and asynchronous use a core_ object. Why can that not be used consistently? > thread model. > > But... these condition looks wrong? > Now I think HasPendingAsyncOperation should be handled at higher priority than > IsClosed() because pending async operation should be finished before the > detected EOF. https://codereview.chromium.org/11366155/diff/123001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/123001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2407: DCHECK(!callback.is_null()); Have you actually run this on a debug build on Linux? You're supplying a null callback on line 2151 - this line should blow up here. Seems like this should be DCHECK(operation == OPERATION_UPDATE || !callback.is_null) https://codereview.chromium.org/11366155/diff/123001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:3044: transport_->socket()->IsConnected()); indents are wrong here
https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/96005/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3010: !core_->IsClosed() && OK, I move the logic for ssl_is_closed_ into core_ object. https://codereview.chromium.org/11366155/diff/123001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/123001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2407: DCHECK(!callback.is_null()); Thanks. I didn't check it in debug build this time. Fixed. https://codereview.chromium.org/11366155/diff/123001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:3044: transport_->socket()->IsConnected()); On 2013/01/18 00:03:24, Ryan Sleevi wrote: > indents are wrong here Done.
You've still got a lot of broken threading here. Highlighted below. https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1123: nss_is_closed_ = true; BUG: Wrong thread here https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1134: DCHECK(!nss_waiting_read_); BUG: Wrong thread here https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1142: nss_waiting_read_ = true; BUG: Wrong thread here https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1153: nss_is_closed_ = true; BUG: Wrong thread here https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1172: nss_is_closed_ = true; BUG: Wrong thread here https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1183: DCHECK(!nss_waiting_write_); BUG: Wrong thread here https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1191: nss_waiting_write_ = true; BUG: Wrong thread here https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1202: nss_is_closed_ = true; BUG: Wrong thread here https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:3052: core_->IsConnected() || This logic change isn't correct. You're short-circuiting to "true" when 1) There's no pending operation 2) There's no pending operation 3) The NSS core thinks its connected (which is to say: The NSS core has not received an explicit disconnect) Compare this with your logic on 3060-3062. core_->HasPendingAsyncOperation() || (core_->IsConnected() && core_->HasUnhandledReceivedData()) || transport_->socket()->IsConnected()
Can you tell me the way to flip the threading model? I'd like to check both threading models in the running test.
On 2013/01/22 02:03:26, Takashi Toyoshima (chromium) wrote: > Can you tell me the way to flip the threading model? > I'd like to check both threading models in the running test. Test it on Linux. But hopefully the code should be self-documenting. If something DCHECK(OnNSSTaskRunner()), then you should only read or set that variable on the NSS task runner. Meaning any function not on NSS Task runner (eg: the Network Task Runner, aka any of the StreamSocket implementation). Your documentation states "Members that are only accessed on the *network* task runner", but then you go and modify them in code that has, within a few lines, done DCHECK(OnNSSTaskRunner()) - which is not the right thread. Before you introduced nss_is_closed_, you were mostly doing the right thing by having the NSS task runner marshal nss_waiting_read_, nss_waiting_write_, and unhandled_buffer_size_ to the network task runner. You have to do the same for the nss_is_closed_ https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1123: nss_is_closed_ = true; On 2013/01/21 16:33:29, Ryan Sleevi wrote: > BUG: Wrong thread here Er, this one was right https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1172: nss_is_closed_ = true; On 2013/01/21 16:33:29, Ryan Sleevi wrote: > BUG: Wrong thread here as was this one
Thank you Ryan. I fixed some threading bugs in the patch set 22. https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1134: DCHECK(!nss_waiting_read_); On 2013/01/21 16:33:29, Ryan Sleevi wrote: > BUG: Wrong thread here Done. https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1142: nss_waiting_read_ = true; On 2013/01/21 16:33:29, Ryan Sleevi wrote: > BUG: Wrong thread here Done. https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1153: nss_is_closed_ = true; On 2013/01/21 16:33:29, Ryan Sleevi wrote: > BUG: Wrong thread here Done. https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1172: nss_is_closed_ = true; On 2013/01/22 02:48:05, Ryan Sleevi wrote: > On 2013/01/21 16:33:29, Ryan Sleevi wrote: > > BUG: Wrong thread here > > as was this one Done. https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1183: DCHECK(!nss_waiting_write_); On 2013/01/21 16:33:29, Ryan Sleevi wrote: > BUG: Wrong thread here Done. https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1191: nss_waiting_write_ = true; On 2013/01/21 16:33:29, Ryan Sleevi wrote: > BUG: Wrong thread here Done. https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1202: nss_is_closed_ = true; On 2013/01/21 16:33:29, Ryan Sleevi wrote: > BUG: Wrong thread here Done. https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2411: void SSLClientSocketNSS::Core::UpdateNSSTaskRunnerStatus( I renamed it as OnNSSTaskRunnerStateUpdated() to follow other functions and moved to proper place for functions working in NetworkTaskRunner. https://codereview.chromium.org/11366155/diff/139001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:3052: core_->IsConnected() || Thank you. I was confused on how to handle transport IsConnected().
Mostly LGTM, but one question, and a few nits. I think we're in the home stretch here - thanks for persisting with this. https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:788: void OnNSSTaskRunnerStateUpdated(int amount_in_read_buffer, Naming: "OnNSSTaskRunnerStateUpdated" is perhaps a bit overkill. The "NSSTaskRunner" doesn't have "state", it's the socket that lives on the NSS task runner. OnNSSBuffersUpdated() ? https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1155: // NetworkTaskRunner because it runs in multi-threaded mode. nit: These comments are superflous Same throughout the rest of the file https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1162: // mode. |nss_waiting_read_| is not set. nit: These comments are superflous Same throughout the rest of the file https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2741: void SSLClientSocketNSS::Core::OnNSSTaskRunnerStateUpdated( I find it weird to have both this function and DidNSSRead / DidNSSWrite, and understanding the distinction between them, given that we update the NSSTaskRunnerState whenever we read or write. Should we remove the operation & bool closed from this, and have the existing functions use DidNSSRead (with extra params) and DidNSSWrite? https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:3101: !(core_->IsConnected() && core_->HasUnhandledReceivedData()) && In thinking through this logic more, perhaps it should be separate. What is the expected return if core_->HasPendingAsyncOperation? It's true that it's connected, but it's false that it's idle (because there's a pending operation)
As you said, buffer update operations and waiting flags reset operations are handled vaguely. I assign flag handling to DidNSSRead/DidNSSWrite, and buffer update operation to OnNSSBufferUpdated respectively. https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:788: void OnNSSTaskRunnerStateUpdated(int amount_in_read_buffer, Agreed. Done! https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1155: // NetworkTaskRunner because it runs in multi-threaded mode. On 2013/01/23 01:46:20, Ryan Sleevi wrote: > nit: These comments are superflous > > Same throughout the rest of the file Done. https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:1162: // mode. |nss_waiting_read_| is not set. On 2013/01/23 01:46:20, Ryan Sleevi wrote: > nit: These comments are superflous > > Same throughout the rest of the file Done. https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2133: base::Closure()); Oops, I missed to post the created task... then DCHECK could not fire in tries. https://codereview.chromium.org/11366155/diff/145005/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:3101: !(core_->IsConnected() && core_->HasUnhandledReceivedData()) && Agreed. I give up to reuse IsConnected() here.
I'm not entirely thrilled with the multiple PostOrRunCallbacks, since it's non-ideal for performance. Considering that the worst case involves three callbacks, it would be nice if we could coalesce those into a single call so that we're not constantly yielding the message loop, but I'm not sure if I feel strongly enough about that to block this CL. One way to reduce the callbacks would be to do something like UpdateHandshakeState, but that would only get it down to two callbacks, unless you were also posting an (optional) closure to continue running. So I think I'm OK with this (for now...), and will take a TODO on myself to reduce the number of calls in ::Core. Note, one BUG below that should be fixed before committing, but no need for re-review. Otherwise, LGTM. https://codereview.chromium.org/11366155/diff/139010/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/139010/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2404: base::Bind(base::ResetAndReturn(&user_read_callback_), rv)); BUG: You copy/pasted the wrong bit here. This should be posting user_cb, or should remove line 2391-2393, and be posting user_write_callback_
Sorry for leaving performance issue behind. I'll submit this CL now. https://codereview.chromium.org/11366155/diff/139010/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11366155/diff/139010/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2404: base::Bind(base::ResetAndReturn(&user_read_callback_), rv)); Oops, sorry for a silly mistake.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/11366155/140048
Presubmit check for 11366155-140048 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/net
Presubmit checks took 1.7s to calculate.
Oops. I'll use try and dcommit for now because wtc@, one of chrome/browser/net OWNER already review the part. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
