Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1965313002:
JNI+mm: Generate certificate if non-default key type is specified. (Closed)
DescriptionJNI+mm: Generate certificate if non-default key type is specified.
By comparing key type with KT_DEFAULT we remove the implicit assumption that
the default is RSA.
Removing the assumptions about what the default is is necessary for a
follow-up CL that will change the default.
BUG=webrtc:5795, webrtc:5707
R=hta@webrtc.org, magjed@webrtc.org, tommi@webrtc.org
TBR=tkchin@webrtc.org
Committed: https://crrev.com/e06c2ddbdea7cfdeae82c3a7ae9f9ea2978f7394
Cr-Commit-Position: refs/heads/master@{#12722}
Patch Set 1 #
Total comments: 3
Patch Set 2 : jni returning null instead #
Total comments: 2
Patch Set 3 : mm: nativeConfiguration logging and returning null on failure #Patch Set 4 : Rebase with master #
Total comments: 14
Messages
Total messages: 36 (15 generated)
Description was changed from ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Also in this change, if we fail to generate a certificate we crash instead of trying to recover by falling back on the default. Under normal circumstances the generation method will never fail so I think we should only preceed if we get the certificate we asked for. Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795 ========== to ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Also in this change, if we fail to generate a certificate we crash instead of trying to recover by falling back on the default. Under normal circumstances the generation method will never fail so I think we should only preceed if we get the certificate we asked for (also strange from a security perspective if you could get a different certificate than you say to use). Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795 ==========
Description was changed from ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Also in this change, if we fail to generate a certificate we crash instead of trying to recover by falling back on the default. Under normal circumstances the generation method will never fail so I think we should only preceed if we get the certificate we asked for (also strange from a security perspective if you could get a different certificate than you say to use). Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795 ========== to ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Also in this change, if we fail to generate a certificate we crash instead of trying to recover by falling back on the default. Under normal circumstances the generation method will never fail so I think we should only preceed if we get the certificate we asked for (also strange from a security perspective if you could get a different certificate than you say to use). Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795 ==========
Description was changed from ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Also in this change, if we fail to generate a certificate we crash instead of trying to recover by falling back on the default. Under normal circumstances the generation method will never fail so I think we should only preceed if we get the certificate we asked for (also strange from a security perspective if you could get a different certificate than you say to use). Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795 ========== to ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Also in this change, if we fail to generate a certificate we crash instead of trying to recover by falling back on the default. Under normal circumstances the generation method will never fail so I think we should only preceed if we get the certificate we asked for (also strange from a security perspective if you could get a different certificate than you say to use). Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795 ==========
hbos@webrtc.org changed reviewers: + hta@webrtc.org, magjed@webrtc.org, mattdr@google.com
Please take a look, magjed, hta and mattdr.
Description was changed from ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Also in this change, if we fail to generate a certificate we crash instead of trying to recover by falling back on the default. Under normal circumstances the generation method will never fail so I think we should only preceed if we get the certificate we asked for (also strange from a security perspective if you could get a different certificate than you say to use). Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795 ========== to ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Also in this change, if we fail to generate a certificate we crash instead of trying to recover by falling back on the default. Under normal circumstances the generation method will never fail so I think we should only preceed if we get the certificate we asked for (also strange from a security perspective if you could get a different certificate than you say to use). Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795, webrtc:5707 ==========
https://codereview.webrtc.org/1965313002/diff/1/webrtc/api/java/jni/peerconne... File webrtc/api/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1965313002/diff/1/webrtc/api/java/jni/peerconne... webrtc/api/java/jni/peerconnection_jni.cc:1575: RTC_CHECK(certificate); If this can ever fail, I think it's better to log and return nullptr instead of crashing.
Need to decide on the crash vs throw thing. Java seems to like throwing exceptions, so that might be the right thing to do (if easy). Are there tests for Java and MM that should have been affected? https://codereview.webrtc.org/1965313002/diff/1/webrtc/api/java/jni/peerconne... File webrtc/api/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1965313002/diff/1/webrtc/api/java/jni/peerconne... webrtc/api/java/jni/peerconnection_jni.cc:1575: RTC_CHECK(certificate); On 2016/05/12 08:04:08, magjed_webrtc wrote: > If this can ever fail, I think it's better to log and return nullptr instead of > crashing. Does putting a null pointer in rtc_config.certificates make sense? (I don't think so.) This function doesn't return a value - does it have the option of returning a status or throwing a Java exception? (I don't know JNI) If we don't have a contract that permits throwing or returning null, I think crashing is the right thing to do.
PTAL magjed, hta. https://codereview.webrtc.org/1965313002/diff/1/webrtc/api/java/jni/peerconne... File webrtc/api/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1965313002/diff/1/webrtc/api/java/jni/peerconne... webrtc/api/java/jni/peerconnection_jni.cc:1575: RTC_CHECK(certificate); On 2016/05/12 08:12:25, hta-webrtc wrote: > On 2016/05/12 08:04:08, magjed_webrtc wrote: > > If this can ever fail, I think it's better to log and return nullptr instead > of > > crashing. > > Does putting a null pointer in rtc_config.certificates make sense? (I don't > think so.) > > This function doesn't return a value - does it have the option of returning a > status or throwing a Java exception? (I don't know JNI) > > If we don't have a contract that permits throwing or returning null, I think > crashing is the right thing to do. It returns a pointer (jlong) to the resulting peer connection, it returning null (0) is handled here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc.... So it is indeed possible, then it is up to the java layer to handle null. I updated it to do that. https://codereview.webrtc.org/1965313002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm (right): https://codereview.webrtc.org/1965313002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:97: RTC_CHECK(certificate); Here I still crash though. Since it is converting to the C++ version of a configuration and it fails, surrounding code does not handle it (like null is being handled in the java case): https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... In this case handling failure without crashing would be a lot of work. It is my understanding that it failing is more of a theoretical scenario anyway, like if you run out of memory, or something incredibly rare happens, I think crashing is fine. Our test code CHECK that certificates are generated all the time and they don't fail.
https://codereview.webrtc.org/1965313002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm (right): https://codereview.webrtc.org/1965313002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:97: RTC_CHECK(certificate); On 2016/05/12 08:27:08, hbos wrote: > Here I still crash though. Since it is converting to the C++ version of a > configuration and it fails, surrounding code does not handle it (like null is > being handled in the java case): > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > In this case handling failure without crashing would be a lot of work. > > It is my understanding that it failing is more of a theoretical scenario anyway, > like if you run out of memory, or something incredibly rare happens, I think > crashing is fine. > > Our test code CHECK that certificates are generated all the time and they don't > fail. Wait let me see if I can make setConfiguration return false and if that code path already covers failure.
PTAL magjed, hta. Handling generation failure (though unlikely) in both jni and mm case.
webrtc/api/java/jni/peerconnection_jni.cc lgtm
lgtm
The CQ bit was checked by hbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1965313002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1965313002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/13247)
hbos@webrtc.org changed reviewers: + tkchin@webrtc.org - mattdr@google.com
Please take a look, tkchin.
Description was changed from ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Also in this change, if we fail to generate a certificate we crash instead of trying to recover by falling back on the default. Under normal circumstances the generation method will never fail so I think we should only preceed if we get the certificate we asked for (also strange from a security perspective if you could get a different certificate than you say to use). Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795, webrtc:5707 ========== to ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795, webrtc:5707 ==========
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
lgtm
On 2016/05/13 11:03:28, tommi-webrtc wrote: > lgtm With this I'll TBR=tkchin and land. Important for the EC default launch.
Description was changed from ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795, webrtc:5707 ========== to ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795, webrtc:5707 TBR=tkchin@webrtc.org ==========
The CQ bit was checked by hbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1965313002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1965313002/60001
The CQ bit was unchecked by hbos@webrtc.org
Message was sent while issue was closed.
Description was changed from ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795, webrtc:5707 TBR=tkchin@webrtc.org ========== to ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795, webrtc:5707 R=hta@webrtc.org, magjed@webrtc.org, tommi@webrtc.org TBR=tkchin@webrtc.org Committed: https://crrev.com/e06c2ddbdea7cfdeae82c3a7ae9f9ea2978f7394 Cr-Commit-Position: refs/heads/master@{#12722} ==========
Message was sent while issue was closed.
Description was changed from ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795, webrtc:5707 R=hta@webrtc.org, magjed@webrtc.org, tommi@webrtc.org TBR=tkchin@webrtc.org Committed: https://crrev.com/e06c2ddbdea7cfdeae82c3a7ae9f9ea2978f7394 Cr-Commit-Position: refs/heads/master@{#12722} ========== to ========== JNI+mm: Generate certificate if non-default key type is specified. By comparing key type with KT_DEFAULT we remove the implicit assumption that the default is RSA. Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default. BUG=webrtc:5795, webrtc:5707 R=hta@webrtc.org, magjed@webrtc.org, tommi@webrtc.org TBR=tkchin@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/e06c2ddbdea7cfdeae82c3a7a... ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e06c2ddbdea7cfdeae82c3a7ae9f9ea2978f7394 Cr-Commit-Position: refs/heads/master@{#12722}
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as e06c2ddbdea7cfdeae82c3a7ae9f9ea2978f7394 (presubmit successful).
Message was sent while issue was closed.
When is the EC default launch? I thought TBRs were reserved for build emergencies or for simple bot configuration changes. https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h (right): https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h:24: webrtc::PeerConnectionInterface::RTCConfiguration* nativeConfiguration; RTCConfiguration *nativeConfiguration; This should be a method instead of a property because it is actually creating an object that needs to be released or at the very least commented to indicate that manual memory management is required. The style is to prefix with "create". - (webrtc::PeerConnectionInterface::RTCConfiguration *)createNativeConfiguration; https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm (right): https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:78: - (webrtc::PeerConnectionInterface::RTCConfiguration*)nativeConfiguration { RTCConfiguration *) https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:79: std::unique_ptr<webrtc::PeerConnectionInterface::RTCConfiguration> Are we returning a pointer to avoid memory copy? Or to surface an error with certificate generation? Why don't we want a default fallback? This will cause RTCPeerConnectionFactory to potentially return nil on init, which isn't great because it's like saying your constructor failed. We generally reserve returning nil on init for fairly exceptional circumstances - when is certificate generation expected to fail? It may be better for RTCConfiguration to store a copy of the C++ object on init, so RTCConfiguration is the object that returns nil on init instead, which makes more sense, and will be caught by assertions in RTCPeerConnectionFactory. https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:108: RTCLogWarning(@"Failed to generate certificate."); this is an error because you will fail to create things like factory RTCLogError https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:203: return nullptr; ObjC style requires braces nil not nullptr. if (!nativeConfiguration) { return nil; } https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:259: return false; BOOL is not a boolean if (!nativeConfiguration) { return NO; }
Message was sent while issue was closed.
On 2016/05/13 17:48:06, tkchin_webrtc wrote: > When is the EC default launch? I thought TBRs were reserved for build > emergencies or for simple bot configuration changes. > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h (right): > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h:24: > webrtc::PeerConnectionInterface::RTCConfiguration* nativeConfiguration; > RTCConfiguration *nativeConfiguration; > > This should be a method instead of a property because it is actually creating an > object that needs to be released or at the very least commented to indicate that > manual memory management is required. The style is to prefix with "create". > > - (webrtc::PeerConnectionInterface::RTCConfiguration > *)createNativeConfiguration; > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm (right): > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:78: - > (webrtc::PeerConnectionInterface::RTCConfiguration*)nativeConfiguration { > RTCConfiguration *) > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:79: > std::unique_ptr<webrtc::PeerConnectionInterface::RTCConfiguration> > Are we returning a pointer to avoid memory copy? Or to surface an error with > certificate generation? Why don't we want a default fallback? > > This will cause RTCPeerConnectionFactory to potentially return nil on init, > which isn't great because it's like saying your constructor failed. > We generally reserve returning nil on init for fairly exceptional circumstances > - when is certificate generation expected to fail? > > It may be better for RTCConfiguration to store a copy of the C++ object on init, > so RTCConfiguration is the object that returns nil on init instead, which makes > more sense, and will be caught by assertions in RTCPeerConnectionFactory. > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:108: > RTCLogWarning(@"Failed to generate certificate."); > this is an error because you will fail to create things like factory > RTCLogError > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:203: return nullptr; > ObjC style requires braces > > nil not nullptr. > if (!nativeConfiguration) { > return nil; > } > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:259: return false; > BOOL is not a boolean > > if (!nativeConfiguration) { > return NO; > } It hasn't been launched, but the target is to include it in 52. We can revert this CL and fix the comments. Wdyt?
Message was sent while issue was closed.
On 2016/05/13 17:50:48, tommi-webrtc wrote: > On 2016/05/13 17:48:06, tkchin_webrtc wrote: > > When is the EC default launch? I thought TBRs were reserved for build > > emergencies or for simple bot configuration changes. > > > > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h (right): > > > > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h:24: > > webrtc::PeerConnectionInterface::RTCConfiguration* nativeConfiguration; > > RTCConfiguration *nativeConfiguration; > > > > This should be a method instead of a property because it is actually creating > an > > object that needs to be released or at the very least commented to indicate > that > > manual memory management is required. The style is to prefix with "create". > > > > - (webrtc::PeerConnectionInterface::RTCConfiguration > > *)createNativeConfiguration; > > > > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm (right): > > > > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:78: - > > (webrtc::PeerConnectionInterface::RTCConfiguration*)nativeConfiguration { > > RTCConfiguration *) > > > > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:79: > > std::unique_ptr<webrtc::PeerConnectionInterface::RTCConfiguration> > > Are we returning a pointer to avoid memory copy? Or to surface an error with > > certificate generation? Why don't we want a default fallback? > > > > This will cause RTCPeerConnectionFactory to potentially return nil on init, > > which isn't great because it's like saying your constructor failed. > > We generally reserve returning nil on init for fairly exceptional > circumstances > > - when is certificate generation expected to fail? > > > > It may be better for RTCConfiguration to store a copy of the C++ object on > init, > > so RTCConfiguration is the object that returns nil on init instead, which > makes > > more sense, and will be caught by assertions in RTCPeerConnectionFactory. > > > > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:108: > > RTCLogWarning(@"Failed to generate certificate."); > > this is an error because you will fail to create things like factory > > RTCLogError > > > > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): > > > > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:203: return nullptr; > > ObjC style requires braces > > > > nil not nullptr. > > if (!nativeConfiguration) { > > return nil; > > } > > > > > https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:259: return false; > > BOOL is not a boolean > > > > if (!nativeConfiguration) { > > return NO; > > } > > It hasn't been launched, but the target is to include it in 52. We can revert > this CL and fix the comments. Wdyt? I'm fine with fixing in subsequent CL since it looks like there is still more work to do anyway ("Removing the assumptions about what the default is is necessary for a follow-up CL that will change the default.")
Message was sent while issue was closed.
https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm (right): https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:79: std::unique_ptr<webrtc::PeerConnectionInterface::RTCConfiguration> On 2016/05/13 17:48:05, tkchin_webrtc wrote: > Are we returning a pointer to avoid memory copy? Or to surface an error with > certificate generation? Why don't we want a default fallback? > > This will cause RTCPeerConnectionFactory to potentially return nil on init, > which isn't great because it's like saying your constructor failed. > We generally reserve returning nil on init for fairly exceptional circumstances > - when is certificate generation expected to fail? > > It may be better for RTCConfiguration to store a copy of the C++ object on init, > so RTCConfiguration is the object that returns nil on init instead, which makes > more sense, and will be caught by assertions in RTCPeerConnectionFactory. and by factory I meant peerconnection's private initWithFactory:configuration... method :) which I guess isn't so bad because it's private and within our control. https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:108: RTCLogWarning(@"Failed to generate certificate."); On 2016/05/13 17:48:05, tkchin_webrtc wrote: > this is an error because you will fail to create things like factory > RTCLogError I meant peerconnection via [factory peerconnectionWithConfiguration..]
Message was sent while issue was closed.
Comments addressed in this follow-up CL: https://codereview.webrtc.org/1978233002/ Will copy this reply to comments so that the discussion can continue there. https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h (right): https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h:24: webrtc::PeerConnectionInterface::RTCConfiguration* nativeConfiguration; On 2016/05/13 17:48:05, tkchin_webrtc wrote: > RTCConfiguration *nativeConfiguration; > > This should be a method instead of a property because it is actually creating an > object that needs to be released or at the very least commented to indicate that > manual memory management is required. The style is to prefix with "create". > > - (webrtc::PeerConnectionInterface::RTCConfiguration > *)createNativeConfiguration; Done. https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm (right): https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:78: - (webrtc::PeerConnectionInterface::RTCConfiguration*)nativeConfiguration { On 2016/05/13 17:48:05, tkchin_webrtc wrote: > RTCConfiguration *) Done. https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:79: std::unique_ptr<webrtc::PeerConnectionInterface::RTCConfiguration> On 2016/05/13 18:07:38, tkchin_webrtc wrote: > On 2016/05/13 17:48:05, tkchin_webrtc wrote: > > Are we returning a pointer to avoid memory copy? Or to surface an error with > > certificate generation? Why don't we want a default fallback? To say that we failed to generate the configuration asked for (surface certificate failure). As for attempting to generate a different certificate as a fallback, if you choose ECDSA over RSA for security reasons it may(?) be bad if you implicitly end up with an RSA certificate. > > This will cause RTCPeerConnectionFactory to potentially return nil on init, > > which isn't great because it's like saying your constructor failed. > > We generally reserve returning nil on init for fairly exceptional > circumstances > > - when is certificate generation expected to fail? Under very rare circumstances. I don't know how to make it fail or if this ever happens in practice. Our unittests EXPECT generate certificate to return a certificate and I haven't ever seen the unittest failing to produce one. But the lower-level crypto libraries could return null and that is propagated by the interface instead of crashing. > > It may be better for RTCConfiguration to store a copy of the C++ object on > init, > > so RTCConfiguration is the object that returns nil on init instead, which > makes > > more sense, and will be caught by assertions in RTCPeerConnectionFactory. > > and by factory I meant peerconnection's private initWithFactory:configuration... > method :) > which I guess isn't so bad because it's private and within our control. Do you mean to move the GenerateCertificate code from createNativeConfiguration to RTCConfiguration's init? That would make _keyType pointless, and it should then be removed or be made const? If we don't move the code we have the option to modify the default value of _keyType to support other certificates. In both cases we end up with an init that could conceivably return nil. Alternatively we could add an assertion that GenerateCertificate does not fail and change the code back so that we do not propagate the error. What do you think I should do? Currently I did not change this and still have createNativeConfiguration. https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:108: RTCLogWarning(@"Failed to generate certificate."); On 2016/05/13 18:07:38, tkchin_webrtc wrote: > On 2016/05/13 17:48:05, tkchin_webrtc wrote: > > this is an error because you will fail to create things like factory > > RTCLogError > > I meant peerconnection via [factory peerconnectionWithConfiguration..] RTCLogError - Done. https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:203: return nullptr; On 2016/05/13 17:48:06, tkchin_webrtc wrote: > ObjC style requires braces > > nil not nullptr. > if (!nativeConfiguration) { > return nil; > } Done. https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:259: return false; On 2016/05/13 17:48:06, tkchin_webrtc wrote: > BOOL is not a boolean > > if (!nativeConfiguration) { > return NO; > } Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
