DnsTransaction::RecordLostPacketsIfAny shouldn't crash if some attempts failed to connect and thus …

DnsTransaction::RecordLostPacketsIfAny shouldn't crash if some attempts failed to connect and thus have NULL socket_lease.

BUG=244507
TEST=net_unittests --gtest_filter=DnsTransactionTest.ConnectFailureFollowedBySuccess

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202967

[mef, 2013-05-29 16:23:12]

Szymon,

could you take a look? I was able to reproduce and fix the crash.

thanks,
-m

[szym, 2013-05-29 17:09:42]

lgtm

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:812: // calculated from attempt index
nit: Format like a sentence.

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction_un...
File net/dns/dns_transaction_unittest.cc (right):

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:188: if (create_failing_sockets_) {
Suggest renaming this field to |fail_next_socket_| since after creating one
socket the behavior reverts to normal.

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:870: TEST_F(DnsTransactionTest,
ConnectFailureDoesntCrashInRecordLostPacketsIfAny) {
suggest just calling the test "ConnectFailureFollowedBySuccess"

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:871: // Fallback on server failure.
suggest: "// Retry after failure."

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:881: // Expect no crash here!
unnecessary comment

[szym, 2013-05-29 17:11:30]

Also, please add to CL description:
TEST=net_unittests --gtest_filter=DnsTransactionTest.<name of the new test>

or at least mention that this CL adds new test.

(There is always a debate about the use of TEST=, but IMO it helps to show that
there is a test confirming the fix.)

[mef, 2013-05-29 17:24:04]

Done. Thanks for review!

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction.cc...
net/dns/dns_transaction.cc:812: // calculated from attempt index
On 2013/05/29 17:09:42, szym wrote:
> nit: Format like a sentence.

Done.

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction_un...
File net/dns/dns_transaction_unittest.cc (right):

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:188: if (create_failing_sockets_) {
On 2013/05/29 17:09:42, szym wrote:
> Suggest renaming this field to |fail_next_socket_| since after creating one
> socket the behavior reverts to normal.

Done.

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:870: TEST_F(DnsTransactionTest,
ConnectFailureDoesntCrashInRecordLostPacketsIfAny) {
On 2013/05/29 17:09:42, szym wrote:
> suggest just calling the test "ConnectFailureFollowedBySuccess"

Done.

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:871: // Fallback on server failure.
On 2013/05/29 17:09:42, szym wrote:
> suggest: "// Retry after failure."

Done.

https://codereview.chromium.org/15881008/diff/1003/net/dns/dns_transaction_un...
net/dns/dns_transaction_unittest.cc:881: // Expect no crash here!
On 2013/05/29 17:09:42, szym wrote:
> unnecessary comment

Done.

[commit-bot: I haz the power, 2013-05-29 18:33:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/15881008/9001

[commit-bot: I haz the power, 2013-05-29 21:06:12]

Change committed as 202967