|
|
Created:
5 years, 7 months ago by Ryan Hamilton Modified:
5 years, 7 months ago Reviewers:
cbentzel CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the use of SocketDataProviders in HttpNetworkTransactionTest.GenerateAuthToken
BUG=489701
Committed: https://crrev.com/cb68dc606b60d51794dec064a5405fd3e0705310
Cr-Commit-Position: refs/heads/master@{#330898}
Patch Set 1 #Patch Set 2 : Fix comments #
Total comments: 12
Patch Set 3 : fix comments #Messages
Total messages: 13 (3 generated)
rch@chromium.org changed reviewers: + cbentzel@chromium.org
cbentzel: ping
On 2015/05/20 13:39:45, Ryan Hamilton wrote: > cbentzel: ping Will take a look now. May take a while as I page things back in.
Thanks for working on this. Just trying to increase my understanding a bit, but this overall looks nice. https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (left): https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10156: session_deps_.socket_factory->AddSocketDataProvider(&data_provider); Wow, I'm really surprised this worked. Here's what I think was happening (which you also covered in the bug, just trying to echo back). - First round, we create StaticSocketDataProvider "A", it get's used, destructor is called, all is pretty OK so far. - Second round, we create StaticSocketDataProvider "B". This just happens to be at the same address as "A". The MockSocket used to be using "A", but because "B" just happens to line up at the same memory address we use it instead. There's also an additional reference to it in MockClientSocketFactory::mock_data_, but this additional reference is never used since we don't create another socket. [note that I wonder if ~MockClientSocketFactory should check that all of mock_data_ and mock_ssl_data_ are consumed] - Subsequent rounds go through similar spells. Yeah, that's weird. https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10146: if (read_write_round.read.data == kProxyChallenge.data && This isn't immediately obvious to me what is going on here. Could you add a comment for why this is being done? In the interim, I'll try to figure out why. https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10165: std::vector<StaticSocketDataProvider*> data_providers; Would ScopedVector<StaticSocketDataProvider> work here instead - so you can save the STLDeleteElements below?
LGTM with request for comment shown below. https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10146: if (read_write_round.read.data == kProxyChallenge.data && On 2015/05/21 00:10:25, cbentzel wrote: > This isn't immediately obvious to me what is going on here. Could you add a > comment for why this is being done? In the interim, I'll try to figure out why. Ah - I'm guessing this is due to Proxy-Connection: Close and needing to create a new StaticSocketDataProvider down below to be used for the new socket. I think a comment would likely help future spelunkers here. https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10152: if (read_write_round.extra_read) { It doesn't look like there is ever a case where a round includes both kProxyChallenge and extra_read/extra_write, but I was wondering about whether this should go above the push_back statements (so it would apply to the current socket, not the new socket). Not a huge deal either way since it doesn't look like that combination happens, but wondering if ytou had considered it.
The CQ bit was checked by rch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbentzel@chromium.org Link to the patchset: https://codereview.chromium.org/1146693003/#ps40001 (title: "fix comments")
https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (left): https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10156: session_deps_.socket_factory->AddSocketDataProvider(&data_provider); On 2015/05/21 00:10:25, cbentzel wrote: > Wow, I'm really surprised this worked. > > Here's what I think was happening (which you also covered in the bug, just > trying to echo back). > > - First round, we create StaticSocketDataProvider "A", it get's used, destructor > is called, all is pretty OK so far. > > - Second round, we create StaticSocketDataProvider "B". This just happens to be > at the same address as "A". The MockSocket used to be using "A", but because "B" > just happens to line up at the same memory address we use it instead. There's > also an additional reference to it in MockClientSocketFactory::mock_data_, but > this additional reference is never used since we don't create another socket. > > [note that I wonder if ~MockClientSocketFactory should check that all of > mock_data_ and mock_ssl_data_ are consumed] > > - Subsequent rounds go through similar spells. > > Yeah, that's weird. Yeah, you got it exactly right! I don't think we can do the check in ~MockClientSocketFactory because there are actually many tests (mostly of error behavior, I think) in which we don't consume all data. https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10146: if (read_write_round.read.data == kProxyChallenge.data && On 2015/05/21 00:10:25, cbentzel wrote: > This isn't immediately obvious to me what is going on here. Could you add a > comment for why this is being done? In the interim, I'll try to figure out why. So kProxyChallenge has Proxy-Connection: close which results in the connection being closed. However, I don't understand why I need the second clause. Do you know? If the write is kConnect and the read is kProxyChallenge the connection is not closed. Is this a bug? https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10146: if (read_write_round.read.data == kProxyChallenge.data && On 2015/05/21 00:22:04, cbentzel wrote: > On 2015/05/21 00:10:25, cbentzel wrote: > > This isn't immediately obvious to me what is going on here. Could you add a > > comment for why this is being done? In the interim, I'll try to figure out > why. > > Ah - I'm guessing this is due to Proxy-Connection: Close and needing to create a > new StaticSocketDataProvider down below to be used for the new socket. I think a > comment would likely help future spelunkers here. You got it. Added a comment. (Good idea) https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10152: if (read_write_round.extra_read) { On 2015/05/21 00:22:04, cbentzel wrote: > It doesn't look like there is ever a case where a round includes both > kProxyChallenge and extra_read/extra_write, but I was wondering about whether > this should go above the push_back statements (so it would apply to the current > socket, not the new socket). > > Not a huge deal either way since it doesn't look like that combination happens, > but wondering if ytou had considered it. I *think* it's correct as is, because if there was an extra read/write it would happen after the Proxy-Connection: close response was processed which *should* mean the old socket was closed, right? Happy to revisit this if you think it best. https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10165: std::vector<StaticSocketDataProvider*> data_providers; On 2015/05/21 00:10:25, cbentzel wrote: > Would ScopedVector<StaticSocketDataProvider> work here instead - so you can save > the STLDeleteElements below? > Ooo! nice!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146693003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cb68dc606b60d51794dec064a5405fd3e0705310 Cr-Commit-Position: refs/heads/master@{#330898}
Message was sent while issue was closed.
https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10146: if (read_write_round.read.data == kProxyChallenge.data && On 2015/05/21 02:31:24, Ryan Hamilton wrote: > On 2015/05/21 00:10:25, cbentzel wrote: > > This isn't immediately obvious to me what is going on here. Could you add a > > comment for why this is being done? In the interim, I'll try to figure out > why. > > So kProxyChallenge has Proxy-Connection: close which results in the connection > being closed. However, I don't understand why I need the second clause. Do you > know? If the write is kConnect and the read is kProxyChallenge the connection is > not closed. Is this a bug? I don't know why off the top of my head. This is worth investigating. https://codereview.chromium.org/1146693003/diff/20001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:10152: if (read_write_round.extra_read) { On 2015/05/21 02:31:24, Ryan Hamilton wrote: > On 2015/05/21 00:22:04, cbentzel wrote: > > It doesn't look like there is ever a case where a round includes both > > kProxyChallenge and extra_read/extra_write, but I was wondering about whether > > this should go above the push_back statements (so it would apply to the > current > > socket, not the new socket). > > > > Not a huge deal either way since it doesn't look like that combination > happens, > > but wondering if ytou had considered it. > > I *think* it's correct as is, because if there was an extra read/write it would > happen after the Proxy-Connection: close response was processed which *should* > mean the old socket was closed, right? Happy to revisit this if you think it > best. This seems reasonable. |