Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 4 months ago by hbos Modified:
5 years, 3 months ago Reviewers:
torbjorng (webrtc), tommi CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRTCCertificates added to RTCConfiguration, used by WebRtcSession/-DescriptionFactory.
This CL allows you to, having generated one or more RTCCertificates, supply them to RTCConfiguration for CreatePeerConnection use. This means an SSLIdentity does not have to be generated with a DtlsIdentityStore[Interface/Impl] as part of the CreatePeerConnection steps because the certificate contains all the necessary information.
To create an RTCCertificate you have to do the identity generation yourself though. But you could reuse the same RTCCertificate for multiple connections.
BUG=webrtc:4927
R=tommi@webrtc.org, torbjorng@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/87713d0fe6fb9c86abe501bdf3d26ef4287ee617
Patch Set 1 : #
Total comments: 11
Patch Set 2 : Parameterized testing in webrtcsession_unittest. Addressed tommi's comments. #
Total comments: 13
Patch Set 3 : Addressed comments #
Total comments: 12
Patch Set 4 : Addressed comments #
Total comments: 2
Patch Set 5 : Addressed torbjorng's comment and merged with master #
Created: 5 years, 3 months ago
Messages
Total messages: 21 (7 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
hbos@webrtc.org changed reviewers: + tommi@webrtc.org, torbjorng@webrtc.org
I will update the webrtcsession unittests to test both using a store and using certificates before I require this to be reviewed, but feel free to take a look already if you like. This allows certificates to be used in RTCConfiguration but instead of passing the certificate around, its SSLIdentity is grabbed and passed on so that no surrounding code has to be updated. Switching over from SSLIdentity* to scoped_refptr<RTCCertificate> should be done in a series of future CLs.
https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:724: if (!certificate) { I don't think you need this variable actually. Here you could just do if (rtc_configuration.certificates.empty()) { https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:735: // Use the already generated certificate. is the intent to support multiple certificates eventually? https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:740: certificate, ...and here you could pass rtc_configuration.certificates[0] https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right): https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsessiondescriptionfactory.cc:187: new rtc::RefCountedObject<WebRtcIdentityRequestObserver>(); initialize in the initializer list? https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsessiondescriptionfactory.h (right): https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsessiondescriptionfactory.h:99: // Construct with DTLS enabled using the specified |dtls_identity_store| to nit: add an empty line above this comment. same below. (for readability) https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsessiondescriptionfactory.h:138: bool waiting_for_certificate() const { can we call this: waiting_for_certificate_for_testing()
Patchset #2 (id:60001) has been deleted
PTAL. webrtcsession_unittest has been parameterized, testing the usage of both store and provided certificate. I have also addressed tommi's comments. https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:724: if (!certificate) { On 2015/08/20 14:58:11, tommi (webrtc) wrote: > I don't think you need this variable actually. > Here you could just do > > if (rtc_configuration.certificates.empty()) { You're right. In latest PS though certificate != nullptr became relevant in two places and the variable more necessary so I still have it but it's moved. https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:735: // Use the already generated certificate. On 2015/08/20 14:58:11, tommi (webrtc) wrote: > is the intent to support multiple certificates eventually? The specs say if multiple are provided it is up to us to choose which one to use but it doesn't say how to pick one. Unless there are compatibility issues(?) this is one way to do it ;) But yeah, me and torbjorng talked about this briefly. If which one to use matters compatibility/security/performance-wise we should have a function that decides which one to pick... (although performance-wise: at this point the certificates have already been generated). I'll add a TODO, forgot to do that. https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right): https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsessiondescriptionfactory.cc:187: new rtc::RefCountedObject<WebRtcIdentityRequestObserver>(); On 2015/08/20 14:58:11, tommi (webrtc) wrote: > initialize in the initializer list? Done. This required moving it to helper constructor's initializer list and initializing it there, but it allowed it to be made const. https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsessiondescriptionfactory.h (right): https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsessiondescriptionfactory.h:99: // Construct with DTLS enabled using the specified |dtls_identity_store| to On 2015/08/20 14:58:11, tommi (webrtc) wrote: > nit: add an empty line above this comment. same below. (for readability) Done. https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsessiondescriptionfactory.h:138: bool waiting_for_certificate() const { On 2015/08/20 14:58:11, tommi (webrtc) wrote: > can we call this: > > waiting_for_certificate_for_testing() Done. https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession_unittest.cc (left): https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession_unittest.cc:3882: Two things wrong with this, which is why I removed it: - A unittest should not do things that will have consequences outside of the test being teared down. Quitting the thread has the lasting consequence that the thread has stopped. If other tests run after it and for example try to do Invoke they will not succeed (and the fact that Invoke did nothing is not immediately clear). Adding TEST_P's changed the order of some tests and exposed this issue. (Previously this was always the last test being run). - Quit() does not look like it is actually process pending messages(?), it just marks it stopped and wakes the thread up (but it's already awake?). With messages not being processed anyway I did not replace this by a message processing loop.
https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:582: // TODO(hbos,torbjorng): How to decide which certificate to use? My understanding is that multiple certificates make sense for DTLS negotiations. Quoting from the RFC at 4.2.1.1: "Although any given DTLS connection will use only one certificate, this attribute allows the caller to provide multiple certificates that support different algorithms. The final certificate will be selected based on the DTLS handshake, which establishes which certificates are allowed. The RTCPeerConnection implementation selects which of the certificates is used for a given connection; how certificates are selected is outside the scope of this specification." https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:728: if (!certificate) { Is dtls_identity_store.Pass() and certificate type compatible? Then, consider combining these calls via a conditionally assigned parameter. https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession_unittest.cc:410: void InitWithDtls(RTCCertificateGenerationMethod cert_gen_method) { Argument splits method into disjoints chunks. Consider splitting into two methods, or else if possible parameterize and share code between then/else. https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession_unittest.cc:1211: InitWithDtls(DTLS_IDENTITY_STORE); Should we use both cert sources here (as provided by TEST_P)? https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right): https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsessiondescriptionfactory.cc:391: // SSLIdentity* (then there will be no need to do GetReference here). Perhaps we ought to rename GetReference to make things less confusing?
can you ping back after addressing Torbjorn's comments? https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectioninterface.h (right): https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectioninterface.h:246: std::vector<rtc::scoped_refptr<rtc::RTCCertificate>> certificates; This "struct" is outgrowing it's "structness". Can you add a TODO for you, me or whomever to change it to be a class and make the member variables private? Since we're now adding a vector, pointers to refcounted objects etc, it is becoming more and more important to check correct usage of this type. (thread checks etc)
PTAL, comments addressed. https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectioninterface.h (right): https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectioninterface.h:246: std::vector<rtc::scoped_refptr<rtc::RTCCertificate>> certificates; On 2015/08/21 14:24:39, tommi (webrtc) wrote: > This "struct" is outgrowing it's "structness". Can you add a TODO for you, me > or whomever to change it to be a class and make the member variables private? > Since we're now adding a vector, pointers to refcounted objects etc, it is > becoming more and more important to check correct usage of this type. (thread > checks etc) Done. https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:582: // TODO(hbos,torbjorng): How to decide which certificate to use? On 2015/08/21 12:39:19, torbjorng1 wrote: > My understanding is that multiple certificates make sense for DTLS negotiations. > Quoting from the RFC at 4.2.1.1: > > "Although any given DTLS connection will use only one certificate, this > attribute allows the caller to provide multiple certificates that support > different algorithms. The final certificate will be selected based on the DTLS > handshake, which establishes which certificates are allowed. The > RTCPeerConnection implementation selects which of the certificates is used for a > given connection; how certificates are selected is outside the scope of this > specification." Acknowledged. We must look into this more, major code changes might be necessary including passing around a collection of certificates instead of a single one. But leaving as-is for the current CL. Let's walk before we can run; let's support ECDSA before we can do proper DTLS negotiations. https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession.cc:728: if (!certificate) { On 2015/08/21 12:39:19, torbjorng1 wrote: > Is dtls_identity_store.Pass() and certificate type compatible? > Then, consider combining these calls via a conditionally assigned parameter. You either use store and not certificate, or certificate and not store. If it took both then it would have to ignore one of the parameters if the other is not null (ignore store if cert != null). I originally made a constructor that took both parameters but was asked in the previous version of these CLs to split it up into different constructor calls. https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession_unittest.cc:410: void InitWithDtls(RTCCertificateGenerationMethod cert_gen_method) { On 2015/08/21 12:39:19, torbjorng1 wrote: > Argument splits method into disjoints chunks. Consider splitting into two > methods, or else if possible parameterize and share code between then/else. Done. https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsession_unittest.cc:1211: InitWithDtls(DTLS_IDENTITY_STORE); On 2015/08/21 12:39:19, torbjorng1 wrote: > Should we use both cert sources here (as provided by TEST_P)? Yeah why not! Done. https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right): https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses... talk/app/webrtc/webrtcsessiondescriptionfactory.cc:391: // SSLIdentity* (then there will be no need to do GetReference here). On 2015/08/21 12:39:19, torbjorng1 wrote: > Perhaps we ought to rename GetReference to make things less confusing? Sure but let's do that in a separate CL, added a TODO.
https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsession.cc:729: // Use the |dtls_identity_store| to generate a certificate. can you add a DCHECK(dtls_identity_store) https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsession_unittest.cc (left): https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsession_unittest.cc:3881: rtc::Thread::Current()->Quit(); why removing this? https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right): https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsessiondescriptionfactory.cc:245: if (msg.message_id != MSG_USE_CONSTRUCTOR_CERTIFICATE) { https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsessiondescriptionfactory.h (right): https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsessiondescriptionfactory.h:102: WebRtcSessionDescriptionFactory( will this ctor eventually be deleted? https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsessiondescriptionfactory.h:117: rtc::scoped_refptr<rtc::RTCCertificate> certificate, const &
PTAL, addressed comments again. https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsession.cc:729: // Use the |dtls_identity_store| to generate a certificate. On 2015/08/24 11:26:32, tommi (webrtc) wrote: > can you add a DCHECK(dtls_identity_store) Done. https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsession_unittest.cc (left): https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsession_unittest.cc:3881: rtc::Thread::Current()->Quit(); On 2015/08/24 11:26:33, tommi (webrtc) wrote: > why removing this? I commented on this earlier: "Two things wrong with this, which is why I removed it: - A unittest should not do things that will have consequences outside of the test being teared down. Quitting the thread has the lasting consequence that the thread has stopped. If other tests run after it and for example try to do Invoke they will not succeed (and the fact that Invoke did nothing is not immediately clear). Adding TEST_P's changed the order of some tests and exposed this issue. (Previously this was always the last test being run). - Quit() does not look like it is actually processing pending messages(?), it just marks it stopped and wakes the thread up (but it's already awake?). With messages not being processed anyway I did not replace this by a message processing loop." https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right): https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsessiondescriptionfactory.cc:245: if (msg.message_id != MSG_USE_CONSTRUCTOR_CERTIFICATE) On 2015/08/24 11:26:33, tommi (webrtc) wrote: > { Done. https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsessiondescriptionfactory.h (right): https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsessiondescriptionfactory.h:102: WebRtcSessionDescriptionFactory( On 2015/08/24 11:26:33, tommi (webrtc) wrote: > will this ctor eventually be deleted? Remember that if no certificates are provided we still have to generate them for the caller, with the same functionality as before the latest RFC. |dtls_identity_store| is required for as long as we want to allow projects using WebRTC to be able to supply own identity generation code in this event. Such as Chrome having its own RSA generation code today. It would be nice to remove it but that will likely have performance implications. In fact, for as long as this holds true, our GenerateCertificate function (not yet added) will need to (optionally) take a |dtls_identity_store| argument as well. When ECDSA is the default and RSA is "almost abandoned" we might not care about this and always use the webrtc implementation of the store, in which case they can be removed (along with the possibility of Chrome generating keys before WebRTC comes into the picture). https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsessiondescriptionfactory.h:117: rtc::scoped_refptr<rtc::RTCCertificate> certificate, On 2015/08/24 11:26:33, tommi (webrtc) wrote: > const & Done.
lgtm https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsession_unittest.cc (left): https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsession_unittest.cc:3881: rtc::Thread::Current()->Quit(); On 2015/08/24 12:01:34, hbos wrote: > On 2015/08/24 11:26:33, tommi (webrtc) wrote: > > why removing this? > > I commented on this earlier: > "Two things wrong with this, which is why I removed it: > - A unittest should not do things that will have consequences outside of the > test being teared down. Quitting the thread has the lasting consequence that the > thread has stopped. If other tests run after it and for example try to do Invoke > they will not succeed (and the fact that Invoke did nothing is not immediately > clear). Adding TEST_P's changed the order of some tests and exposed this issue. > (Previously this was always the last test being run). This thread is created by and for the test. If other tests are relying on it, aren't they broken or the harness itself? > - Quit() does not look like it is actually processing pending messages(?), it > just marks it stopped and wakes the thread up (but it's already awake?). > > With messages not being processed anyway I did not replace this by a message > processing loop." As discussed offline, I'm going to look into fixing the issue whereby the ThreadManager implicitly wraps the current thread and causes the main thread object (and queue) to be shared by multiple different tests. I think this sort of thing could be causing flakiness.
LGTM. https://codereview.webrtc.org/1288033009/diff/120001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1288033009/diff/120001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsession.cc:582: // TODO(hbos,torbjorng): How to decide which certificate to use? Please spell out the [0] uglyness in the comment.
https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsession_unittest.cc (left): https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsession_unittest.cc:3881: rtc::Thread::Current()->Quit(); On 2015/08/24 15:15:12, tommi (webrtc) wrote: > On 2015/08/24 12:01:34, hbos wrote: > > On 2015/08/24 11:26:33, tommi (webrtc) wrote: > > > why removing this? > > > > I commented on this earlier: > > "Two things wrong with this, which is why I removed it: > > - A unittest should not do things that will have consequences outside of the > > test being teared down. Quitting the thread has the lasting consequence that > the > > thread has stopped. If other tests run after it and for example try to do > Invoke > > they will not succeed (and the fact that Invoke did nothing is not immediately > > clear). Adding TEST_P's changed the order of some tests and exposed this > issue. > > (Previously this was always the last test being run). > > This thread is created by and for the test. If other tests are relying on it, > aren't they broken or the harness itself? > > > - Quit() does not look like it is actually processing pending messages(?), it > > just marks it stopped and wakes the thread up (but it's already awake?). > > > > With messages not being processed anyway I did not replace this by a message > > processing loop." > > As discussed offline, I'm going to look into fixing the issue whereby the > ThreadManager implicitly wraps the current thread and causes the main thread > object (and queue) to be shared by multiple different tests. I think this sort > of thing could be causing flakiness. Acknowledged. https://codereview.webrtc.org/1288033009/diff/120001/talk/app/webrtc/webrtcse... File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1288033009/diff/120001/talk/app/webrtc/webrtcse... talk/app/webrtc/webrtcsession.cc:582: // TODO(hbos,torbjorng): How to decide which certificate to use? On 2015/08/24 15:34:34, torbjorng1 wrote: > Please spell out the [0] uglyness in the comment. Done.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from torbjorng@webrtc.org, tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/1288033009/#ps140001 (title: "Addressed torbjorng's comment and merged with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288033009/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288033009/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Message was sent while issue was closed.
Committed patchset #5 (id:140001) manually as 87713d0fe6fb9c86abe501bdf3d26ef4287ee617 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
