Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
35443002:
Update CastSocket connection flow to check for receiver credentials. (Closed)
|
Created:
7 years, 2 months ago by Munjal (Google) Modified:
7 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionUpdate CastSocket connection flow to check for receiver credentials.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230748
Patch Set 1 #Patch Set 2 : #
Total comments: 18
Patch Set 3 : #
Total comments: 16
Patch Set 4 : #
Total comments: 27
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 11
Patch Set 9 : #
Total comments: 9
Patch Set 10 : #Patch Set 11 : #
Total comments: 10
Patch Set 12 : #
Total comments: 1
Patch Set 13 : #
Total comments: 2
Patch Set 14 : #
Total comments: 1
Messages
Total messages: 31 (0 generated)
Note that I still have to add unit tests but I wanted you to start looking at the implementation in the mean time if you get time.
The SxS diffs were broken again so it was a bit hard to follow. I'm concerned about the number of type casts required to use the cert verification APIs; it's hard for me to reason about how safe they are. https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_channel.proto (right): https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_channel.proto:56: +// Protos for auth challenge and response Messages for authentication ... https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_channel.proto:60: +message AuthResponse { These messages are different from what's in the spec. https://docs.google.com/a/google.com/document/d/1FxxpHLNitzJkX7K69noI2J2Yi8JT... https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_channel.proto:67: + INTERNAL_ERROR = 0; If the error field is not set it will return the first value of INTERNAL_ERROR. It would be better to have a NONE value at the beginning. https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_channel.proto:73: +message DeviceAuthMessage { Please add some comments about which fields must be set in the challenge and which will be set in the response. https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_message_util.cc (right): https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_message_util.cc:15: +static const char kAuthNamespace[] = "com.google.cast.tp.deviceauth"; I believe there will be a prefix attached of urn:x-cast: https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_message_util.cc:102: + if (message.SerializeToString(&result)) This produces the binary form (not a human readable one). The Chrome build system generates messages that derive from protobuf::MessageLite which doesn't have the debug serialization for messages. Thus we write our own. I didn't take the time to dig into making it possible to generate full messages but it may be code size optimization for the Chrome binary. https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_message_util.h (right): https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_message_util.h:27: +std::string MessageProtoToString(const CastMessage& message_proto); Might be a good idea to revise names of mehods and arguments: CastMessageToString(cast_message_proto) AuthMessageToString(auth_message_proto) https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:53: +static const unsigned char kCAPublicKeyDER[] = { Please put all the code related to cert verfication into its own .cc, i.e. cast_auth_util.cc https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:359: + } else if (result == net::OK && is_secure_) { We should probably rename is_secure_ to requires_auth_ https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:375: + Extra newline https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:433: + der_cert.data = reinterpret_cast<unsigned char*>(const_cast<char*>( Is this really a copy? Looks like you are assigning a ptr to the string's char*. https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:433: + der_cert.data = reinterpret_cast<unsigned char*>(const_cast<char*>( It seems like you want a scoped_array<unsigned char> instead of a std::string for data, cert_data since that's what the destination structs expect. https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:434: + cert_data.c_str())); c_str() or data()? c_str() will add a NULL. https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:451: + SECKEY_ImportDERPublicKey(&trusted_ca_key_der_item, CKK_RSA)); How expensive is parsing the pubkey? Can it be done once and the result re-used? https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:468: + const_cast<char*>(signature.c_str())); Again, watch out for null terminations https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/35443002/diff/120001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:184: + bool VerifyCredentials(const std::string& certificate, Would prefer cert handling code be put into its own .cc
Initial pass. Concerned about the loop re-entrancy. Happy to suggest some improvements if it's not clear. In the ConnectJob at all examples, we sometimes use subloops for the state machines, so that we can maintain ERR_IO_PENDING invariants. For example, having ReadData as a state machine that "could" return ERR_IO_PENDING or not. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:20: #include "crypto/scoped_nss_types.h" DESIGN: Strictly speaking, while I realize extensions are not used in Android, depending on NSS directly like this is "discouraged" outside of NSS-specific or ChromeOS specific files. eg: cast_socket_nss.cc Or by implementing the auth logic in a separate helper class, and having _nss / _openssl defines. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:255: base::Bind(&CastSocket::OnChallengeEvent, AsWeakPtr())); It's unfortunate that you have to use WeakPtr here. Is there no way to avoid that - eg: by having defined lifetimes? https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:384: next_state_ = CONN_STATE_AUTH_CHALLENGE_REPLY_COMPLETE; Shouldn't you update this only after the error check - eg: line 387? You're forcing an extra pass through the loop unnecessarily, it seems. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:388: ReadData(); BUG: This may synchronously complete (line 635), which in turn go through ProcessHeader(), ProcessBody(), and ParseMessageFromBody(), which may force a re-entrance into the loop when OnChallengeEvent() is called (from line 713) https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:403: CHECK(challenge_reply_.get()); SECURITY BUG: Seems like an attacker could induce a remote crash by sending an invalid reply that failed to parse. It doesn't seem like this should be a DCHECK, but something you (gracefully) handle. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:408: LOG(ERROR) << "Wrong payload type in challenge reply"; DVLOG? There's fairly extensive use of LOG() here, which incurs size in all shipping binaries. Really think carefully about how often users will need to log this - and whether it's worth that cost to the hundreds of millions of Chrome users, or if it's just meant as a debugging aid for Chrome development. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:451: CERT_GetDefaultCertDB(), &der_cert, NULL, PR_FALSE, PR_TRUE)); You should make sure that crypto::EnsureNSSInit() is called at some point before using NSS functions. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:542: callback.Run(net::OK); BUG: You run a callback but then continue executing the body. Seems like you should be returning here immediately following the abort.
I am unable to reply to Mark's comments inline. But I think I addressed all of them. Some notes: - I changed to use data() instead of c_str() but left the casts around. You are right that we are using string as a scoped_array in this case. - Unit tests are now added https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:20: #include "crypto/scoped_nss_types.h" On 2013/10/22 20:23:22, Ryan Sleevi wrote: > DESIGN: Strictly speaking, while I realize extensions are not used in Android, > depending on NSS directly like this is "discouraged" outside of NSS-specific or > ChromeOS specific files. > > eg: cast_socket_nss.cc > > Or by implementing the auth logic in a separate helper class, and having _nss / > _openssl defines. Trying to understand this: - You are saying that I should have another .h file that includes either _nss or _openssl based on the platform? Can you point me to an example? I was just following the NetworkingPrivate code you pointed me to and it seems to include _nss files directly. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:255: base::Bind(&CastSocket::OnChallengeEvent, AsWeakPtr())); On 2013/10/22 20:23:22, Ryan Sleevi wrote: > It's unfortunate that you have to use WeakPtr here. Is there no way to avoid > that - eg: by having defined lifetimes? Right now, all code in this file uses WeakPtr. I will leave it as is and investigate if there is a way to eliminate it. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:384: next_state_ = CONN_STATE_AUTH_CHALLENGE_REPLY_COMPLETE; On 2013/10/22 20:23:22, Ryan Sleevi wrote: > Shouldn't you update this only after the error check - eg: line 387? > > You're forcing an extra pass through the loop unnecessarily, it seems. Done. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:388: ReadData(); On 2013/10/22 20:23:22, Ryan Sleevi wrote: > BUG: This may synchronously complete (line 635), which in turn go through > ProcessHeader(), ProcessBody(), and ParseMessageFromBody(), which may force a > re-entrance into the loop when OnChallengeEvent() is called (from line 713) Done. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:403: CHECK(challenge_reply_.get()); On 2013/10/22 20:23:22, Ryan Sleevi wrote: > SECURITY BUG: Seems like an attacker could induce a remote crash by sending an > invalid reply that failed to parse. > > It doesn't seem like this should be a DCHECK, but something you (gracefully) > handle. Done. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:408: LOG(ERROR) << "Wrong payload type in challenge reply"; On 2013/10/22 20:23:22, Ryan Sleevi wrote: > DVLOG? > > There's fairly extensive use of LOG() here, which incurs size in all shipping > binaries. Really think carefully about how often users will need to log this - > and whether it's worth that cost to the hundreds of millions of Chrome users, or > if it's just meant as a debugging aid for Chrome development. Done. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:451: CERT_GetDefaultCertDB(), &der_cert, NULL, PR_FALSE, PR_TRUE)); On 2013/10/22 20:23:22, Ryan Sleevi wrote: > You should make sure that crypto::EnsureNSSInit() is called at some point before > using NSS functions. Done. https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:542: callback.Run(net::OK); On 2013/10/22 20:23:22, Ryan Sleevi wrote: > BUG: You run a callback but then continue executing the body. > > Seems like you should be returning here immediately following the abort. Done.
lgtm https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:112: static bool ParseAuthMessage(const CastMessage& challenge_reply, If these are only used in this .cc I would put them in the anonymous namespace. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:145: std::string cert_data(certificate); cert_data is unused in this function. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:150: std::string(certificate).data())); Safe to assign with the content of a temporary string? This may work because the unmodified buffer will continued to be owned by |certificate| but I am not sure you should rely on it. If you want to make sure that a separate buffer is used, I would allocate scoped_array<unsigned char> and memcpy into it. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:164: trusted_ca_key_der_item.data = const_cast<unsigned char*>(kCAPublicKeyDER); This is scary. Are you certain that SECKEY_ImportDERPublicKey doesn't modify kCAPublicKeyDER? https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:184: signature_item.data = reinterpret_cast<unsigned char*>( Similar comment about assigning content of a temporary string. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_message_util.cc (right): https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_message_util.cc:16: "urn:x-cast:com.google.cast.tp.deviceauth"; wrap to previous line? https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_message_util.cc:129: message_proto->set_namespace_(kAuthNamespace); You need to set source and destination ids. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:140: CHECK(peer_cert_.empty()); CHECK(cert) https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:271: // TODO(mfoltz,munjal): Authenticate the channel if is_secure_ == true. Remove TODO https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:276: } else if (result == net::OK && is_secure_) { is_secure_ might be better named auth_required_ https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:290: extra newline
https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:112: static bool ParseAuthMessage(const CastMessage& challenge_reply, On 2013/10/23 00:33:41, mark a. foltz wrote: > If these are only used in this .cc I would put them in the anonymous namespace. Done. I put them in extensions::api::cast_channel namespace so that I don't have to prefix types with namespace in anonymous namespace. But that is not a good reason. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:145: std::string cert_data(certificate); On 2013/10/23 00:33:41, mark a. foltz wrote: > cert_data is unused in this function. Done. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:150: std::string(certificate).data())); On 2013/10/23 00:33:41, mark a. foltz wrote: > Safe to assign with the content of a temporary string? This may work because > the unmodified buffer will continued to be owned by |certificate| but I am not > sure you should rely on it. > > If you want to make sure that a separate buffer is used, I would allocate > scoped_array<unsigned char> and memcpy into it. What we have should be safe because we are not casting the buffer owned by |certificate|, we are casting buffer owned by a new string object (hence std::string(certificate)). Also it is fine to pass a pointer to an object on the stack since the calls are synchronous. That is der_cert is also local and the use of that variable is also local. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:164: trusted_ca_key_der_item.data = const_cast<unsigned char*>(kCAPublicKeyDER); On 2013/10/23 00:33:41, mark a. foltz wrote: > This is scary. Are you certain that SECKEY_ImportDERPublicKey doesn't modify > kCAPublicKeyDER? The code in NetworkingPrivate extension API does the same thing. I think the SECItem type is not well designed - it can do with a const pointer but it does not use a const data type. The alternative is to make a new copy of the cert data every time, which seems a bit unnecessary. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:184: signature_item.data = reinterpret_cast<unsigned char*>( On 2013/10/23 00:33:41, mark a. foltz wrote: > Similar comment about assigning content of a temporary string. We do std::string(signature) and then cast the buffer of the temporary string. So we do not rely on the buffer of incoming parameter. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_message_util.cc (right): https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_message_util.cc:16: "urn:x-cast:com.google.cast.tp.deviceauth"; On 2013/10/23 00:33:41, mark a. foltz wrote: > wrap to previous line? Becomes 81 cols. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_message_util.cc:129: message_proto->set_namespace_(kAuthNamespace); On 2013/10/23 00:33:41, mark a. foltz wrote: > You need to set source and destination ids. Ah I see. To what values? https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:140: CHECK(peer_cert_.empty()); On 2013/10/23 00:33:41, mark a. foltz wrote: > CHECK(cert) Done. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:271: // TODO(mfoltz,munjal): Authenticate the channel if is_secure_ == true. On 2013/10/23 00:33:41, mark a. foltz wrote: > Remove TODO Done. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:276: } else if (result == net::OK && is_secure_) { On 2013/10/23 00:33:41, mark a. foltz wrote: > is_secure_ might be better named auth_required_ Done. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:290: On 2013/10/23 00:33:41, mark a. foltz wrote: > extra newline Done.
Ryan, I will commit this but will address any of your remaining comments in a separate patch.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Ryan, turns out I need your LGTM anyway :). Please take a look as soon as you get a chance.
https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_message_util.cc (right): https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_message_util.cc:129: message_proto->set_namespace_(kAuthNamespace); On 2013/10/23 03:22:55, Munjal (Google) wrote: > On 2013/10/23 00:33:41, mark a. foltz wrote: > > You need to set source and destination ids. > > Ah I see. To what values? The destination id should be 'receiver-0'. The source id is not specified in our docs, I would fill in some value like 'sender-0' there.
https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_message_util.cc (right): https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_message_util.cc:129: message_proto->set_namespace_(kAuthNamespace); On 2013/10/23 05:27:45, mark a. foltz wrote: > On 2013/10/23 03:22:55, Munjal (Google) wrote: > > On 2013/10/23 00:33:41, mark a. foltz wrote: > > > You need to set source and destination ids. > > > > Ah I see. To what values? > > The destination id should be 'receiver-0'. The source id is not specified in > our docs, I would fill in some value like 'sender-0' there. Done.
https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:150: std::string(certificate).data())); On 2013/10/23 03:22:55, Munjal (Google) wrote: > On 2013/10/23 00:33:41, mark a. foltz wrote: > > Safe to assign with the content of a temporary string? This may work because > > the unmodified buffer will continued to be owned by |certificate| but I am not > > sure you should rely on it. > > > > If you want to make sure that a separate buffer is used, I would allocate > > scoped_array<unsigned char> and memcpy into it. > > What we have should be safe because we are not casting the buffer owned by > |certificate|, we are casting buffer owned by a new string object (hence > std::string(certificate)). > > Also it is fine to pass a pointer to an object on the stack since the calls are > synchronous. That is der_cert is also local and the use of that variable is also > local. I changed this to use a temporary variable that is declared after Mark's point (in person) that a tempoary variable is only guaranteed to be in scope for the lifetime of the expression. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:184: signature_item.data = reinterpret_cast<unsigned char*>( On 2013/10/23 03:22:55, Munjal (Google) wrote: > On 2013/10/23 00:33:41, mark a. foltz wrote: > > Similar comment about assigning content of a temporary string. > > We do std::string(signature) and then cast the buffer of the temporary string. > So we do not rely on the buffer of incoming parameter. Same as above: I changed this to use a temporary variable that is declared after Mark's point (in person) that a temporary variable is only guaranteed to be in scope for the lifetime of the expression.
Not LGTM, still seems to have the same bugs? https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:20: #include "crypto/scoped_nss_types.h" On 2013/10/22 23:42:43, Munjal (Google) wrote: > On 2013/10/22 20:23:22, Ryan Sleevi wrote: > > DESIGN: Strictly speaking, while I realize extensions are not used in Android, > > depending on NSS directly like this is "discouraged" outside of NSS-specific > or > > ChromeOS specific files. > > > > eg: cast_socket_nss.cc > > > > Or by implementing the auth logic in a separate helper class, and having _nss > / > > _openssl defines. > > Trying to understand this: > - You are saying that I should have another .h file that includes either _nss or > _openssl based on the platform? Can you point me to an example? I was just > following the NetworkingPrivate code you pointed me to and it seems to include > _nss files directly. That's because the NetworkingPrivate code is/was only ChromeOS, IIRC. If not, then we've got a bug there in terms of style. You would have cast_auth_util.h, then have cast_auth_util_nss.cc / cast_auth_util_openssl.cc You can leave the OpenSSL not-implemented for now. Both .cc would implement the same method from cast_auth_util.h, and you would use the GYP file to trigger which dependenc. https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/35443002/diff/170001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:164: trusted_ca_key_der_item.data = const_cast<unsigned char*>(kCAPublicKeyDER); On 2013/10/23 03:22:55, Munjal (Google) wrote: > On 2013/10/23 00:33:41, mark a. foltz wrote: > > This is scary. Are you certain that SECKEY_ImportDERPublicKey doesn't modify > > kCAPublicKeyDER? > > The code in NetworkingPrivate extension API does the same thing. I think the > SECItem type is not well designed - it can do with a const pointer but it does > not use a const data type. Yes, SECItem is used for both in/out and thus uses a non-const member. > > The alternative is to make a new copy of the cert data every time, which seems a > bit unnecessary. https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_message_util.cc (right): https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_message_util.cc:122: return out; I can't recall any code where I've seen this sort of pretty-printing, other than possibly sync. I just hope it's truly necessary, as it doesn't seem so. https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:141: CHECK(peer_cert_.empty()); These both seem like they should be DCHECKs. CHECK(cert) is *definitely* testing an API boundary - so DCHECK, because only a dev could screw it up. Same with the other. https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:283: return SendAuthChallenge(); BUG: You're still potentially reentering the loop here DoLoop -> DoAuthChallengeSend() -> SendAuthChallenge() -> SendMessageInternal() If SendMessageInternal hits line 351, it will run Callback, which is bound to OnChallengeEvent (line 155) OnChallengeEvent will then invoke DoLoop DoLoop -> DoAuthChallengeSend() -> SendAuthChallenge() -> SendMessageInternal() -> base::Closure::Run() -> OnChallengeEvent() -> DoLoop() https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:299: return net::ERR_FAILED; style suggestion: if (!VerifyChallengeReply()) return net::ERR_FAILED; return net::OK; https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:351: callback.Run(net::OK); is net::OK really the right error to run the callback with? Shouldn't you run with ERR_FAILED ? https://codereview.chromium.org/35443002/diff/650001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/35443002/diff/650001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:11: #include <string.h> Why string.h? What function is being used from here that I missed? https://codereview.chromium.org/35443002/diff/650001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:144: std::string certificate_copy(certificate); Don't make a copy; NSS will not modify this buffer. Just cast. https://codereview.chromium.org/35443002/diff/650001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:180: std::string signature_copy(signature); Don't make a copy https://codereview.chromium.org/35443002/diff/650001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:185: reinterpret_cast<unsigned char*>(const_cast<char*>(data.c_str())), .data()
https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/60001/chrome/browser/extensions... chrome/browser/extensions/api/cast_channel/cast_socket.cc:20: #include "crypto/scoped_nss_types.h" On 2013/10/23 19:14:04, Ryan Sleevi wrote: > On 2013/10/22 23:42:43, Munjal (Google) wrote: > > On 2013/10/22 20:23:22, Ryan Sleevi wrote: > > > DESIGN: Strictly speaking, while I realize extensions are not used in > Android, > > > depending on NSS directly like this is "discouraged" outside of NSS-specific > > or > > > ChromeOS specific files. > > > > > > eg: cast_socket_nss.cc > > > > > > Or by implementing the auth logic in a separate helper class, and having > _nss > > / > > > _openssl defines. > > > > Trying to understand this: > > - You are saying that I should have another .h file that includes either _nss > or > > _openssl based on the platform? Can you point me to an example? I was just > > following the NetworkingPrivate code you pointed me to and it seems to include > > _nss files directly. > > That's because the NetworkingPrivate code is/was only ChromeOS, IIRC. If not, > then we've got a bug there in terms of style. > > You would have cast_auth_util.h, then have cast_auth_util_nss.cc / > cast_auth_util_openssl.cc > > You can leave the OpenSSL not-implemented for now. > > Both .cc would implement the same method from cast_auth_util.h, and you would > use the GYP file to trigger which dependenc. Done. https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_message_util.cc (right): https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_message_util.cc:122: return out; On 2013/10/23 19:14:05, Ryan Sleevi wrote: > I can't recall any code where I've seen this sort of pretty-printing, other than > possibly sync. I just hope it's truly necessary, as it doesn't seem so. It is only used in logs. https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:141: CHECK(peer_cert_.empty()); On 2013/10/23 19:14:05, Ryan Sleevi wrote: > These both seem like they should be DCHECKs. > > CHECK(cert) is *definitely* testing an API boundary - so DCHECK, because only a > dev could screw it up. Same with the other. Done. https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:141: CHECK(peer_cert_.empty()); On 2013/10/23 19:14:05, Ryan Sleevi wrote: > These both seem like they should be DCHECKs. > > CHECK(cert) is *definitely* testing an API boundary - so DCHECK, because only a > dev could screw it up. Same with the other. Done. https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:283: return SendAuthChallenge(); On 2013/10/23 19:14:05, Ryan Sleevi wrote: > BUG: You're still potentially reentering the loop here > > DoLoop -> DoAuthChallengeSend() -> SendAuthChallenge() -> SendMessageInternal() > > If SendMessageInternal hits line 351, it will run Callback, which is bound to > OnChallengeEvent (line 155) > > OnChallengeEvent will then invoke DoLoop > > DoLoop -> DoAuthChallengeSend() -> SendAuthChallenge() -> SendMessageInternal() > -> base::Closure::Run() -> OnChallengeEvent() -> DoLoop() Done. Good catch. https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:299: return net::ERR_FAILED; On 2013/10/23 19:14:05, Ryan Sleevi wrote: > style suggestion: > > if (!VerifyChallengeReply()) > return net::ERR_FAILED; > return net::OK; Done. https://codereview.chromium.org/35443002/diff/410001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:351: callback.Run(net::OK); On 2013/10/23 19:14:05, Ryan Sleevi wrote: > is net::OK really the right error to run the callback with? Shouldn't you run > with ERR_FAILED ? Done. https://codereview.chromium.org/35443002/diff/650001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_auth_util.cc (right): https://codereview.chromium.org/35443002/diff/650001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:11: #include <string.h> On 2013/10/23 19:14:05, Ryan Sleevi wrote: > Why string.h? What function is being used from here that I missed? Done. Meant #include <string> https://codereview.chromium.org/35443002/diff/650001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:144: std::string certificate_copy(certificate); On 2013/10/23 19:14:05, Ryan Sleevi wrote: > Don't make a copy; NSS will not modify this buffer. Just cast. Done. https://codereview.chromium.org/35443002/diff/650001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:144: std::string certificate_copy(certificate); On 2013/10/23 19:14:05, Ryan Sleevi wrote: > Don't make a copy; NSS will not modify this buffer. Just cast. Done. https://codereview.chromium.org/35443002/diff/650001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:180: std::string signature_copy(signature); On 2013/10/23 19:14:05, Ryan Sleevi wrote: > Don't make a copy Done. https://codereview.chromium.org/35443002/diff/650001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util.cc:185: reinterpret_cast<unsigned char*>(const_cast<char*>(data.c_str())), On 2013/10/23 19:14:05, Ryan Sleevi wrote: > .data() Done.
https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:17: #include "net/cert/x509_certificate.h" Why do you need to bring this in? You don't seem to be using any of these types? https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:343: if (result == net::ERR_FAILED || result == net::OK) BUG: You fail to handle a number of cases here. SendMessageInternal() -> WriteData() -> socket_->Write() -> net::Error results Shouldn't this just be if (result != net::ERR_IO_PENDING && result != net::OK) { CloseWithError(...) callback.Run(...) ? } https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:344: { style: brace goes on line above https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:356: return WriteData(); BUG: Potentially bad recursion design SendMessage -> SendMessageInternal -> WriteData() -> OnWriteData() WriteData() -> OnWriteData() -> WriteData() -> OnWriteData(), etc This is why I suggested in my first review that you consider a state machine here. https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:406: request.callback.Run(result); DESIGN: If this callback wants to delete the object, it cannot (safely) do so, because the next line will cause an out-of-memory error. This is not generally preferable, and not stated as part of the contract/lifetime of the CastSocket. https://codereview.chromium.org/35443002/diff/740001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/740001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:389: return result >= 0 ? net::OK : result; It does not mean the full message was written, however. Your return values for WriteData() are unclear, especially when coupled with SendMessage. If WriteData() is just supposed to signal a message was successfully queued for delivery, why bother with line 380 at all? Especially when it has no relation to the message sent in SendMessageInternal
https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:17: #include "net/cert/x509_certificate.h" On 2013/10/23 23:28:51, Ryan Sleevi wrote: > Why do you need to bring this in? You don't seem to be using any of these types? Done. https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:343: if (result == net::ERR_FAILED || result == net::OK) On 2013/10/23 23:28:51, Ryan Sleevi wrote: > BUG: You fail to handle a number of cases here. > > SendMessageInternal() -> WriteData() -> socket_->Write() -> net::Error results > > Shouldn't this just be > > if (result != net::ERR_IO_PENDING && result != net::OK) { > CloseWithError(...) > callback.Run(...) ? > } Done. https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:344: { On 2013/10/23 23:28:51, Ryan Sleevi wrote: > style: brace goes on line above Done. https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:356: return WriteData(); On 2013/10/23 23:28:51, Ryan Sleevi wrote: > BUG: Potentially bad recursion design > > SendMessage -> SendMessageInternal -> WriteData() -> OnWriteData() WriteData() > -> OnWriteData() -> WriteData() -> OnWriteData(), etc > > This is why I suggested in my first review that you consider a state machine > here. The recursive nature of Write was already there in this class. I will address it outside of this patch. https://codereview.chromium.org/35443002/diff/710001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:406: request.callback.Run(result); On 2013/10/23 23:28:51, Ryan Sleevi wrote: > DESIGN: If this callback wants to delete the object, it cannot (safely) do so, > because the next line will cause an out-of-memory error. > > This is not generally preferable, and not stated as part of the > contract/lifetime of the CastSocket. Right. But let me address it separately since it was already there.
Munjal, Mark: As noted, I'm still very concerned about this code and its interactions not quite being up to Chromium's patterns. Consider this a provisional LGTM. I think there's more cleanup work, such as what we've identified with the tests, or the recursion with WriteData(), but the *crypto* side of this looks fine. I'm going to be OOO for a few days, so I'd like to just akalin@chromium or eroman@chromium as two net/ OWNERS that can offer design feedback and guidance. I'd strongly recommend akalin@ first, due to his familiarity with the SPDY state machine, which faced many of the same challenges you did. I realize this isn't net/ code, but it's heavily based on it, and it would be good for reviewing and bug fixing to be able to follow those patterns :) You'll still need a chrome/browser/extensions/ OWNER, I believe. https://codereview.chromium.org/35443002/diff/720010/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/720010/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:353: if (result != net::ERR_IO_PENDING && result != net::OK) { BUG: Your line 356 will never be hit, because result >= 0 will always match this condition
On 2013/10/24 00:14:48, Ryan Sleevi wrote: > Munjal, Mark: As noted, I'm still very concerned about this code and its > interactions not quite being up to Chromium's patterns. > > Consider this a provisional LGTM. I think there's more cleanup work, such as > what we've identified with the tests, or the recursion with WriteData(), but the > *crypto* side of this looks fine. Ryan, I would appreciate it if you would please be explicit about the items that you feel need to be looked at. So far I have: - Choice of unit test framework - Recursion in WriteData() Thanks, Mark
Hi Mark, Those are the two issues that come to mind, and make it harder to evaluate for more subtle issues, such as the timing and order of callback execution and the safety of when it is "safe" to delete the CastSocket. The WriteData() is, I believe, the most pressing issue, because I believe it hides the other issues. The choice of testing framework equally makes it harder to test for these issues. I've spoken with Fred Akalin, who has agreed to take on the reviews while I'm OOO, and can provide the design guidance on how we avoid these issues in net/, since they're very similar to the issues/challenges that socket code like this faces. On Wed, Oct 23, 2013 at 5:37 PM, <mfoltz@chromium.org> wrote: > On 2013/10/24 00:14:48, Ryan Sleevi wrote: > >> Munjal, Mark: As noted, I'm still very concerned about this code and its >> interactions not quite being up to Chromium's patterns. >> > > Consider this a provisional LGTM. I think there's more cleanup work, such >> as >> what we've identified with the tests, or the recursion with WriteData(), >> but >> > the > >> *crypto* side of this looks fine. >> > > Ryan, I would appreciate it if you would please be explicit about the > items that > you feel need to be looked at. So far I have: > - Choice of unit test framework > - Recursion in WriteData() > > Thanks, > Mark > > > https://codereview.chromium.**org/35443002/<https://codereview.chromium.org/3... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Additionally, to make sure it's clear: I think we can and should land this authentication fix now. As Munjal notes, the WriteData() is pre-existing, and simply exacerbated by the authentication flow. Separate CLs for both WriteData() and unit tests are heartily encouraged. On Wed, Oct 23, 2013 at 5:54 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > Hi Mark, > > Those are the two issues that come to mind, and make it harder to evaluate > for more subtle issues, such as the timing and order of callback execution > and the safety of when it is "safe" to delete the CastSocket. The > WriteData() is, I believe, the most pressing issue, because I believe it > hides the other issues. The choice of testing framework equally makes it > harder to test for these issues. > > I've spoken with Fred Akalin, who has agreed to take on the reviews while > I'm OOO, and can provide the design guidance on how we avoid these issues > in net/, since they're very similar to the issues/challenges that socket > code like this faces. > > > On Wed, Oct 23, 2013 at 5:37 PM, <mfoltz@chromium.org> wrote: > >> On 2013/10/24 00:14:48, Ryan Sleevi wrote: >> >>> Munjal, Mark: As noted, I'm still very concerned about this code and its >>> interactions not quite being up to Chromium's patterns. >>> >> >> Consider this a provisional LGTM. I think there's more cleanup work, >>> such as >>> what we've identified with the tests, or the recursion with WriteData(), >>> but >>> >> the >> >>> *crypto* side of this looks fine. >>> >> >> Ryan, I would appreciate it if you would please be explicit about the >> items that >> you feel need to be looked at. So far I have: >> - Choice of unit test framework >> - Recursion in WriteData() >> >> Thanks, >> Mark >> >> >> https://codereview.chromium.**org/35443002/<https://codereview.chromium.org/3... >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ryan, thanks for taking another look before you leave. I will own the code and make the necessary changes. I know of the following things you want addressed: - Remove recursion of WriteData - Tests should use pre-existing mocks in socket_test_utils instead of Gmock - Find if we can remove the use of weak pointers
https://codereview.chromium.org/35443002/diff/720010/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/35443002/diff/720010/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:353: if (result != net::ERR_IO_PENDING && result != net::OK) { On 2013/10/24 00:14:48, Ryan Sleevi wrote: > BUG: Your line 356 will never be hit, because result >= 0 will always match this > condition Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/munjal@chromium.org/35443002/790001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/munjal@chromium.org/35443002/790001
Message was sent while issue was closed.
Change committed as 230748
Message was sent while issue was closed.
Message was sent while issue was closed.
On 2013/10/24 02:43:44, Munjal (Google) wrote: > Ryan, thanks for taking another look before you leave. I will own the code and > make the necessary changes. I know of the following things you want addressed: > - Remove recursion of WriteData > - Tests should use pre-existing mocks in socket_test_utils instead of Gmock > - Find if we can remove the use of weak pointers
Message was sent while issue was closed.
On 2013/10/24 02:43:44, Munjal (Google) wrote:
> Ryan, thanks for taking another look before you leave. I will own the code and
> make the necessary changes. I know of the following things you want addressed:
> - Remove recursion of WriteData
> - Tests should use pre-existing mocks in socket_test_utils instead of Gmock
> - Find if we can remove the use of weak pointers
(sigh, Reitveld ate my reply)
Yes, please send code reviews for those three items to me.
For the first one, you may want to look at spdy_session.{h,cc} and emulate the
write loop there.
For the second one, I suggest removing all use of Gmocks entirely in favor of
existing fake classes and making new fakes as appropriate, since they're used
incorrectly anyway. (You're not supposed to interleave EXPECT_CALL statements
with interacting with the mock object.)
As for the weak pointers, I don't think Ryan means to remove weak ptrs entirely,
but simply to remove the inheritance from SupportsWeakPtr in favor of a private
WeakPtrFactory.
Message was sent while issue was closed.
On 2013/10/25 01:17:02, akalin1 wrote:
> On 2013/10/24 02:43:44, Munjal (Google) wrote:
> > Ryan, thanks for taking another look before you leave. I will own the code
and
> > make the necessary changes. I know of the following things you want
addressed:
> > - Remove recursion of WriteData
> > - Tests should use pre-existing mocks in socket_test_utils instead of Gmock
> > - Find if we can remove the use of weak pointers
>
> (sigh, Reitveld ate my reply)
>
> Yes, please send code reviews for those three items to me.
>
> For the first one, you may want to look at spdy_session.{h,cc} and emulate the
> write loop there.
>
> For the second one, I suggest removing all use of Gmocks entirely in favor of
> existing fake classes and making new fakes as appropriate, since they're used
> incorrectly anyway. (You're not supposed to interleave EXPECT_CALL statements
> with interacting with the mock object.)
>
> As for the weak pointers, I don't think Ryan means to remove weak ptrs
entirely,
> but simply to remove the inheritance from SupportsWeakPtr in favor of a
private
> WeakPtrFactory.
Actually, I was suggesting that removing WeakPtrs entirely would be a better
design.
Any time you see WeakPtr or RefCounted, it's generally a sign of "I'm not sure
how long this object will live" - which is to say, the lifetime of the object is
under-specified.
That's not to say there isn't a right time to use them, but from my examination
of the class and how it was used, it *seems* that, similar to net/ classes, the
use of WeakPtr could be avoided entirely. This has the benefit of having
strongly defined semantics for any consumers of this class.
I think the refactoring akin to Spdy's WriteLoop will make this much easier.
Message was sent while issue was closed.
https://codereview.chromium.org/35443002/diff/790001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_channel.proto (right): https://codereview.chromium.org/35443002/diff/790001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_channel.proto:61: required bytes signature = 1; another problem -- required fields in protobufs are almost always a bad idea |
