Cleanup of OpenSSL/NSS implementation of ProofVerfifier release.

Cleanup of OpenSSL/NSS implementation of ProofVerfifier release.

Implemented comments from wtc in CL https://chromiumcodereview.appspot.com/17385010/#ps170001 (Patch Set 12).

R=wtc@chromium.org

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

[ramant (doing other things), 2013-07-03 18:19:26]

Hi Wan-Teh,
  Implemented most of the changes you have suggested (except  DCHECK for |in| ==
NULL). Could you please take a look when you have a chance. thanks much.

[wtc, 2013-07-03 19:06:58]

Patch set 1 LGTM. Thanks.

https://codereview.chromium.org/18033005/diff/1/net/quic/crypto/crypto_handsh...
File net/quic/crypto/crypto_handshake.h (right):

https://codereview.chromium.org/18033005/diff/1/net/quic/crypto/crypto_handsh...
net/quic/crypto/crypto_handshake.h:271: // If the proof has changed then it
needs to be revalidated. Helper function

"the proof has changed" is just one reason we need to revalidate the proof.
Another reason is that the server config has changed. Of course, a new server
config comes with a new proof, so "the proof has changed" is strictly speaking
still correct, but a little confusing.

I suggest "the server config or the proof has changed".

https://codereview.chromium.org/18033005/diff/1/net/quic/crypto/crypto_handsh...
net/quic/crypto/crypto_handshake.h:272: // to keep server_config_valid_and
generation_counter_ in sync.

Nit: server_config_valid_and => server_config_valid_ and

(Needs a space before "and".)

https://codereview.chromium.org/18033005/diff/1/net/quic/crypto/crypto_handsh...
net/quic/crypto/crypto_handshake.h:296: // server_config_valid_ to false.

Nit: it looks a little weird that the comment block style changes. But I guess
it is more convenient to have the full line length available to this comment.

https://codereview.chromium.org/18033005/diff/1/net/quic/crypto/proof_verifie...
File net/quic/crypto/proof_verifier_chromium.cc (left):

https://codereview.chromium.org/18033005/diff/1/net/quic/crypto/proof_verifie...
net/quic/crypto/proof_verifier_chromium.cc:46: cert_verify_result_.Reset();

Thank you for making this change.  I confirmed that this code was copied
from SSLClientSocketNSS::Disconnect(), where it is necessary to do this.

https://codereview.chromium.org/18033005/diff/1/net/quic/crypto/proof_verifie...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/18033005/diff/1/net/quic/crypto/proof_verifie...
net/quic/crypto/proof_verifier_chromium.cc:74: cert_verify_result_.Reset();

This line should be removed, too.

https://codereview.chromium.org/18033005/diff/1/net/quic/quic_crypto_client_s...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/18033005/diff/1/net/quic/quic_crypto_client_s...
net/quic/quic_crypto_client_stream.h:63: // verification is performed
asynchrnously.

Typo: asynchrnously => asynchronously

[ramant (doing other things), 2013-07-03 20:31:35]

https://chromiumcodereview.appspot.com/18033005/diff/1/net/quic/crypto/crypto...
File net/quic/crypto/crypto_handshake.h (right):

https://chromiumcodereview.appspot.com/18033005/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.h:271: // If the proof has changed then it
needs to be revalidated. Helper function
On 2013/07/03 19:06:58, wtc wrote:
> 
> "the proof has changed" is just one reason we need to revalidate the proof.
> Another reason is that the server config has changed. Of course, a new server
> config comes with a new proof, so "the proof has changed" is strictly speaking
> still correct, but a little confusing.
> 
> I suggest "the server config or the proof has changed".

Done.

https://chromiumcodereview.appspot.com/18033005/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.h:272: // to keep server_config_valid_and
generation_counter_ in sync.
On 2013/07/03 19:06:58, wtc wrote:
> 
> Nit: server_config_valid_and => server_config_valid_ and
> 
> (Needs a space before "and".)

Done.

https://chromiumcodereview.appspot.com/18033005/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.h:296: // server_config_valid_ to false.
On 2013/07/03 19:06:58, wtc wrote:
> 
> Nit: it looks a little weird that the comment block style changes. But I guess
> it is more convenient to have the full line length available to this comment.

The above was the reason. Comment was too big.

https://chromiumcodereview.appspot.com/18033005/diff/1/net/quic/crypto/proof_...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://chromiumcodereview.appspot.com/18033005/diff/1/net/quic/crypto/proof_...
net/quic/crypto/proof_verifier_chromium.cc:74: cert_verify_result_.Reset();
On 2013/07/03 19:06:58, wtc wrote:
> 
> This line should be removed, too.

Done.

https://chromiumcodereview.appspot.com/18033005/diff/1/net/quic/quic_crypto_c...
File net/quic/quic_crypto_client_stream.h (right):

https://chromiumcodereview.appspot.com/18033005/diff/1/net/quic/quic_crypto_c...
net/quic/quic_crypto_client_stream.h:63: // verification is performed
asynchrnously.
On 2013/07/03 19:06:58, wtc wrote:
> 
> Typo: asynchrnously => asynchronously

Done.

[wtc, 2013-07-03 20:48:58]

Patch set 2 LGTM. Thanks.

[commit-bot: I haz the power, 2013-07-03 20:50:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/18033005/7001

[commit-bot: I haz the power, 2013-07-03 22:23:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-03 22:51:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/18033005/7001

[commit-bot: I haz the power, 2013-07-04 01:36:06]

Change committed as 210095