Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15969025:
Generates the DTLS identity in browser process and returns it to render process. (Closed)
|
Created:
7 years, 6 months ago by jiayl Modified:
7 years, 5 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, juberti Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionGenerates the DTLS identity in browser process and returns it to the renderer process.
This is the first part of implementing a persistent identity store for WebRTC in Chrome. It implements the IPC messages to request and return the DTLS identity and the identity generation in browser process. A persistent store based on SQLite DB will be implemented later.
DTLSIdentityService: used in the renderer process to request a DTLS identity. It sends an IPC to the browser process and receives the reply containing the identity in a callback.
DTLSIdentityServiceHost: listens for the IPC message from DTLSIdentityService and passes the request to DTLSIdentityStore.
DTLSIdentityStore: created per RenerProcessHost. For now it always generates a new identity on a worker thread for each request, without any caching or persistent storage.
BUG=https://code.google.com/p/webrtc/issues/detail?id=1864
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209499
Patch Set 1 : #
Total comments: 11
Patch Set 2 : #
Total comments: 28
Patch Set 3 : async #Patch Set 4 : #
Total comments: 38
Patch Set 5 : fix #
Total comments: 19
Patch Set 6 : #
Total comments: 13
Patch Set 7 : fix #
Total comments: 1
Patch Set 8 : #Patch Set 9 : #
Total comments: 33
Patch Set 10 : #
Total comments: 30
Patch Set 11 : #
Total comments: 10
Patch Set 12 : #
Total comments: 13
Patch Set 13 : #
Total comments: 5
Patch Set 14 : #
Total comments: 17
Patch Set 15 : #
Total comments: 18
Patch Set 16 : fix #
Total comments: 7
Patch Set 17 : #
Total comments: 18
Patch Set 18 : #
Total comments: 51
Patch Set 19 : #
Total comments: 7
Patch Set 20 : #Patch Set 21 : sync #Patch Set 22 : #Patch Set 23 : #Patch Set 24 : #Patch Set 25 : #Patch Set 26 : #Messages
Total messages: 80 (0 generated)
Could you take a look? jam: content/, content/common/, tsepez: security review for the new IPC message sky: content/browser/renderer_host fischman: content/*/media ekr, rsleevi: identity generation and DTLSIdentityService interface
https://codereview.chromium.org/15969025/diff/2001/content/browser/renderer_h... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/15969025/diff/2001/content/browser/renderer_h... content/browser/renderer_host/render_process_host_impl.h:333: scoped_refptr<DTLSIdentityServiceHost> dtls_identity_service_host_; if defined(ENABLE_WEBRTC)?
Messages - my only concern is what the interaction might be in a site-isolated world. We want to ensure that a renderer can obtain information about URLs only for the domains which it has been designated to service. Is there any harm in returning an identity for an URL to a (compromised) renderer that isn't permitted to service that domain? +creis otherwise LGTM.
https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.cc:8: #include <cert.h> BUG: You cannot depend on this header directly in content/. This can ONLY be used in NSS-only files. Same with all types used beyond. That is, it MAY be acceptable to separate the design into dtls_identity_store, dtls_identity_store_nss, and dtls_identity_store_openssl, but please think carefully about the design before doing so. We have intentional abstractions like crypto/ and net/ to give you implementation-agnostic types. https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.cc:19: #include "net/cert/x509_util_nss.h" BUG: You cannot depend on this header directly in content/. This can ONLY be used in NSS-only files. https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:24: static DTLSIdentityStore* GetInstance(); DESIGN: No new singletons. DESIGN: You can't mix the guarantees of a singleton while still having public constructors. It's a design contract that cannot be met. https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:26: DTLSIdentityStore(const scoped_refptr<base::TaskRunner>& task_runner); STYLE: explicit https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:34: // |url| is the frame URL of the PeerConnection requesting the identity; STYLE: terms like "frame URL" don't really fit here. What if the page isn't using frames? It's better to express what it is (and perhaps rename the variable) - it's the origin, is it not? https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:41: const OnCompleteCallback& callback DESIGN: If a requester goes away while an identity request is being made, is there any way to abort this request? https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:42: ); STYLE: closing parens go on the same line as the last parameter. https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store_unittest.cc:39: } There is no need to use SetUp/TearDown see https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de... https://codereview.chromium.org/15969025/diff/4002/content/renderer/media/dtl... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/4002/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:8: STYLE: Delete line break. https://codereview.chromium.org/15969025/diff/4002/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:9: #include <string> STYLE: add line break https://codereview.chromium.org/15969025/diff/4002/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:19: DTLSIdentityService(const GURL& url); STYLE: Explicit
Only looked at content/renderer/media/ https://codereview.chromium.org/15969025/diff/2001/content/renderer/media/dtl... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/2001/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:19: DTLSIdentityService(const GURL& url); explicit https://codereview.chromium.org/15969025/diff/2001/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:20: virtual ~DTLSIdentityService(); virtual dtor implies you expect to be subclassed (as does the "virtual" on the GOGI() below). Is that really the case? https://codereview.chromium.org/15969025/diff/2001/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:22: // Called on the renderer main thread to get the DTLS identity or geneate a typo: geneate https://codereview.chromium.org/15969025/diff/2001/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:24: // |identity_name| is the name of the identity; This is uninformative, and I am left not knowing what goes here. Would an example help? Or a pointer to a spec for the functionality this provides? https://codereview.chromium.org/15969025/diff/2001/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:25: // |common_name| ???; ditto^2 https://codereview.chromium.org/15969025/diff/2001/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:28: virtual bool GetOrGenerateIdentity(const std::string& identity_name, "bool" implies failure is possible; what can cause failure? I _think_ the answer is "channel to the browser is closed", but I also think that's probably a terminal condition. Does this need to return bool as opposed to void+CHECK? https://codereview.chromium.org/15969025/diff/2001/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:37: DISALLOW_COPY_AND_ASSIGN(DTLSIdentityService); DISALLOW_IMPLICIT_CONSTRUCTORS
did you run this in a debug build? it's not clear how this doesn't assert: DTLSIdentityServiceHost::OnGetOrGenerateIdentity runs on the IO thread since it's a message filter. It calls DTLSIdentityStore::GetInstance()->GetOrGenerateIdentity which has DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Independent of that, then you post a task to the UI thread to send the reply. The UI thread can't be involved in the processing of a sync IPC message, or else a deadlock can occur on windows when windowed NPAPI plugins are used. see this comment for more information https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... Does this really need to be a sync IPC? I strongly suggest making it async. making the renderer process paused while this is done isn't good for a variety of obvious reasons
On 2013/06/04 17:57:32, Tom Sepez wrote: > Messages - my only concern is what the interaction might be in a site-isolated > world. We want to ensure that a renderer can obtain information about URLs only > for the domains which it has been designated to service. Is there any harm in > returning an identity for an URL to a (compromised) renderer that isn't > permitted to service that domain? > > +creis > > otherwise LGTM. Currently the webrtc DTLS certificate is generated in the renderer process; my change moves the generation to the browser process and (plans to) cache it per origin. The cached identity is similar to cookies in terms of security properties. An attacker will get the same information it can already get without my change if the renderer is compromised.
> Currently the webrtc DTLS certificate is generated in the renderer process; my > change moves the generation to the browser process and (plans to) cache it per > origin. The cached identity is similar to cookies in terms of security > properties. An attacker will get the same information it can already get without > my change if the renderer is compromised. yes, and this is a step in the right direction, which is why I blessed the CL. However, you're throwing another obstacle to the site isolation project which could be simply avoided by performing a check that the renderer has the rights to sensitive origin data for a given URL. Charlie could probably point you in the right direction.
https://codereview.chromium.org/15969025/diff/2001/content/browser/renderer_h... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/15969025/diff/2001/content/browser/renderer_h... content/browser/renderer_host/render_process_host_impl.h:333: scoped_refptr<DTLSIdentityServiceHost> dtls_identity_service_host_; On 2013/06/04 17:46:07, sky wrote: > if defined(ENABLE_WEBRTC)? Done. https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.cc:19: #include "net/cert/x509_util_nss.h" On 2013/06/04 19:13:52, Ryan Sleevi wrote: > BUG: You cannot depend on this header directly in content/. This can ONLY be > used in NSS-only files. Can I add a new method similar to CreateDomainBoundCertEC to x509_util.h and put the implementation in x509_util_nss.cc calling the existing CreateSelfSignedCert?
> Does this really need to be a sync IPC? I strongly suggest making it async. > making the renderer process paused while this is done isn't good for a variety > of obvious reasons Making the identity request API async will require a lot of more change in Libjingle and highly complicate the code. Since the DTLS identity is already generated synchronously on the main thread in Libjingle before this change, should it be a big concern? On Tue, Jun 4, 2013 at 12:36 PM, <jam@chromium.org> wrote: > did you run this in a debug build? it's not clear how this doesn't assert: > > DTLSIdentityServiceHost::**OnGetOrGenerateIdentity runs on the IO thread > since > it's a message filter. It calls > DTLSIdentityStore::**GetInstance()->**GetOrGenerateIdentity which has > DCHECK(BrowserThread::**CurrentlyOn(BrowserThread::UI)**); > > > Independent of that, then you post a task to the UI thread to send the > reply. > The UI thread can't be involved in the processing of a sync IPC message, > or else > a deadlock can occur on windows when windowed NPAPI plugins are used. see > this > comment for more information > https://code.google.com/p/**chromium/codesearch#chromium/** > src/content/public/browser/**browser_message_filter.cc&q=** > BrowserMessageFilter::**CheckCanDispatchOnUI&sq=** > package:chromium&type=cs&l=103<https://code.google.com/p/chromium/codesearch#chromium/src/content/public/browser/browser_message_filter.cc&q=BrowserMessageFilter::CheckCanDispatchOnUI&sq=package:chromium&type=cs&l=103> > > > Does this really need to be a sync IPC? I strongly suggest making it async. > making the renderer process paused while this is done isn't good for a > variety > of obvious reasons > > > https://codereview.chromium.**org/15969025/<https://codereview.chromium.org/1... >
On 2013/06/04 21:08:54, jiayl wrote: > > Does this really need to be a sync IPC? I strongly suggest making it > async. > > making the renderer process paused while this is done isn't good for a > variety > > of obvious reasons > > Making the identity request API async will require a lot of more change in > Libjingle and highly complicate the code. Since the DTLS identity is > already generated synchronously on the main thread in Libjingle before this > change, should it be a big concern? Generating something through a function call on the renderer thread is very different from now generating it in the browser process and using a synchronous IPC to do so. We try to avoid sync IPCs at all cost because of all the issues they can cause, i.e. this change as written introduces a deadlock.
Yes. This is why I pointed you at the asynchronous API of Origin Bound Certs & Cookies as the model we should be trying to emulate. On Tue, Jun 4, 2013 at 2:08 PM, Jiayang Liu <jiayl@chromium.org> wrote: >> Does this really need to be a sync IPC? I strongly suggest making it >> async. >> making the renderer process paused while this is done isn't good for a >> variety >> of obvious reasons > > Making the identity request API async will require a lot of more change in > Libjingle and highly complicate the code. Since the DTLS identity is already > generated synchronously on the main thread in Libjingle before this change, > should it be a big concern? > > > On Tue, Jun 4, 2013 at 12:36 PM, <jam@chromium.org> wrote: >> >> did you run this in a debug build? it's not clear how this doesn't assert: >> >> DTLSIdentityServiceHost::OnGetOrGenerateIdentity runs on the IO thread >> since >> it's a message filter. It calls >> DTLSIdentityStore::GetInstance()->GetOrGenerateIdentity which has >> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); >> >> >> Independent of that, then you post a task to the UI thread to send the >> reply. >> The UI thread can't be involved in the processing of a sync IPC message, >> or else >> a deadlock can occur on windows when windowed NPAPI plugins are used. see >> this >> comment for more information >> >> https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... >> >> >> Does this really need to be a sync IPC? I strongly suggest making it >> async. >> making the renderer process paused while this is done isn't good for a >> variety >> of obvious reasons >> >> >> https://codereview.chromium.org/15969025/ > >
https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.cc:19: #include "net/cert/x509_util_nss.h" On 2013/06/04 20:23:10, jiayl wrote: > On 2013/06/04 19:13:52, Ryan Sleevi wrote: > > BUG: You cannot depend on this header directly in content/. This can ONLY be > > used in NSS-only files. > > Can I add a new method similar to CreateDomainBoundCertEC to x509_util.h and put > the implementation in x509_util_nss.cc calling the existing > CreateSelfSignedCert? > > > > We'll want to tackle that in a separate CL.
LGTM
On 2013/06/04 20:03:46, Tom Sepez wrote: > > Currently the webrtc DTLS certificate is generated in the renderer process; my > > change moves the generation to the browser process and (plans to) cache it per > > origin. The cached identity is similar to cookies in terms of security > > properties. An attacker will get the same information it can already get > without > > my change if the renderer is compromised. > > yes, and this is a step in the right direction, which is why I blessed the CL. > However, you're throwing another obstacle to the site isolation project which > could be simply avoided by performing a check that the renderer has the rights > to sensitive origin data for a given URL. Charlie could probably point you in > the right direction. I'm glad to hear that this will be scoped per origin similar to cookies. This will let us lock it down to renderer processes based on the origins the process has access to. The final logic for locking things like this down is not in place yet, but I would recommend following the pattern of ChildProcessSecurityPolicyImpl::CanAccessCookiesForOrigin. You're welcome to call that directly if you want it to match the policy for cookies, which might make sense here. If there are reasons for it to differ, then we can think about adding a similar CanAccessDTLSIdentityForOrigin.
But cookies are requested through sync IPC ViewHostMsg_GetCookies<https://code.google.com/p/chromium/codesearch#chromium/src/content/common/view_messages.h&ct=xref_usages&gs=cpp:class-ViewHostMsg_GetCookies@chromium/content/common/view_messages.h%257Cdef&l=1642&gsn=ViewHostMsg_GetCookies> . On Tue, Jun 4, 2013 at 2:11 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > Yes. > > This is why I pointed you at the asynchronous API of Origin Bound > Certs & Cookies as the model we should be trying to emulate. > > On Tue, Jun 4, 2013 at 2:08 PM, Jiayang Liu <jiayl@chromium.org> wrote: > >> Does this really need to be a sync IPC? I strongly suggest making it > >> async. > >> making the renderer process paused while this is done isn't good for a > >> variety > >> of obvious reasons > > > > Making the identity request API async will require a lot of more change > in > > Libjingle and highly complicate the code. Since the DTLS identity is > already > > generated synchronously on the main thread in Libjingle before this > change, > > should it be a big concern? > > > > > > On Tue, Jun 4, 2013 at 12:36 PM, <jam@chromium.org> wrote: > >> > >> did you run this in a debug build? it's not clear how this doesn't > assert: > >> > >> DTLSIdentityServiceHost::OnGetOrGenerateIdentity runs on the IO thread > >> since > >> it's a message filter. It calls > >> DTLSIdentityStore::GetInstance()->GetOrGenerateIdentity which has > >> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); > >> > >> > >> Independent of that, then you post a task to the UI thread to send the > >> reply. > >> The UI thread can't be involved in the processing of a sync IPC message, > >> or else > >> a deadlock can occur on windows when windowed NPAPI plugins are used. > see > >> this > >> comment for more information > >> > >> > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > >> > >> > >> Does this really need to be a sync IPC? I strongly suggest making it > >> async. > >> making the renderer process paused while this is done isn't good for a > >> variety > >> of obvious reasons > >> > >> > >> https://codereview.chromium.org/15969025/ > > > > >
On 2013/06/04 21:59:26, jiayl wrote: > But cookies are requested through sync IPC That's different: 1) this was an existing web standard and we didn't have the ability of making it asynchronous (document.cookie). Here this appears like a new API. Any new API should be designed in a way that's compatible with a multi-process browser. 2) the above doesn't have deadlocks because it goes directly to the IO thread.
https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:24: static DTLSIdentityStore* GetInstance(); On 2013/06/04 19:13:52, Ryan Sleevi wrote: > DESIGN: No new singletons. > DESIGN: You can't mix the guarantees of a singleton while still having public > constructors. It's a design contract that cannot be met. What's the alternative to singleton? I tried to find a global-ish object to own this object and didn't find a good place. ServerBoundCertService lives in the IO thread globals in src/chrome, but it cannot be used in Content.
The new patch changes the sync IPC message to async and resolves other comments. PTAL. Ryan, the use of nss headers will go away when https://codereview.chromium.org/16158005/ lands. And where do you suggest DTLSIdentityStore should live to replace the singleton? https://codereview.chromium.org/15969025/diff/2001/content/renderer/media/dtl... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/2001/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:20: virtual ~DTLSIdentityService(); On 2013/06/04 19:29:09, Ami Fischman wrote: > virtual dtor implies you expect to be subclassed (as does the "virtual" on the > GOGI() below). Is that really the case? It'll implement an interface defined by Libjingle for Libjingle to use, so it should be virtual. https://codereview.chromium.org/15969025/diff/2001/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:28: virtual bool GetOrGenerateIdentity(const std::string& identity_name, On 2013/06/04 19:29:09, Ami Fischman wrote: > "bool" implies failure is possible; what can cause failure? I _think_ the > answer is "channel to the browser is closed", but I also think that's probably a > terminal condition. Does this need to return bool as opposed to void+CHECK? It seems it never returns false for async msg. Changed to void. https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:26: DTLSIdentityStore(const scoped_refptr<base::TaskRunner>& task_runner); On 2013/06/04 19:13:52, Ryan Sleevi wrote: > STYLE: explicit Done. https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:34: // |url| is the frame URL of the PeerConnection requesting the identity; On 2013/06/04 19:13:52, Ryan Sleevi wrote: > STYLE: terms like "frame URL" don't really fit here. What if the page isn't > using frames? It's better to express what it is (and perhaps rename the > variable) - it's the origin, is it not? Renamed to origin. https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:41: const OnCompleteCallback& callback On 2013/06/04 19:13:52, Ryan Sleevi wrote: > DESIGN: If a requester goes away while an identity request is being made, is > there any way to abort this request? How can we tell a requester goes away? It seems the only way is to send another IPC message saying "cancel the previous request". Is it worth the extra complexity? https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:42: ); On 2013/06/04 19:13:52, Ryan Sleevi wrote: > STYLE: closing parens go on the same line as the last parameter. Done. https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store_unittest.cc:39: } On 2013/06/04 19:13:52, Ryan Sleevi wrote: > There is no need to use SetUp/TearDown > > see > https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de... Done. https://codereview.chromium.org/15969025/diff/4002/content/renderer/media/dtl... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/4002/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:8: On 2013/06/04 19:13:52, Ryan Sleevi wrote: > STYLE: Delete line break. Done. https://codereview.chromium.org/15969025/diff/4002/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:9: #include <string> On 2013/06/04 19:13:52, Ryan Sleevi wrote: > STYLE: add line break Done. https://codereview.chromium.org/15969025/diff/4002/content/renderer/media/dtl... content/renderer/media/dtls_identity_service.h:19: DTLSIdentityService(const GURL& url); On 2013/06/04 19:13:52, Ryan Sleevi wrote: > STYLE: Explicit Done.
https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:20: namespace { put this in the content namespace as well, so that you don't need to use content:: here https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:63: LOG(INFO) << "DTLSIdentityStore: a new identity is gnerated."; here and below, does this need to be LOG and not DLOG? all the LOG strings get added to release builds https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... File content/browser/media/dtls_identity_store_browsertest.cc (right): https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... content/browser/media/dtls_identity_store_browsertest.cc:19: class DummyListener : public IPC::Listener { this isn't used https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... content/browser/media/dtls_identity_store_browsertest.cc:39: host->OnMessageReceived(msg, &ok); what is the point of this test? i.e. what failure conditions are you look for? because it looks like it'll pass even if this method doesn't get called. also checking that an IPC handler returns true doesn't tell much, all that means is that it dispatched the IPC https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:22: IPC_MESSAGE_HANDLER(DTLSIdentityMsg_GetOrGenerate, OnGetOrGenerateIdentity) nit: indent next two lines https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:32: LOG(INFO) << "Sent identity request to DTLSIdentityStore with " same comment re logging as other files https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:47: content::BrowserThread::PostTask( you can send the reply directly here (see BrowserMessageFilter::Send), instead of going to the UI thread first. then you don't need to keep render_process_id_ https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:5: #ifndef DTLSIdentityServiceCONTENT_BROWSER_RENDERER_HOST_MEDIA_DTLS_IDENTITY_SERVICE_HOST_H_ nit: fix up ifdef guard https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:11: #include "content/browser/media/dtls_identity_store.h" why? https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:18: } // namespace IPC this is redundant from browser_message_filter.h by definition https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:23: // owned by RenderProcessHostImpl. it's actually owned by the IPC channel proxy. I would just skip the second line, since if someone wants to find out this information they can look for it https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:29: virtual bool OnMessageReceived(const IPC::Message& message, nit: hide this https://codereview.chromium.org/15969025/diff/27001/content/common/media/dtls... File content/common/media/dtls_identity_messages.h (right): https://codereview.chromium.org/15969025/diff/27001/content/common/media/dtls... content/common/media/dtls_identity_messages.h:15: IPC_MESSAGE_CONTROL3(DTLSIdentityMsg_GetOrGenerate, nit: DTLSIdentityHostMsg. we use that convention to differentiate messages to/from the browser when the browser has a class called "Host" https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:26: IPC_MESSAGE_HANDLER(DTLSIdentityMsg_IdentityReady, OnIdentityReady) nit: indent this line and next https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:17: class DTLSIdentityObserver { avoid single-method interfaces, and instead use a callback. this gives more flexibility for the implementer https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:35: virtual bool OnControlMessageReceived(const IPC::Message& message) OVERRIDE; nit: put this in the private section https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:37: // Called on the renderer main thread to get the DTLS identity or generate a nit: by default, code in the renderer runs on the main thread. so no need to call that out https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:42: const std::string& common_name); why is this virtual?
PTAL https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:20: namespace { On 2013/06/06 16:22:32, jam wrote: > put this in the content namespace as well, so that you don't need to use > content:: here Done. https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:63: LOG(INFO) << "DTLSIdentityStore: a new identity is gnerated."; On 2013/06/06 16:22:32, jam wrote: > here and below, does this need to be LOG and not DLOG? all the LOG strings get > added to release builds Done. https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... File content/browser/media/dtls_identity_store_browsertest.cc (right): https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... content/browser/media/dtls_identity_store_browsertest.cc:19: class DummyListener : public IPC::Listener { On 2013/06/06 16:22:32, jam wrote: > this isn't used Done. https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... content/browser/media/dtls_identity_store_browsertest.cc:39: host->OnMessageReceived(msg, &ok); On 2013/06/06 16:22:32, jam wrote: > what is the point of this test? i.e. what failure conditions are you look for? > > because it looks like it'll pass even if this method doesn't get called. also > checking that an IPC handler returns true doesn't tell much, all that means is > that it dispatched the IPC I want to check the IPC handling and DTLSIdentityStore do not throw any error, like asserting on the wrong thread. Could the method not get called? Does not the navigation pump the IO thread tasks? https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:22: IPC_MESSAGE_HANDLER(DTLSIdentityMsg_GetOrGenerate, OnGetOrGenerateIdentity) On 2013/06/06 16:22:32, jam wrote: > nit: indent next two lines Done. https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:32: LOG(INFO) << "Sent identity request to DTLSIdentityStore with " On 2013/06/06 16:22:32, jam wrote: > same comment re logging as other files Done. https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:47: content::BrowserThread::PostTask( On 2013/06/06 16:22:32, jam wrote: > you can send the reply directly here (see BrowserMessageFilter::Send), instead > of going to the UI thread first. then you don't need to keep render_process_id_ Done. https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:5: #ifndef DTLSIdentityServiceCONTENT_BROWSER_RENDERER_HOST_MEDIA_DTLS_IDENTITY_SERVICE_HOST_H_ On 2013/06/06 16:22:32, jam wrote: > nit: fix up ifdef guard Done. https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:11: #include "content/browser/media/dtls_identity_store.h" On 2013/06/06 16:22:32, jam wrote: > why? Removed. https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:18: } // namespace IPC On 2013/06/06 16:22:32, jam wrote: > this is redundant from browser_message_filter.h by definition removed https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:23: // owned by RenderProcessHostImpl. On 2013/06/06 16:22:32, jam wrote: > it's actually owned by the IPC channel proxy. I would just skip the second line, > since if someone wants to find out this information they can look for it Done. https://codereview.chromium.org/15969025/diff/27001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:29: virtual bool OnMessageReceived(const IPC::Message& message, On 2013/06/06 16:22:32, jam wrote: > nit: hide this hmm, but I need to call it from the browser test https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:26: IPC_MESSAGE_HANDLER(DTLSIdentityMsg_IdentityReady, OnIdentityReady) On 2013/06/06 16:22:32, jam wrote: > nit: indent this line and next Done. https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:17: class DTLSIdentityObserver { On 2013/06/06 16:22:32, jam wrote: > avoid single-method interfaces, and instead use a callback. this gives more > flexibility for the implementer This will be used in Libjingle which cannot use the Chrome callback types. https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:35: virtual bool OnControlMessageReceived(const IPC::Message& message) OVERRIDE; On 2013/06/06 16:22:33, jam wrote: > nit: put this in the private section Done. https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:37: // Called on the renderer main thread to get the DTLS identity or generate a On 2013/06/06 16:22:33, jam wrote: > nit: by default, code in the renderer runs on the main thread. so no need to > call that out Done. https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:42: const std::string& common_name); On 2013/06/06 16:22:33, jam wrote: > why is this virtual? This class will implement a Libjingle interface which is not added yet. This method will be virtual override.
only skimmed the code outside media. https://codereview.chromium.org/15969025/diff/38001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/38001/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:69: base::Bind(callback, certificate, priveta_key)); typo: priveta_key https://codereview.chromium.org/15969025/diff/38001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/38001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:26: typedef base::Callback<void(const std::string&, const std::string&)> doco params (cert/privatekey) https://codereview.chromium.org/15969025/diff/38001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/38001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:47: Send(new DTLSIdentityHostMsg_IdentityReady(certificate, private_key)); If the renderer is gone by the time this is triggered will Send() crash the browser? (do you need to listen for channel closure and avoid Send()ing afterwards?) https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:39: void DTLSIdentityService::OnIdentityReady(const std::string& certificate, Is it ok that libjingle will ask for the identity on the render thread but get the answer callback on the I/O thread, or do you need to trampoline this callback back to the requesting thread here? https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:17: class DTLSIdentityObserver { Comment this is here only temporarily until it moves into libjingle, where the impl will also live. https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:27: class DTLSIdentityService : public RenderProcessObserver { How is this working without calling RenderThread()::Get()->{Add,Remove}Observer in the ctor/dtor ?? I don't understand how OCMR() is even being called. https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:29: // |observer| is owned by DTLSIdentityService. Passing by scoped_ptr would allow the compiler to check this instead of relying on a comment. https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:37: virtual void GetOrGenerateIdentity(const std::string& identity_name, nit: name & comment still sound synchronous. s/GetOrGenerate/Request/ ?
Comments resolved. PTAL. https://codereview.chromium.org/15969025/diff/38001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/38001/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:69: base::Bind(callback, certificate, priveta_key)); On 2013/06/06 18:28:51, Ami Fischman wrote: > typo: priveta_key Done. https://codereview.chromium.org/15969025/diff/38001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/38001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:26: typedef base::Callback<void(const std::string&, const std::string&)> On 2013/06/06 18:28:51, Ami Fischman wrote: > doco params (cert/privatekey) Done. https://codereview.chromium.org/15969025/diff/38001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/38001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:47: Send(new DTLSIdentityHostMsg_IdentityReady(certificate, private_key)); On 2013/06/06 18:28:51, Ami Fischman wrote: > If the renderer is gone by the time this is triggered will Send() crash the > browser? > (do you need to listen for channel closure and avoid Send()ing afterwards?) BrowserMessageFilter::OnChannelClosing already takes care of that. If send is called after the channel is closed, it will abort since channel_ is null. https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:39: void DTLSIdentityService::OnIdentityReady(const std::string& certificate, On 2013/06/06 18:28:51, Ami Fischman wrote: > Is it ok that libjingle will ask for the identity on the render thread but get > the answer callback on the I/O thread, or do you need to trampoline this > callback back to the requesting thread here? The callback is always called on the render thread as well. https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:17: class DTLSIdentityObserver { On 2013/06/06 18:28:51, Ami Fischman wrote: > Comment this is here only temporarily until it moves into libjingle, where the > impl will also live. Done. https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:17: class DTLSIdentityObserver { On 2013/06/06 18:28:51, Ami Fischman wrote: > Comment this is here only temporarily until it moves into libjingle, where the > impl will also live. Done. https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:27: class DTLSIdentityService : public RenderProcessObserver { On 2013/06/06 18:28:51, Ami Fischman wrote: > How is this working without calling RenderThread()::Get()->{Add,Remove}Observer > in the ctor/dtor ?? > I don't understand how OCMR() is even being called. Oops, added these calls. I'm not sure how to test this in either unittest or browser test. :( https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:29: // |observer| is owned by DTLSIdentityService. On 2013/06/06 18:28:51, Ami Fischman wrote: > Passing by scoped_ptr would allow the compiler to check this instead of relying > on a comment. Done. https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:29: // |observer| is owned by DTLSIdentityService. On 2013/06/06 18:28:51, Ami Fischman wrote: > Passing by scoped_ptr would allow the compiler to check this instead of relying > on a comment. Done. https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:37: virtual void GetOrGenerateIdentity(const std::string& identity_name, On 2013/06/06 18:28:51, Ami Fischman wrote: > nit: name & comment still sound synchronous. > s/GetOrGenerate/Request/ ? Done. https://codereview.chromium.org/15969025/diff/38001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:37: virtual void GetOrGenerateIdentity(const std::string& identity_name, On 2013/06/06 18:28:51, Ami Fischman wrote: > nit: name & comment still sound synchronous. > s/GetOrGenerate/Request/ ? Done.
Do you have this working with a libjingle end-to-end now? (do you plan to land this before that is the case?) https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:25: GetRenderThread()->AddObserver(this); Why not in ctor? https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:13: #include "third_party/libjingle/source/talk/base/scoped_ptr.h" unnecessary see below https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:33: talk_base::scoped_ptr<DTLSIdentityObserver>& observer); the ctor is not part of the interface, so can be pure chrome; i.e. you can use chromium's scoped_ptr instead of introducing talk_base here. https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:45: virtual RenderThread* GetRenderThread(); Comment this is protected for testing. https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:56: talk_base::scoped_ptr<DTLSIdentityObserver> observer_; ditto above https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:57: RenderThread* render_thread_; unused https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service_unittest.cc (right): https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service_unittest.cc:63: EXPECT_EQ(common_name, params.c); The only thing this test tests is that when you request an identity an IPC is crafted. This doesn't seem like a worthwhile test. (for example, the test code is significantly more complicated than the code it is testing) If you do decide to drop this test then you can simplify the non-test code somewhat by dropping GetRenderThread().
One idea for end-to-end testing: add a UMA histogram to the browser with a bucket for "create identity" and a bucket for "reuse existing identity". Test can launch whatever JS is supposed to trigger this code and then inspect histogram values to ensure that the right things happen.
Comments resolved. dtls_identity_service_unittest is removed. PTAL I tested that it works end to end by locally modifying the glue code to request an identity when a peer connection is created. https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:25: GetRenderThread()->AddObserver(this); On 2013/06/06 21:33:04, Ami Fischman wrote: > Why not in ctor? because GetRenderThread was virtual. now removed. https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:13: #include "third_party/libjingle/source/talk/base/scoped_ptr.h" On 2013/06/06 21:33:04, Ami Fischman wrote: > unnecessary see below Done. https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:33: talk_base::scoped_ptr<DTLSIdentityObserver>& observer); On 2013/06/06 21:33:04, Ami Fischman wrote: > the ctor is not part of the interface, so can be pure chrome; i.e. you can use > chromium's scoped_ptr instead of introducing talk_base here. Done. https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:45: virtual RenderThread* GetRenderThread(); On 2013/06/06 21:33:04, Ami Fischman wrote: > Comment this is protected for testing. removed. https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:56: talk_base::scoped_ptr<DTLSIdentityObserver> observer_; On 2013/06/06 21:33:04, Ami Fischman wrote: > ditto above Done. https://codereview.chromium.org/15969025/diff/53001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:57: RenderThread* render_thread_; On 2013/06/06 21:33:04, Ami Fischman wrote: > unused removed
content/renderer/media LGTM https://codereview.chromium.org/15969025/diff/59001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/59001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:22: virtual ~DTLSIdentityObserver() {} clang may complain about inlined dtor (move impl to .cc file)
https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:24: static DTLSIdentityStore* GetInstance(); On 2013/06/05 00:08:09, jiayl wrote: > On 2013/06/04 19:13:52, Ryan Sleevi wrote: > > DESIGN: No new singletons. > > DESIGN: You can't mix the guarantees of a singleton while still having public > > constructors. It's a design contract that cannot be met. > > What's the alternative to singleton? I tried to find a global-ish object to own > this object and didn't find a good place. ServerBoundCertService lives in the IO > thread globals in src/chrome, but it cannot be used in Content. The alternative to a Singleton is not using a Singleton, and creating an instance variable. Minimally, it's a given that this Singleton design will not scale the moment you move to persisting, because persistence is going to be on a per-Profile basis. You need to work through your initialization layers to find an appropriate place to hang this object off of - which is almost certainly going to be on the Profile or related - I think the "new hotness" in content/ is StoragePartition (by way of BrowserContext -> SiteInstance). Either way, this code should not land with a Singleton, and any code that assumes this is a Singleton is making a bad design assumption. https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:41: const OnCompleteCallback& callback On 2013/06/06 05:10:24, jiayl wrote: > On 2013/06/04 19:13:52, Ryan Sleevi wrote: > > DESIGN: If a requester goes away while an identity request is being made, is > > there any way to abort this request? > > How can we tell a requester goes away? It seems the only way is to send another > IPC message saying "cancel the previous request". Is it worth the extra > complexity? Yes. You're going to be performing expensive operations on a fixed worker pool, and you're going to be enqueuing these in order to ensure they remain servicable. You're going to have to consider the situation where something is requested of the browser process, and then goes away. Using WeakPtr with the callback is not sufficient. This is again why I strongly encouraged you to follow the OriginBoundCertService model, which looks at all of the sorts of issues you're going to have to solve for this design as well. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:39: return; BUG: As highlighted in the PostTaskAndReply comment below, this actually makes it impossible for the caller to distinguish when creation fails, because their callback is never invoked (line 66). This will lead to hung state machines. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:49: if (cert == NULL) { if (!cert) { https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:56: cert = NULL; style: Don't include this = NULL - such assignments are seen as superflous. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:65: DLOG(INFO) << "DTLSIdentityStore: a new identity is gnerated."; typo: s/gnerated/generated STYLE: Don't log this. It provides no real actionable information. If you absolutely, positively believe it should be logged (and I think I'll disagree with you if so), then this should be a DVLOG(3) or something otherwise "unchatty". https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:68: base::Bind(callback, certificate, private_key)); DESIGN: Do not structure the PostTask interaction like this. Instead, use PostTaskAndReply/PostTaskAndReplyWithResult to handle the marshalling for ensuring that |callback| is invoked on the right thread. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:75: } Again, no go for Singletons. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:104: callback)); std::string* private_key = new std::string; std::string* cert = new std::string; PostTaskAndReply( task_runner_.get(), FROM_HERE, base::Bind(&GenerateIdentityWorker, origin, identity_name, common_name, private_key, cert), base::Bind(callback, base::Owned(private_key), base::Owned(cert))); https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:53: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/15969025/diff/57002/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/57002/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:24: IPC_END_MESSAGE_MAP_EX() STYLE: Indent the IPC_MESSAGE_[UN]HAN* two spaces, as done everywhere else. https://codereview.chromium.org/15969025/diff/57002/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:35: << "common_name = " << common_name << "."; STYLE: Don't log all this. https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:17: observer_.reset(observer.release()); BUG: As mentioned before, you should be using Pass() You can find more about the right way to use Chromium's smart pointers at http://www.chromium.org/developers/smart-pointer-guidelines https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:29: DLOG(INFO) << "DTLSIdentityService::RequestIdentity"; Unnecessary https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:37: IPC_MESSAGE_UNHANDLED(handled = false) STYLE: Indents https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:44: DLOG(INFO) << "DTLSIdentityService::OnIdentityReady"; Unnecessary https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:19: // TODO (jiayl): Move it to Libjingle and implement it there. It seems like these should happen before continuing this CL. Otherwise, the design anti-patterns here (Observer vs base::Callback<>) make it hard to review. https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:33: scoped_ptr<DTLSIdentityObserver>& observer); BUG: Do not pass this as a non-const reference. Use "scoped_ptr<DTLSIdentityObserver> observer" This *forces* callers to be aware that they're transferring ownership (via Pass() and friends) https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:37: // |identity_name| is used to identity an identity within an origin. s/identity an identity/identify an identity/ https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:39: virtual void RequestIdentity(const std::string& identity_name, COMMENT: Please expand on what these mean within the comment. What is a "DTLS identity", why would there be multiple identities, and what or how is the common name significant. A reader of this header should be able to discern the "right" way to use this class from reading the comments, and these headers rely on too much hidden understanding about how to 'request'.
https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:41: const OnCompleteCallback& callback But ServerBoundCertService does not remove a cancelled job from the queue, but runs the job without calling the callback. On 2013/06/06 23:57:37, Ryan Sleevi wrote: > On 2013/06/06 05:10:24, jiayl wrote: > > On 2013/06/04 19:13:52, Ryan Sleevi wrote: > > > DESIGN: If a requester goes away while an identity request is being made, is > > > there any way to abort this request? > > > > How can we tell a requester goes away? It seems the only way is to send > another > > IPC message saying "cancel the previous request". Is it worth the extra > > complexity? > > Yes. > > You're going to be performing expensive operations on a fixed worker pool, and > you're going to be enqueuing these in order to ensure they remain servicable. > You're going to have to consider the situation where something is requested of > the browser process, and then goes away. > > Using WeakPtr with the callback is not sufficient. > > This is again why I strongly encouraged you to follow the OriginBoundCertService > model, which looks at all of the sorts of issues you're going to have to solve > for this design as well.
https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:24: static DTLSIdentityStore* GetInstance(); jam@, could you give some suggestion on who should own this object in content/browser? On 2013/06/06 23:57:37, Ryan Sleevi wrote: > On 2013/06/05 00:08:09, jiayl wrote: > > On 2013/06/04 19:13:52, Ryan Sleevi wrote: > > > DESIGN: No new singletons. > > > DESIGN: You can't mix the guarantees of a singleton while still having > public > > > constructors. It's a design contract that cannot be met. > > > > What's the alternative to singleton? I tried to find a global-ish object to > own > > this object and didn't find a good place. ServerBoundCertService lives in the > IO > > thread globals in src/chrome, but it cannot be used in Content. > > The alternative to a Singleton is not using a Singleton, and creating an > instance variable. > > Minimally, it's a given that this Singleton design will not scale the moment you > move to persisting, because persistence is going to be on a per-Profile basis. > > You need to work through your initialization layers to find an appropriate place > to hang this object off of - which is almost certainly going to be on the > Profile or related - I think the "new hotness" in content/ is StoragePartition > (by way of BrowserContext -> SiteInstance). > > Either way, this code should not land with a Singleton, and any code that > assumes this is a Singleton is making a bad design assumption.
https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:24: static DTLSIdentityStore* GetInstance(); if this code is per profile, then it should either be per BrowserContext (i.e. content's version of Profile), or StoragePartition. ajwong or creis can tell you which one is appropriate in this case as I've forgotten the nuances. if you put this per BrowserContext, then make this class derive from base::SupportsUserData::Data. you can then attach it to BC since that one derives from SupportsUserData. if you put this per StoragePartition, then you can hang it off StoragePartitionImpl https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... File content/browser/media/dtls_identity_store_browsertest.cc (right): https://codereview.chromium.org/15969025/diff/27001/content/browser/media/dtl... content/browser/media/dtls_identity_store_browsertest.cc:39: host->OnMessageReceived(msg, &ok); On 2013/06/06 17:07:37, jiayl wrote: > On 2013/06/06 16:22:32, jam wrote: > > what is the point of this test? i.e. what failure conditions are you look for? > > > > because it looks like it'll pass even if this method doesn't get called. also > > checking that an IPC handler returns true doesn't tell much, all that means is > > that it dispatched the IPC > > I want to check the IPC handling and DTLSIdentityStore do not throw any error, > like asserting on the wrong thread. > Could the method not get called? Does not the navigation pump the IO thread > tasks? i'm just saying if there was an error that got introduced in this code, and it didn't run, then this test would silently fail. when you say threading, how can it fail? are you testing that BrowserMessageFilter is working? that should be tested by BMF, not every consumer i think this test as written is pretty weak and it's not useful. the unit test seems more appropriate since it actually checks that an operation succeeded https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:17: class DTLSIdentityObserver { On 2013/06/06 17:07:37, jiayl wrote: > On 2013/06/06 16:22:32, jam wrote: > > avoid single-method interfaces, and instead use a callback. this gives more > > flexibility for the implementer > > This will be used in Libjingle which cannot use the Chrome callback types. where exactly will the calling code be? because anything that can access contnet/renderer can access callbacks. https://codereview.chromium.org/15969025/diff/27001/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:42: const std::string& common_name); On 2013/06/06 17:07:37, jiayl wrote: > On 2013/06/06 16:22:33, jam wrote: > > why is this virtual? > This class will implement a Libjingle interface which is not added yet. This > method will be virtual override. ok, when that happens make this class virtual, until then it's confusing to read this
PTAL. All comments resolved: 1. moved the DTLSIdentityStore object into StoragePartition and removed the singleton pattern 2. made it possible to return an error back to the renderer 3. added the ability to cancel a request from the renderer through a new IPC message. 4. added a request id unique per render process to tell apart which response is for which request. The renderer (DTLSIdentityService) assigns a request id for each RequestIdentity call, and DTLSIdentityServiceHost echos the request id back in the response IPC, so DTLSIdentityService can filter it. 5. removed the dependency on _nss files and used the X509Certificate implementations to generate the cert 6. the dependent Libjingle interface has been merged https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/4002/content/browser/media/dtls... content/browser/media/dtls_identity_store.h:41: const OnCompleteCallback& callback OK, now the request is cancelled when the DTLSIdentityService and DTLSIdentityServiceHost go away. Note "cancelled" means the callback will not be called and the backward IPC will not be sent; the cert will still be generated, just like ServerBoundCertService. On 2013/06/07 00:24:07, jiayl wrote: > But ServerBoundCertService does not remove a cancelled job from the queue, but > runs the job without calling the callback. > > On 2013/06/06 23:57:37, Ryan Sleevi wrote: > > On 2013/06/06 05:10:24, jiayl wrote: > > > On 2013/06/04 19:13:52, Ryan Sleevi wrote: > > > > DESIGN: If a requester goes away while an identity request is being made, > is > > > > there any way to abort this request? > > > > > > How can we tell a requester goes away? It seems the only way is to send > > another > > > IPC message saying "cancel the previous request". Is it worth the extra > > > complexity? > > > > Yes. > > > > You're going to be performing expensive operations on a fixed worker pool, and > > you're going to be enqueuing these in order to ensure they remain servicable. > > You're going to have to consider the situation where something is requested of > > the browser process, and then goes away. > > > > Using WeakPtr with the callback is not sufficient. > > > > This is again why I strongly encouraged you to follow the > OriginBoundCertService > > model, which looks at all of the sorts of issues you're going to have to solve > > for this design as well. > https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:39: return; On 2013/06/06 23:57:37, Ryan Sleevi wrote: > BUG: As highlighted in the PostTaskAndReply comment below, this actually makes > it impossible for the caller to distinguish when creation fails, because their > callback is never invoked (line 66). This will lead to hung state machines. Done. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:49: if (cert == NULL) { On 2013/06/06 23:57:37, Ryan Sleevi wrote: > if (!cert) { Done. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:56: cert = NULL; On 2013/06/06 23:57:37, Ryan Sleevi wrote: > style: Don't include this = NULL - such assignments are seen as superflous. Done. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:65: DLOG(INFO) << "DTLSIdentityStore: a new identity is gnerated."; On 2013/06/06 23:57:37, Ryan Sleevi wrote: > typo: s/gnerated/generated > STYLE: Don't log this. It provides no real actionable information. If you > absolutely, positively believe it should be logged (and I think I'll disagree > with you if so), then this should be a DVLOG(3) or something otherwise > "unchatty". Done. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:68: base::Bind(callback, certificate, private_key)); On 2013/06/06 23:57:37, Ryan Sleevi wrote: > DESIGN: Do not structure the PostTask interaction like this. > > Instead, use PostTaskAndReply/PostTaskAndReplyWithResult to handle the > marshalling for ensuring that |callback| is invoked on the right thread. Done. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:75: } On 2013/06/06 23:57:37, Ryan Sleevi wrote: > Again, no go for Singletons. Done. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:104: callback)); On 2013/06/06 23:57:37, Ryan Sleevi wrote: > std::string* private_key = new std::string; > std::string* cert = new std::string; > PostTaskAndReply( > task_runner_.get(), > FROM_HERE, > base::Bind(&GenerateIdentityWorker, origin, identity_name, common_name, > private_key, cert), > base::Bind(callback, base::Owned(private_key), base::Owned(cert))); > Done. https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/57002/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:53: }; On 2013/06/06 23:57:37, Ryan Sleevi wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/15969025/diff/57002/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/57002/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:24: IPC_END_MESSAGE_MAP_EX() On 2013/06/06 23:57:37, Ryan Sleevi wrote: > STYLE: Indent the IPC_MESSAGE_[UN]HAN* two spaces, as done everywhere else. Done. https://codereview.chromium.org/15969025/diff/57002/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:35: << "common_name = " << common_name << "."; On 2013/06/06 23:57:37, Ryan Sleevi wrote: > STYLE: Don't log all this. Done. https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:29: DLOG(INFO) << "DTLSIdentityService::RequestIdentity"; On 2013/06/06 23:57:37, Ryan Sleevi wrote: > Unnecessary Done. https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:37: IPC_MESSAGE_UNHANDLED(handled = false) On 2013/06/06 23:57:37, Ryan Sleevi wrote: > STYLE: Indents Done. https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.cc:44: DLOG(INFO) << "DTLSIdentityService::OnIdentityReady"; On 2013/06/06 23:57:37, Ryan Sleevi wrote: > Unnecessary Done. https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... File content/renderer/media/dtls_identity_service.h (right): https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:37: // |identity_name| is used to identity an identity within an origin. On 2013/06/06 23:57:37, Ryan Sleevi wrote: > s/identity an identity/identify an identity/ Done. https://codereview.chromium.org/15969025/diff/57002/content/renderer/media/dt... content/renderer/media/dtls_identity_service.h:39: virtual void RequestIdentity(const std::string& identity_name, On 2013/06/06 23:57:37, Ryan Sleevi wrote: > COMMENT: Please expand on what these mean within the comment. > > What is a "DTLS identity", why would there be multiple identities, and what or > how is the common name significant. > > A reader of this header should be able to discern the "right" way to use this > class from reading the comments, and these headers rely on too much hidden > understanding about how to 'request'. Done.
https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:22: namespace { STYLE: Linebreak https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:112: DCHECK(request_ == NULL); DCHECK(!request_) https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:22: } // namespace C++ LANGUAGE: You cannot forward declare a type in an unnamed namespace - it's ill-formed, as each TU ends up with its own type. https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:24: class DTLSIdentityStore : public base::RefCounted<DTLSIdentityStore> { DESIGN: Please do not use RefCounting for this class. Provide a clear ownership model for it. https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:26: DTLSIdentityStore(); STYLE: Per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... , this should appear after typedefs and enums (and generally classes) https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:33: class RequestHandle { DESIGN: This would be better served as an opaque handle w/ the DTLSIdentityStore having a CancelRequest, which is a much more dominant pattern. https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:56: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:61: // |identity_name| is used to identity an identity within an origin; s/identity an/identify an/ https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... File content/browser/media/dtls_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store_unittest.cc:39: TestBrowserThread io_thread_; Please see the PSA https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qQiWjlxRcE4/DQRLP... and use TestBrowserThreadBundle https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:43: DCHECK(pending_request_map_.find(request_id) == pending_request_map_.end()); DCHECK_EQ https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:56: DCHECK(pending_request_map_.find(request_id) != pending_request_map_.end()); DCHECK_EQ https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:64: DCHECK(pending_request_map_.find(request_id) != pending_request_map_.end()); DCHECK_EQ https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:48: typedef std::map<int, DTLSIdentityStore::RequestHandle> RequestMap; DESIGN: When you add the DISALLOW_COPY_AND_ASSIGN, this will fail, because you'll realize you've been copying the classes over using the implicit copy constructor. This is important to note, because it means you're actually calling the destructor on all of these requests every time you assign into it. It "works" because the Request being dtor'd is unassigned, but overall the design pattern at issue here with having a class as the value field in a map. https://codereview.chromium.org/15969025/diff/71001/content/common/media/dtls... File content/common/media/dtls_identity_messages.h (right): https://codereview.chromium.org/15969025/diff/71001/content/common/media/dtls... content/common/media/dtls_identity_messages.h:22: int /* request_id */) Should the renderer be the one cancelling, or should the browser be tracking when a request should be cancelled? From a security perspective, I don't think we'd want to trust the renderer to be responsible for cancelling or cleaning up after itself, so I'm not sure where this IPC comes into play.
PTAL https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:22: namespace { On 2013/06/13 22:36:47, Ryan Sleevi wrote: > STYLE: Linebreak Done. https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.cc:112: DCHECK(request_ == NULL); On 2013/06/13 22:36:47, Ryan Sleevi wrote: > DCHECK(!request_) Done. https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:22: } // namespace On 2013/06/13 22:36:47, Ryan Sleevi wrote: > C++ LANGUAGE: You cannot forward declare a type in an unnamed namespace - it's > ill-formed, as each TU ends up with its own type. Done. https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:24: class DTLSIdentityStore : public base::RefCounted<DTLSIdentityStore> { On 2013/06/13 22:36:47, Ryan Sleevi wrote: > DESIGN: Please do not use RefCounting for this class. Provide a clear ownership > model for it. Done. StoragePartitionImpl owns it. https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:26: DTLSIdentityStore(); On 2013/06/13 22:36:47, Ryan Sleevi wrote: > STYLE: Per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... > , this should appear after typedefs and enums (and generally classes) Done. https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:33: class RequestHandle { On 2013/06/13 22:36:47, Ryan Sleevi wrote: > DESIGN: This would be better served as an opaque handle w/ the DTLSIdentityStore > having a CancelRequest, which is a much more dominant pattern. Do you mean make RequestHandler::Cancel private and DTLSIdentityStore::CancelRequest(RequestHandle*) public? https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:56: }; On 2013/06/13 22:36:47, Ryan Sleevi wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:61: // |identity_name| is used to identity an identity within an origin; On 2013/06/13 22:36:47, Ryan Sleevi wrote: > s/identity an/identify an/ Done. https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... File content/browser/media/dtls_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store_unittest.cc:39: TestBrowserThread io_thread_; On 2013/06/13 22:36:47, Ryan Sleevi wrote: > Please see the PSA > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qQiWjlxRcE4/DQRLP... > and use TestBrowserThreadBundle Done. https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:43: DCHECK(pending_request_map_.find(request_id) == pending_request_map_.end()); On 2013/06/13 22:36:47, Ryan Sleevi wrote: > DCHECK_EQ It cannot compile, seemingly due to the use of template. https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:56: DCHECK(pending_request_map_.find(request_id) != pending_request_map_.end()); On 2013/06/13 22:36:47, Ryan Sleevi wrote: > DCHECK_EQ cannot compile https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.cc:64: DCHECK(pending_request_map_.find(request_id) != pending_request_map_.end()); On 2013/06/13 22:36:47, Ryan Sleevi wrote: > DCHECK_EQ cannot compile https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:48: typedef std::map<int, DTLSIdentityStore::RequestHandle> RequestMap; On 2013/06/13 22:36:47, Ryan Sleevi wrote: > DESIGN: When you add the DISALLOW_COPY_AND_ASSIGN, this will fail, because > you'll realize you've been copying the classes over using the implicit copy > constructor. > > This is important to note, because it means you're actually calling the > destructor on all of these requests every time you assign into it. It "works" > because the Request being dtor'd is unassigned, but overall the design pattern > at issue here with having a class as the value field in a map. Changed to store the pointer instead. Note that scoped_ptr will not work in std::map. https://codereview.chromium.org/15969025/diff/71001/content/common/media/dtls... File content/common/media/dtls_identity_messages.h (right): https://codereview.chromium.org/15969025/diff/71001/content/common/media/dtls... content/common/media/dtls_identity_messages.h:22: int /* request_id */) On 2013/06/13 22:36:47, Ryan Sleevi wrote: > Should the renderer be the one cancelling, or should the browser be tracking > when a request should be cancelled? > > From a security perspective, I don't think we'd want to trust the renderer to be > responsible for cancelling or cleaning up after itself, so I'm not sure where > this IPC comes into play. We want to cancel the request to avoid wasting resources on unuseful tasks; so if DTLSIdentityService is going away, it has a chance to cancel the request. The browser does not assume that the request has to be cancelled, and it should work fine if an obsolete request is not cancelled, except wasting some CPU cycles.
https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:33: class RequestHandle { On 2013/06/14 00:37:01, jiayl wrote: > On 2013/06/13 22:36:47, Ryan Sleevi wrote: > > DESIGN: This would be better served as an opaque handle w/ the > DTLSIdentityStore > > having a CancelRequest, which is a much more dominant pattern. > > Do you mean make RequestHandler::Cancel private and > DTLSIdentityStore::CancelRequest(RequestHandle*) public? yeah, that is much better. this whole class (RequestHandle) should just move to the cc file https://codereview.chromium.org/15969025/diff/88001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/88001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:22: class DTLSIdentityStore { nit: comment this class https://codereview.chromium.org/15969025/diff/88001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:29: class RequestHandle { do you really need to expose this in the header? why not hide it from callers and put this class in the cc file, and make the out_request parameter below be a callback that the caller to RequestIdentity can call to cancel? https://codereview.chromium.org/15969025/diff/88001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:76: friend class base::RefCounted<DTLSIdentityStore>; nit: not needed https://codereview.chromium.org/15969025/diff/88001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/88001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:25: virtual ~DTLSIdentityServiceHost(); why is this in protected instead of private? https://codereview.chromium.org/15969025/diff/88001/content/public/browser/st... File content/public/browser/storage_partition.h (right): https://codereview.chromium.org/15969025/diff/88001/content/public/browser/st... content/public/browser/storage_partition.h:59: virtual DTLSIdentityStore* GetDTLSIdentityStore() = 0; no need to modify content api since this is only called from inside content. just keep this on StoragePartitionImpl only
All comments resolved. PTAL. Thanks! https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/71001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:33: class RequestHandle { On 2013/06/14 23:33:13, jam wrote: > On 2013/06/14 00:37:01, jiayl wrote: > > On 2013/06/13 22:36:47, Ryan Sleevi wrote: > > > DESIGN: This would be better served as an opaque handle w/ the > > DTLSIdentityStore > > > having a CancelRequest, which is a much more dominant pattern. > > > > Do you mean make RequestHandler::Cancel private and > > DTLSIdentityStore::CancelRequest(RequestHandle*) public? > > yeah, that is much better. this whole class (RequestHandle) should just move to > the cc file Done. https://codereview.chromium.org/15969025/diff/88001/content/browser/media/dtl... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/88001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:22: class DTLSIdentityStore { On 2013/06/14 23:33:13, jam wrote: > nit: comment this class Done. https://codereview.chromium.org/15969025/diff/88001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:29: class RequestHandle { On 2013/06/14 23:33:13, jam wrote: > do you really need to expose this in the header? why not hide it from callers > and put this class in the cc file, and make the out_request parameter below be a > callback that the caller to RequestIdentity can call to cancel? Done. Now the caller get a Callback returned to cancel a request and the RequestHandle class is moved into the .cc file. https://codereview.chromium.org/15969025/diff/88001/content/browser/media/dtl... content/browser/media/dtls_identity_store.h:76: friend class base::RefCounted<DTLSIdentityStore>; On 2013/06/14 23:33:13, jam wrote: > nit: not needed Done. https://codereview.chromium.org/15969025/diff/88001/content/browser/renderer_... File content/browser/renderer_host/media/dtls_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/88001/content/browser/renderer_... content/browser/renderer_host/media/dtls_identity_service_host.h:25: virtual ~DTLSIdentityServiceHost(); On 2013/06/14 23:33:13, jam wrote: > why is this in protected instead of private? Changed to private. https://codereview.chromium.org/15969025/diff/88001/content/public/browser/st... File content/public/browser/storage_partition.h (right): https://codereview.chromium.org/15969025/diff/88001/content/public/browser/st... content/public/browser/storage_partition.h:59: virtual DTLSIdentityStore* GetDTLSIdentityStore() = 0; On 2013/06/14 23:33:13, jam wrote: > no need to modify content api since this is only called from inside content. > just keep this on StoragePartitionImpl only Done.
https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:110: store_(store), request_(NULL), callback_(callback) { STYLE: Please see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... about the two acceptable forms of constructor initializer lists. https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:119: if (request_) { Try to handle errors immediately, to reduce the level of indentation and complexity if (!request_) return; callback_.Reset(); .... https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:130: DCHECK_NE(static_cast<DTLSIdentityRequest*>(NULL), request); DCHECK(request) https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:169: task_runner_->PostTaskAndReply( SECURITY BUG: If this fails to Post, then |request| will be deleted. When |request| is deleted, it will run |callback_|'s destructor (line 98), which is the callback setup on line 164-165. |callback_| owns |handle| (line 165), which will then cause a U-A-F if the caller attempts to Cancel the request (line 175-176) https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.h:10: #include "base/threading/non_thread_safe.h" Unused https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.h:51: explicit DTLSIdentityStore( Why is this protected, when this class has no virtual methods (beyond its Destructor)?
PTAL https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:110: store_(store), request_(NULL), callback_(callback) { On 2013/06/17 18:13:23, Ryan Sleevi wrote: > STYLE: Please see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... > about the two acceptable forms of constructor initializer lists. Done. https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:119: if (request_) { On 2013/06/17 18:13:23, Ryan Sleevi wrote: > Try to handle errors immediately, to reduce the level of indentation and > complexity > > if (!request_) > return; > > callback_.Reset(); > .... Done. https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:130: DCHECK_NE(static_cast<DTLSIdentityRequest*>(NULL), request); On 2013/06/17 18:13:23, Ryan Sleevi wrote: > DCHECK(request) Done. https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:169: task_runner_->PostTaskAndReply( Fixed now by changing the return type to boolean and making the Closure an out param. If it fails to post task, we do not set Closure. On 2013/06/17 18:13:23, Ryan Sleevi wrote: > SECURITY BUG: > If this fails to Post, then |request| will be deleted. > > When |request| is deleted, it will run |callback_|'s destructor (line 98), which > is the callback setup on line 164-165. > > |callback_| owns |handle| (line 165), which will then cause a U-A-F if the > caller attempts to Cancel the request (line 175-176) https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.h:10: #include "base/threading/non_thread_safe.h" On 2013/06/17 18:13:23, Ryan Sleevi wrote: > Unused Removed https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.h:51: explicit DTLSIdentityStore( On 2013/06/17 18:13:23, Ryan Sleevi wrote: > Why is this protected, when this class has no virtual methods (beyond its > Destructor)? It's used in the unit test to inject the TaskRunner
https://codereview.chromium.org/15969025/diff/119001/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/119001/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:105: class DTLSIdentityRequestHandle { I don't understand why you need this class and DTLSIdentityRequest? why isn't the former enough? https://codereview.chromium.org/15969025/diff/119001/content/public/browser/s... File content/public/browser/storage_partition.h (right): https://codereview.chromium.org/15969025/diff/119001/content/public/browser/s... content/public/browser/storage_partition.h:40: class DTLSIdentityStore; not needed
PTAL. Thanks! https://codereview.chromium.org/15969025/diff/119001/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/119001/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:105: class DTLSIdentityRequestHandle { On 2013/06/17 21:36:23, jam wrote: > I don't understand why you need this class and DTLSIdentityRequest? why isn't > the former enough? The separation of Request and RequestHandle gives us more flexibility for future changes, for example, when the persistent store is in place, DTLSIdentityStore will join multiple external identical requests into one internal request through a Callback list in DTLSIdentityRequest. https://codereview.chromium.org/15969025/diff/119001/content/public/browser/s... File content/public/browser/storage_partition.h (right): https://codereview.chromium.org/15969025/diff/119001/content/public/browser/s... content/public/browser/storage_partition.h:40: class DTLSIdentityStore; On 2013/06/17 21:36:23, jam wrote: > not needed Done.
https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/102002/content/browser/media/dt... content/browser/media/dtls_identity_store.h:51: explicit DTLSIdentityStore( On 2013/06/17 18:58:00, jiayl wrote: > On 2013/06/17 18:13:23, Ryan Sleevi wrote: > > Why is this protected, when this class has no virtual methods (beyond its > > Destructor)? > It's used in the unit test to inject the TaskRunner It doesn't need to be protected though - it should be private, and you should friend the unit test fixture. "protected" is a signal you intend for your (public) API to allow specializations of this class, and you don't. https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:38: scoped_ptr<crypto::RSAPrivateKey> key(crypto::RSAPrivateKey::Create(1024)); DESIGN BUG: This forces WebRTC keys into the TPM on ChromeOS, or into the persistent OS store on Linux/Mac, which is specifically *not* desirable (we desire them to be ephemeral, in-memory keys, persisted to a temporary storage). This is because RSAPrivateKey attempts to store into the private key slot (TPM/persistent storage) or the Keychain when it's executed in the Browser process. The Server Bound Cert Service code doesn't have this problem, because it always uses EC keys, which are always keyed against the public key slot (ephemeral/in-memory), and always use NSS (eg: not platform APIs). There is no way with RSAPrivateKey to indicate that an ephemeral key is being generated. https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:177: if (posted && canceller) { This check indicates that the caller need not supply a valid pointer for canceller. This creates a confusing API. It would be better to universally require that the caller supply a valid pointer. https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.h:35: // |origin| is the origin of the PeerConnection requesting the identity; comment: Your design indicates you're coupling this to "DTLS", except this comment here indicates it's specific to PeerConnection. Perhaps these classes should be named "WebRTCIdentityStore" ? That is, the DTLS seems to be a lower-level detail of the WebRTC implementation, and you're really trying to implement something for WebRTC/PeerConnection. https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.h:37: // |common_name| is the common name used to generate the certificate; comment nit: Please provide clearer indication about these parameters. Namely that |identity_name| remains private to the client, whereas |common_name| will be shared with the peer. You should also expand upon the comment to indicate whether or not multiple certificates can share the same common_name but differing identity names (presumably, yes), and whether the same identity name can share multiple common names (presumably, no). https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.h:39: // |canceller| will be set to the Closure used to cancel the request if the naming nit: canceller -> cancel_callback canceller is an awkward name. https://codereview.chromium.org/15969025/diff/118002/content/browser/renderer... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/118002/content/browser/renderer... content/browser/renderer_host/media/dtls_identity_service_host.cc:49: RequestIdentity( style: Seems this wrapping is over-eager. Perhaps a clang-format bug? https://codereview.chromium.org/15969025/diff/118002/content/browser/renderer... File content/browser/renderer_host/media/dtls_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/118002/content/browser/renderer... content/browser/renderer_host/media/dtls_identity_service_host.h:41: virtual void OnComplete(int request_id, DESIGN: Why is this virtual?
https://codereview.chromium.org/15969025/diff/119001/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/119001/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:105: class DTLSIdentityRequestHandle { On 2013/06/17 22:16:17, jiayl wrote: > On 2013/06/17 21:36:23, jam wrote: > > I don't understand why you need this class and DTLSIdentityRequest? why isn't > > the former enough? > The separation of Request and RequestHandle gives us more flexibility for future > changes, for example, when the persistent store is in place, DTLSIdentityStore > will join multiple external identical requests into one internal request through > a Callback list in DTLSIdentityRequest. i see, ok i was wondering if that was being done because of lifetime issues which could be solved by other methods, but looks like that's not the case. so i'll defer to media owners https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:151: : task_runner_(base::WorkerPool::GetTaskRunner(true)) {} why are you using base::WorkerPool instead of SequencedWorkerPool?
All comments addressed, except for the persistent private key slot one. https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:151: : task_runner_(base::WorkerPool::GetTaskRunner(true)) {} On 2013/06/18 00:03:18, jam wrote: > why are you using base::WorkerPool instead of SequencedWorkerPool? It's not necessary to enforce the an order on the tasks, i.e. identities for different origin/name can be created in any order. So I didn't use SequcencedWorkerPool. https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:177: if (posted && canceller) { On 2013/06/17 23:08:34, Ryan Sleevi wrote: > This check indicates that the caller need not supply a valid pointer for > canceller. > > This creates a confusing API. It would be better to universally require that the > caller supply a valid pointer. Done. https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.h:35: // |origin| is the origin of the PeerConnection requesting the identity; This class does not really rely on any WebRTC feature. I changed the description of origin to "origin of the DTLS connection" instead. On 2013/06/17 23:08:34, Ryan Sleevi wrote: > comment: Your design indicates you're coupling this to "DTLS", except this > comment here indicates it's specific to PeerConnection. > > Perhaps these classes should be named "WebRTCIdentityStore" ? That is, the DTLS > seems to be a lower-level detail of the WebRTC implementation, and you're really > trying to implement something for WebRTC/PeerConnection. https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.h:37: // |common_name| is the common name used to generate the certificate; I improved the comments. On 2013/06/17 23:08:34, Ryan Sleevi wrote: > comment nit: Please provide clearer indication about these parameters. Namely > that |identity_name| remains private to the client, whereas |common_name| will > be shared with the peer. > > You should also expand upon the comment to indicate whether or not multiple > certificates can share the same common_name but differing identity names > (presumably, yes), and whether the same identity name can share multiple common > names (presumably, no). https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.h:39: // |canceller| will be set to the Closure used to cancel the request if the On 2013/06/17 23:08:34, Ryan Sleevi wrote: > naming nit: canceller -> cancel_callback > > canceller is an awkward name. Done. https://codereview.chromium.org/15969025/diff/118002/content/browser/renderer... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/118002/content/browser/renderer... content/browser/renderer_host/media/dtls_identity_service_host.cc:49: RequestIdentity( On 2013/06/17 23:08:34, Ryan Sleevi wrote: > style: Seems this wrapping is over-eager. Perhaps a clang-format bug? Done. https://codereview.chromium.org/15969025/diff/118002/content/browser/renderer... File content/browser/renderer_host/media/dtls_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/118002/content/browser/renderer... content/browser/renderer_host/media/dtls_identity_service_host.h:41: virtual void OnComplete(int request_id, Removed. On 2013/06/17 23:08:34, Ryan Sleevi wrote: > DESIGN: Why is this virtual?
https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:151: : task_runner_(base::WorkerPool::GetTaskRunner(true)) {} On 2013/06/18 01:07:55, jiayl wrote: > On 2013/06/18 00:03:18, jam wrote: > > why are you using base::WorkerPool instead of SequencedWorkerPool? > It's not necessary to enforce the an order on the tasks, i.e. identities for > different origin/name can be created in any order. So I didn't use > SequcencedWorkerPool. sequencing is not the only reason to use sequencedworkerpool. i.e. one can use it to post random tasks. how slow is this operation?
On 2013/06/18 23:22:45, jam wrote: > https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... > File content/browser/media/dtls_identity_store.cc (right): > > https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... > content/browser/media/dtls_identity_store.cc:151: : > task_runner_(base::WorkerPool::GetTaskRunner(true)) {} > On 2013/06/18 01:07:55, jiayl wrote: > > On 2013/06/18 00:03:18, jam wrote: > > > why are you using base::WorkerPool instead of SequencedWorkerPool? > > It's not necessary to enforce the an order on the tasks, i.e. identities for > > different origin/name can be created in any order. So I didn't use > > SequcencedWorkerPool. > > sequencing is not the only reason to use sequencedworkerpool. i.e. one can use > it to post random tasks. how slow is this operation? What are the other benefits? It's around 200ms for one request on my Linux work station.
On 2013/06/18 23:22:45, jam wrote: > https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... > File content/browser/media/dtls_identity_store.cc (right): > > https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... > content/browser/media/dtls_identity_store.cc:151: : > task_runner_(base::WorkerPool::GetTaskRunner(true)) {} > On 2013/06/18 01:07:55, jiayl wrote: > > On 2013/06/18 00:03:18, jam wrote: > > > why are you using base::WorkerPool instead of SequencedWorkerPool? > > It's not necessary to enforce the an order on the tasks, i.e. identities for > > different origin/name can be created in any order. So I didn't use > > SequcencedWorkerPool. > > sequencing is not the only reason to use sequencedworkerpool. i.e. one can use > it to post random tasks. how slow is this operation? jam: The use of base::WorkerPool here is consistent with how we've treated other crypto operations, due to the fact that depending on the underlying implementation, these operations may block indefinitely (eg: for user input). I agree, however, that if we can get this to using a comparable solution to what's being done in the renderer (eg: it's entirely in memory, with no dependency on the underlying platform/sensitive operations), then it's perfectly suitable to run on any arbitrary SequencedTaskRunner, rather than the WorkerPool directly.
PTAL https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:38: scoped_ptr<crypto::RSAPrivateKey> key(crypto::RSAPrivateKey::Create(1024)); Since Ryan has started two other CLs to address this issue for all RSAPrivateKey consumers, I think we can unblock this CL. Ryan's patches: Patch 1: https://codereview.chromium.org/17265013/ Patch 2: https://codereview.chromium.org/17447009/ On 2013/06/17 23:08:34, Ryan Sleevi wrote: > DESIGN BUG: This forces WebRTC keys into the TPM on ChromeOS, or into the > persistent OS store on Linux/Mac, which is specifically *not* desirable (we > desire them to be ephemeral, in-memory keys, persisted to a temporary storage). > > This is because RSAPrivateKey attempts to store into the private key slot > (TPM/persistent storage) or the Keychain when it's executed in the Browser > process. > > The Server Bound Cert Service code doesn't have this problem, because it always > uses EC keys, which are always keyed against the public key slot > (ephemeral/in-memory), and always use NSS (eg: not platform APIs). > > There is no way with RSAPrivateKey to indicate that an ephemeral key is being > generated.
John&Ryan, Could you take another look? On Wed, Jun 19, 2013 at 6:04 PM, <jiayl@chromium.org> wrote: > PTAL > > > > https://codereview.chromium.**org/15969025/diff/118002/** > content/browser/media/dtls_**identity_store.cc<https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dtls_identity_store.cc> > File content/browser/media/dtls_**identity_store.cc (right): > > https://codereview.chromium.**org/15969025/diff/118002/** > content/browser/media/dtls_**identity_store.cc#newcode38<https://codereview.chromium.org/15969025/diff/118002/content/browser/media/dtls_identity_store.cc#newcode38> > content/browser/media/dtls_**identity_store.cc:38: > scoped_ptr<crypto::**RSAPrivateKey> > key(crypto::RSAPrivateKey::**Create(1024)); > Since Ryan has started two other CLs to address this issue for all > RSAPrivateKey consumers, I think we can unblock this CL. > > Ryan's patches: > > Patch 1: https://codereview.chromium.**org/17265013/<https://codereview.chromium.org/1... > Patch 2: https://codereview.chromium.**org/17447009/<https://codereview.chromium.org/1... > > On 2013/06/17 23:08:34, Ryan Sleevi wrote: > >> DESIGN BUG: This forces WebRTC keys into the TPM on ChromeOS, or into >> > the > >> persistent OS store on Linux/Mac, which is specifically *not* >> > desirable (we > >> desire them to be ephemeral, in-memory keys, persisted to a temporary >> > storage). > > This is because RSAPrivateKey attempts to store into the private key >> > slot > >> (TPM/persistent storage) or the Keychain when it's executed in the >> > Browser > >> process. >> > > The Server Bound Cert Service code doesn't have this problem, because >> > it always > >> uses EC keys, which are always keyed against the public key slot >> (ephemeral/in-memory), and always use NSS (eg: not platform APIs). >> > > There is no way with RSAPrivateKey to indicate that an ephemeral key >> > is being > >> generated. >> > > https://codereview.chromium.**org/15969025/<https://codereview.chromium.org/1... >
lgtm for code outside the media directories, for those i defer to their respective owners
content/browser/renderer_host/render_process_host_impl.cc LGTM
I am very concerned about the security of the IPCs. Please review my comments below, in particular with respect to the 'hostile renderer' threat model and how your implementation has placed the security checks in the renderer process, rather than the browser. https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:108: : store_(store), request_(NULL), callback_(callback) {} Was this generated with clang-format? My understanding is that when the constructor signature+initializer list exceeds more than one line, the initializer list should be one-variable-per-line, as per the only two examples given at http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:172: style nit: Chromium style generally handles the error and short circuits, to avoid indentation if (!posted) return false; *cancel_callback = ... return true; Or even better: if (!task_runner_->PoasTaskAndReply(...)) { return false; } *cancel_callback = ... return true; https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.h:24: class DTLSIdentityStore { I'm still concerned about the naming here, in that "DTLS" is a very broad space, particularly within content. If there was content::media:: namespace, I suppose this would be less of a concern - but I would rather see a more specific name (eg: reflecting its purpose) rather than something generic like "DTLS", since this is intended to only be used in a very specific context (WebRTC) https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.h:32: virtual ~DTLSIdentityStore(); unnecessary virtual dtor. https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.h:37: // identity's certificate. From reading this comment, I'm still not sure I understand your model of handling conflicts. If I make two requests for identity "a", for site "example.com", but with CN "Foo" and "Bar", what is reflected in the underlying storage? 1) Two unique private keys, "a-foo" and "a-bar", with two unique certificates? 2) One unique private key "a", with two unique certificates ("a-foo" and "a-bar") 3) One unique private key and certificate generated for "a-foo", and then replaced (eg: a-foo fully destroyed) by "a-bar" It should be abundantly clear from reading this header what the contract is, and there should be unittests to ensure this contract. https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.h:39: // Asynchronous. "// Asynchronous" is unnecessary, and it's also not consistent with the remark on line 53. https://codereview.chromium.org/15969025/diff/140001/content/browser/renderer... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/140001/content/browser/renderer... content/browser/renderer_host/media/dtls_identity_service_host.cc:66: } It still seems to me that you're trusting the renderer to be well behaved and cancel requests. It seems like you should be observing if the renderer process disappears while requests are pending, so that you cancel requests. I'm also concerned about rate limiting - this gives the renderer a vector to spawn an arbitrary number of threads
Further SECURITY concerns: https://codereview.chromium.org/15969025/diff/140001/content/browser/renderer... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/140001/content/browser/renderer... content/browser/renderer_host/media/dtls_identity_service_host.cc:56: pending_request_cancel_callback_map_[request_id] = cancel_callback; As mentioned via e-mail, a hostile renderer could spam requests for the same request_id repeatedly. This would replace the cancel callbacks, allowing them to request an arbitrary number of requests that the browser would be unable to cancel (because the cancel callbacks were overwritten) https://codereview.chromium.org/15969025/diff/140001/content/browser/renderer... content/browser/renderer_host/media/dtls_identity_service_host.cc:64: pending_request_cancel_callback_map_[request_id].Run(); If |request_id| is not a valid request, this will cause the browser to crash, by attempting to run a null callback.
The new patch resolves the security concerns with rate limiting in IdentityServiceHost with tests and early returning on invalid request_id, and other style issues. PTAL. Thanks! https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... File content/browser/media/dtls_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:108: : store_(store), request_(NULL), callback_(callback) {} This is generated from git-cl format, which calls clang-format. On 2013/06/24 19:29:16, Ryan Sleevi wrote: > Was this generated with clang-format? > > My understanding is that when the constructor signature+initializer list exceeds > more than one line, the initializer list should be one-variable-per-line, as per > the only two examples given at > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.cc:172: On 2013/06/24 19:29:16, Ryan Sleevi wrote: > style nit: Chromium style generally handles the error and short circuits, to > avoid indentation > if (!posted) > return false; > > *cancel_callback = ... > return true; > > Or even better: > > if (!task_runner_->PoasTaskAndReply(...)) { > return false; > } > > *cancel_callback = ... > return true; Done. https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... File content/browser/media/dtls_identity_store.h (right): https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.h:24: class DTLSIdentityStore { Renamed to WebRTCIdentity* On 2013/06/24 19:29:16, Ryan Sleevi wrote: > I'm still concerned about the naming here, in that "DTLS" is a very broad space, > particularly within content. > > If there was content::media:: namespace, I suppose this would be less of a > concern - but I would rather see a more specific name (eg: reflecting its > purpose) rather than something generic like "DTLS", since this is intended to > only be used in a very specific context (WebRTC) https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.h:32: virtual ~DTLSIdentityStore(); On 2013/06/24 19:29:16, Ryan Sleevi wrote: > unnecessary virtual dtor. Done. https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.h:37: // identity's certificate. On 2013/06/24 19:29:16, Ryan Sleevi wrote: > From reading this comment, I'm still not sure I understand your model of > handling conflicts. > > If I make two requests for identity "a", for site "example.com", but with CN > "Foo" and "Bar", what is reflected in the underlying storage? > > 1) Two unique private keys, "a-foo" and "a-bar", with two unique certificates? > 2) One unique private key "a", with two unique certificates ("a-foo" and > "a-bar") > 3) One unique private key and certificate generated for "a-foo", and then > replaced (eg: a-foo fully destroyed) by "a-bar" > > It should be abundantly clear from reading this header what the contract is, and > there should be unittests to ensure this contract. Done. https://codereview.chromium.org/15969025/diff/140001/content/browser/media/dt... content/browser/media/dtls_identity_store.h:39: // Asynchronous. On 2013/06/24 19:29:16, Ryan Sleevi wrote: > "// Asynchronous" is unnecessary, and it's also not consistent with the remark > on line 53. Done. https://codereview.chromium.org/15969025/diff/140001/content/browser/renderer... File content/browser/renderer_host/media/dtls_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/140001/content/browser/renderer... content/browser/renderer_host/media/dtls_identity_service_host.cc:56: pending_request_cancel_callback_map_[request_id] = cancel_callback; Changed the DCHECK to if-return. On 2013/06/24 23:27:07, Ryan Sleevi wrote: > As mentioned via e-mail, a hostile renderer could spam requests for the same > request_id repeatedly. > > This would replace the cancel callbacks, allowing them to request an arbitrary > number of requests that the browser would be unable to cancel (because the > cancel callbacks were overwritten) https://codereview.chromium.org/15969025/diff/140001/content/browser/renderer... content/browser/renderer_host/media/dtls_identity_service_host.cc:64: pending_request_cancel_callback_map_[request_id].Run(); Changed the DCHECK to if-return On 2013/06/24 23:27:07, Ryan Sleevi wrote: > If |request_id| is not a valid request, this will cause the browser to crash, by > attempting to run a null callback. https://codereview.chromium.org/15969025/diff/140001/content/browser/renderer... content/browser/renderer_host/media/dtls_identity_service_host.cc:66: } Rate limiting is added to WebRTCIdentityServiceHost with max 5 requests per second. When the renderer process disappears, WebRTCIdentityServiceHost detor will cancel requests. On 2013/06/24 19:29:16, Ryan Sleevi wrote: > It still seems to me that you're trusting the renderer to be well behaved and > cancel requests. > > It seems like you should be observing if the renderer process disappears while > requests are pending, so that you cancel requests. > > I'm also concerned about rate limiting - this gives the renderer a vector to > spawn an arbitrary number of threads
https://codereview.chromium.org/15969025/diff/156001/content/browser/media/we... File content/browser/media/webrtc_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/156001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:32: message_was_ok); Why OVERRIDE this, when you're just forwarding to the parent implementation? https://codereview.chromium.org/15969025/diff/156001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:45: DCHECK(request_result_map_.find(request_id) != request_result_map_.end()); Don't use DCHECK in tests - it will crash the harness. https://codereview.chromium.org/15969025/diff/156001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:99: request_id, origin, identity_name, common_name), &message_was_ok); This seems like you're actually going to kick off multiple generation requests, which will run quite slow under valgrind/asan/tsan. You should also mock the store, to return dummy results, rather than actually generating legitimate certificates. This will also help you better ensure constant time execution (eg: within the 1 second) https://codereview.chromium.org/15969025/diff/156001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:106: request_id ++) { style: eliminate extra whitespace https://codereview.chromium.org/15969025/diff/156001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:146: } This form of testing (effectively sleep(1)) is strongly discouraged. Instead, using a time service you can mock/lie with is recommended. https://codereview.chromium.org/15969025/diff/156001/content/browser/renderer... File content/browser/renderer_host/media/webrtc_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/156001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.cc:101: base::Time now = base::Time::NowFromSystemTime(); If you continue to want a time, do not use base::Time - use base::TimeTicks, which is a monotonic clock. https://codereview.chromium.org/15969025/diff/156001/content/browser/renderer... File content/browser/renderer_host/media/webrtc_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/156001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.h:32: static const int MAX_REQUESTS_PER_SECOND; This is going to have issues compiling across different compilers (MSVC and GCC don't play nice on this). Can you not just allow one outstanding request per renderer? The inherent PostTask involved with IPC will act as an effective rate limiter, by allowing a fair scheduling, rather than allowing the renderer to enqueue an arbitrary number of requests and serving them all.
All comments resolved. Now only one pending request is allowed per renderer. I removed the tests for WebRTCIdentityServiceHost since it's much simpler now and it will need more code to mock the dependencies than the code being tested.
PTAL. Thanks!
lgtm https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... File content/browser/media/webrtc_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:131: void OnRequestComplete(int error, nit: newline https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... File content/browser/media/webrtc_identity_store.h (right): https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... content/browser/media/webrtc_identity_store.h:36: // |common_name| if such identity does not exist. s/such/such an/ https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... content/browser/media/webrtc_identity_store.h:45: // presetnt in the certificate; s/presetnt/present/ https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... File content/browser/media/webrtc_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:22: class WebRTCIdentityStoreTest : public testing::Test { Place these tests in an unnamed namespace. https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:36: } Unneeded https://codereview.chromium.org/15969025/diff/178001/content/browser/renderer... File content/browser/renderer_host/media/webrtc_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/178001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.h:39: virtual void OnComplete(int request_id, This doesn't need to be virtual https://codereview.chromium.org/15969025/diff/178001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.h:53: // Cancels a pending request by its id. If there is no pending request having nit: newline https://codereview.chromium.org/15969025/diff/178001/content/renderer/media/w... File content/renderer/media/webrtc_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/178001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.cc:29: static int s_next_request_id = 0; Are you sure a static is appropriate here? Seems like an instance member should be used. Statics are almost always a red flag, much like singletons. https://codereview.chromium.org/15969025/diff/178001/content/renderer/media/w... File content/renderer/media/webrtc_identity_service.h (right): https://codereview.chromium.org/15969025/diff/178001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.h:11: #include "base/memory/scoped_ptr.h" Is this actually needed? Doesn't seem like it.
Comments resolved. Thanks! https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... File content/browser/media/webrtc_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:131: void OnRequestComplete(int error, On 2013/06/27 18:05:24, Ryan Sleevi wrote: > nit: newline Done. https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... File content/browser/media/webrtc_identity_store.h (right): https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... content/browser/media/webrtc_identity_store.h:36: // |common_name| if such identity does not exist. On 2013/06/27 18:05:24, Ryan Sleevi wrote: > s/such/such an/ Done. https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... content/browser/media/webrtc_identity_store.h:45: // presetnt in the certificate; On 2013/06/27 18:05:24, Ryan Sleevi wrote: > s/presetnt/present/ Done. https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... File content/browser/media/webrtc_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:22: class WebRTCIdentityStoreTest : public testing::Test { On 2013/06/27 18:05:24, Ryan Sleevi wrote: > Place these tests in an unnamed namespace. Class WebRTCIdentityStoreTest needs to be forward declared in WebRTCIdentityStore, so cannot be in unnamed namespace. The rest of the file is in unnamed namespace now. https://codereview.chromium.org/15969025/diff/178001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:36: } On 2013/06/27 18:05:24, Ryan Sleevi wrote: > Unneeded Done. https://codereview.chromium.org/15969025/diff/178001/content/browser/renderer... File content/browser/renderer_host/media/webrtc_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/178001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.h:39: virtual void OnComplete(int request_id, On 2013/06/27 18:05:24, Ryan Sleevi wrote: > This doesn't need to be virtual Done. https://codereview.chromium.org/15969025/diff/178001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.h:53: // Cancels a pending request by its id. If there is no pending request having On 2013/06/27 18:05:24, Ryan Sleevi wrote: > nit: newline Done. https://codereview.chromium.org/15969025/diff/178001/content/renderer/media/w... File content/renderer/media/webrtc_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/178001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.cc:29: static int s_next_request_id = 0; On 2013/06/27 18:05:24, Ryan Sleevi wrote: > Are you sure a static is appropriate here? > > Seems like an instance member should be used. Statics are almost always a red > flag, much like singletons. WebRTCIdentityService is per peer connection, so multiple instances can run in one renderer. The static is for making sure the request id is unique across WebRTCIdentityService instances. https://codereview.chromium.org/15969025/diff/178001/content/renderer/media/w... File content/renderer/media/webrtc_identity_service.h (right): https://codereview.chromium.org/15969025/diff/178001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.h:11: #include "base/memory/scoped_ptr.h" On 2013/06/27 18:05:24, Ryan Sleevi wrote: > Is this actually needed? Doesn't seem like it. Done.
https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... File content/browser/media/webrtc_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:22: namespace { this isn't useful (and makes debug tooling sadder) (media code generally prefers file-static to anonymous namespace, and the style guide allows either; the struct is sufficiently specifically named that collision within the content namespace seems unlikely) https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:33: std::string certificate; unused https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:34: std::vector<uint8> private_key_info; can move to just before first use (l.62). https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:85: friend class WebRTCIdentityStore; This is a strange combination of lines considering this whole class is only known to this (impl) file. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:102: // WebRTCIdentityRequest. I'm confused as to why this class exists. Why not simply bind Request's Cancel for the cancel_callback in RequestIdentity? https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:122: // "this" will be deleted after the following call, becuase "this" is typo: becuase https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:124: store_->CancelRequestInternal(request); Why go through store_ for this instead of simply calling request->Cancel(); ? (you'd have to make WRIRequest::Cancel public in that class, of course) https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:157: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); I think it is true that all the methods in this class other than GenerateIdentityWorker want to have this DCHECK, but most don't. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:166: WebRTCIdentityRequestResult* result = new WebRTCIdentityRequestResult; nit: always include the ()'s on a new expression. There are semantic differences (initialization style) between including them and excluding them for some new expressions, though not here, but for consistency it's best to always include them https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:177: return true; fwiw: since this method returns true iff *cancel_callback is set, could as well return the Cancel closure (or Closure()) and have the caller test the return value for is_null() to decide whether the request has been kicked off. The (tiny) advantage of doing this is that it makes the caller more likely to examine the return value (as opposed to caller incorrectly forgetting to examine the return value because it is focused on the output parameter). Some examples of such forgetfulness are in the unittest for this class ;) https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... File content/browser/media/webrtc_identity_store.h (right): https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.h:66: explicit WebRTCIdentityStore( Is this ctor only used for testing? If so, might be useful to mark it as such in a comment, or, better yet, have an explicit private void SetTaskRunnerForTest(const scoped_refptr<base::TaskRunner>& task_runner); method that modifies an already-constructed object. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... File content/browser/media/webrtc_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:27: new base::SequencedWorkerPool(3, "ServerBoundCertServiceTest")) { "ServerBoundCertServiceTest" is copy/pasta? https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:40: namespace { nit: prefer to make ORC below static and the TEST_F's don't need to be in an anonymous namespace. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:53: scoped_ptr<bool> completed(new bool(false)); why not just bool completed = false; and pass &completed to the Bind below? https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:55: webrtc_identity_store_->RequestIdentity( ASSERT_TRUE? https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:86: webrtc_identity_store_->RequestIdentity( ditto ASSERT_TRUE https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:93: webrtc_identity_store_->RequestIdentity( ditto ASSERT_TRUE https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... File content/browser/renderer_host/media/webrtc_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.cc:16: : pending_request_id_(-1), identity_store_(identity_store) {} This assumes -1 is not a valid request_id. Should the IPC say so? https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.cc:40: if (!cancel_callback_.is_null()) { || pending_request_id_ != -1 ? (put another way, CHECK((pending_request_id_ == -1) == cancel_callback_.is_null()); ) https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.cc:65: cancel_callback_.Reset(); base::ResetAndRun(&cancel_callback_); ? https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.cc:73: DCHECK_EQ(request_id, pending_request_id_); Why does pending_request_id_ exist? If it's to safeguard against this very DCHECK condition, then it should probably be a CHECK or have explicit error handling even in non-debug builds. If it's not for that then I think you can drop it and simply use the is_null() status of cancel_callback_ to know whether a request is pending. https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... File content/browser/renderer_host/media/webrtc_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.h:25: // Only one outstanding request is allowed at a time. If a second request is s/is allowed/is allowed per renderer/ ? https://codereview.chromium.org/15969025/diff/188001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/15969025/diff/188001/content/content_tests.gy... content/content_tests.gypi:801: 'browser/media/audio_browsertest.cc', Not sure what this is doing here. https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... File content/renderer/media/webrtc_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.cc:13: : origin_(origin), pending_observer_(NULL), pending_request_id_(0) { s/0/-1/ to match browser side? https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.cc:29: static int s_next_request_id = 0; This is not thread-safe. You might want base::AtomicSequenceNumber. https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.cc:67: pending_request_id_ = 0; ditto s/0/-1/ https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.cc:75: pending_request_id_ = 0; ditto s/0/-1/ https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... File content/renderer/media/webrtc_identity_service.h (right): https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.h:22: WebRTCIdentityService(const GURL& origin); explicit https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.h:28: webrtc::DTLSIdentityRequestObserver* observer) why does this interface pass a raw pointer to a refcounted object?
PTAL. Thanks!! https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... File content/browser/media/webrtc_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:85: friend class WebRTCIdentityStore; On 2013/06/27 20:05:18, Ami Fischman wrote: > This is a strange combination of lines considering this whole class is only > known to this (impl) file. This is to make sure WebRTCIdentityRequestHandle cannot access the methods that should only be called from WebRTCIdentityStore. If you think it's unnecessary, I can remove it and make all methods public. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:102: // WebRTCIdentityRequest. On 2013/06/27 20:05:18, Ami Fischman wrote: > I'm confused as to why this class exists. Why not simply bind Request's Cancel > for the cancel_callback in RequestIdentity? When the certificate caching is in place, we'll want combine multiple external requests with the same origin/name into one internal request. It gives use more flexibility to separate the external and internal representation of a request. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:122: // "this" will be deleted after the following call, becuase "this" is On 2013/06/27 20:05:18, Ami Fischman wrote: > typo: becuase Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:157: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); On 2013/06/27 20:05:18, Ami Fischman wrote: > I think it is true that all the methods in this class other than > GenerateIdentityWorker want to have this DCHECK, but most don't. Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:166: WebRTCIdentityRequestResult* result = new WebRTCIdentityRequestResult; On 2013/06/27 20:05:18, Ami Fischman wrote: > nit: always include the ()'s on a new expression. There are semantic > differences (initialization style) between including them and excluding them for > some new expressions, though not here, but for consistency it's best to always > include them Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:177: return true; On 2013/06/27 20:05:18, Ami Fischman wrote: > fwiw: since this method returns true iff *cancel_callback is set, could as well > return the Cancel closure (or Closure()) and have the caller test the return > value for is_null() to decide whether the request has been kicked off. The > (tiny) advantage of doing this is that it makes the caller more likely to > examine the return value (as opposed to caller incorrectly forgetting to examine > the return value because it is focused on the output parameter). > Some examples of such forgetfulness are in the unittest for this class ;) Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... File content/browser/media/webrtc_identity_store.h (right): https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.h:66: explicit WebRTCIdentityStore( On 2013/06/27 20:05:18, Ami Fischman wrote: > Is this ctor only used for testing? If so, might be useful to mark it as such > in a comment, or, better yet, have an explicit private > void SetTaskRunnerForTest(const scoped_refptr<base::TaskRunner>& task_runner); > method that modifies an already-constructed object. Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... File content/browser/media/webrtc_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:27: new base::SequencedWorkerPool(3, "ServerBoundCertServiceTest")) { On 2013/06/27 20:05:18, Ami Fischman wrote: > "ServerBoundCertServiceTest" is copy/pasta? Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:40: namespace { On 2013/06/27 20:05:18, Ami Fischman wrote: > nit: prefer to make ORC below static and the TEST_F's don't need to be in an > anonymous namespace. Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:53: scoped_ptr<bool> completed(new bool(false)); On 2013/06/27 20:05:18, Ami Fischman wrote: > why not just > bool completed = false; > and pass &completed to the Bind below? Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:55: webrtc_identity_store_->RequestIdentity( On 2013/06/27 20:05:18, Ami Fischman wrote: > ASSERT_TRUE? Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:86: webrtc_identity_store_->RequestIdentity( On 2013/06/27 20:05:18, Ami Fischman wrote: > ditto ASSERT_TRUE Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:93: webrtc_identity_store_->RequestIdentity( On 2013/06/27 20:05:18, Ami Fischman wrote: > ditto ASSERT_TRUE Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... File content/browser/renderer_host/media/webrtc_identity_service_host.cc (right): https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.cc:16: : pending_request_id_(-1), identity_store_(identity_store) {} On 2013/06/27 20:05:18, Ami Fischman wrote: > This assumes -1 is not a valid request_id. Should the IPC say so? Removed. https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.cc:65: cancel_callback_.Reset(); On 2013/06/27 20:05:18, Ami Fischman wrote: > base::ResetAndRun(&cancel_callback_); > ? Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.cc:73: DCHECK_EQ(request_id, pending_request_id_); On 2013/06/27 20:05:18, Ami Fischman wrote: > Why does pending_request_id_ exist? > If it's to safeguard against this very DCHECK condition, then it should probably > be a CHECK or have explicit error handling even in non-debug builds. > If it's not for that then I think you can drop it and simply use the is_null() > status of cancel_callback_ to know whether a request is pending. Done. https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... File content/browser/renderer_host/media/webrtc_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/188001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.h:25: // Only one outstanding request is allowed at a time. If a second request is On 2013/06/27 20:05:18, Ami Fischman wrote: > s/is allowed/is allowed per renderer/ ? Done. https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... File content/renderer/media/webrtc_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.cc:13: : origin_(origin), pending_observer_(NULL), pending_request_id_(0) { On 2013/06/27 20:05:18, Ami Fischman wrote: > s/0/-1/ to match browser side? Browser side initial value removed. https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.cc:29: static int s_next_request_id = 0; On 2013/06/27 20:05:18, Ami Fischman wrote: > This is not thread-safe. > You might want base::AtomicSequenceNumber. Done. https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... File content/renderer/media/webrtc_identity_service.h (right): https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.h:22: WebRTCIdentityService(const GURL& origin); On 2013/06/27 20:05:18, Ami Fischman wrote: > explicit Done. https://codereview.chromium.org/15969025/diff/188001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.h:28: webrtc::DTLSIdentityRequestObserver* observer) On 2013/06/27 20:05:18, Ami Fischman wrote: > why does this interface pass a raw pointer to a refcounted object? This is following the other webrtc interfaces, like PeerConnectionInterface::GetStats.
LGTM % nits https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... File content/browser/media/webrtc_identity_store.cc (right): https://codereview.chromium.org/15969025/diff/188001/content/browser/media/we... content/browser/media/webrtc_identity_store.cc:102: // WebRTCIdentityRequest. On 2013/06/27 21:08:37, jiayl wrote: > On 2013/06/27 20:05:18, Ami Fischman wrote: > > I'm confused as to why this class exists. Why not simply bind Request's > Cancel > > for the cancel_callback in RequestIdentity? > > When the certificate caching is in place, we'll want combine multiple external > requests with the same origin/name into one internal request. It gives use more > flexibility to separate the external and internal representation of a request. I don't quite get this, and in general I prefer to not to land code because it'll be useful in the future. But you're much more involved with this code and where it's going than I am so I leave it up to you to decide if you want to keep it. https://codereview.chromium.org/15969025/diff/193001/content/browser/media/we... File content/browser/media/webrtc_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/193001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:57: base::Bind(&OnRequestCompleted, base::Unretained(&completed))); Is Unretained really necessary here?? https://codereview.chromium.org/15969025/diff/193001/content/browser/renderer... File content/browser/renderer_host/media/webrtc_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/193001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.h:25: // Only one outstanding request is allowed per renderer at a time. If a second I wasn't part of the conversation that led to this decision so I don't know why you imposed this limit. I just want to point out that this will cause apparent flakiness as unrelated webpages loading in different tabs (but the same renderer process) will appear to interfere with each other. Hopefully there's a good reason for this limit ;) https://codereview.chromium.org/15969025/diff/193001/content/renderer/media/w... File content/renderer/media/webrtc_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/193001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.cc:30: static base::AtomicSequenceNumber s_next_request_id; "static" storage generally is a code smell so good to explain why to future readers: // Static because this id needs to be unique per renderer process.
Thanks! https://codereview.chromium.org/15969025/diff/193001/content/browser/media/we... File content/browser/media/webrtc_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/193001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:57: base::Bind(&OnRequestCompleted, base::Unretained(&completed))); On 2013/06/27 21:30:32, Ami Fischman wrote: > Is Unretained really necessary here?? I thought it won't compile without it because it will try addref it. But apparently that's not true. https://codereview.chromium.org/15969025/diff/193001/content/browser/renderer... File content/browser/renderer_host/media/webrtc_identity_service_host.h (right): https://codereview.chromium.org/15969025/diff/193001/content/browser/renderer... content/browser/renderer_host/media/webrtc_identity_service_host.h:25: // Only one outstanding request is allowed per renderer at a time. If a second On 2013/06/27 21:30:32, Ami Fischman wrote: > I wasn't part of the conversation that led to this decision so I don't know why > you imposed this limit. I just want to point out that this will cause apparent > flakiness as unrelated webpages loading in different tabs (but the same renderer > process) will appear to interfere with each other. Hopefully there's a good > reason for this limit ;) It's for the security concern that an evil renderer can exhaust the browser resource by sending a lot of requests. We'll need to retry the request at somewhere, but I think I'll defer that to a follow up CL. https://codereview.chromium.org/15969025/diff/193001/content/renderer/media/w... File content/renderer/media/webrtc_identity_service.cc (right): https://codereview.chromium.org/15969025/diff/193001/content/renderer/media/w... content/renderer/media/webrtc_identity_service.cc:30: static base::AtomicSequenceNumber s_next_request_id; On 2013/06/27 21:30:32, Ami Fischman wrote: > "static" storage generally is a code smell so good to explain why to future > readers: > // Static because this id needs to be unique per renderer process. Done.
FYI https://codereview.chromium.org/15969025/diff/193001/content/browser/media/we... File content/browser/media/webrtc_identity_store_unittest.cc (right): https://codereview.chromium.org/15969025/diff/193001/content/browser/media/we... content/browser/media/webrtc_identity_store_unittest.cc:57: base::Bind(&OnRequestCompleted, base::Unretained(&completed))); On 2013/06/27 21:48:28, jiayl wrote: > On 2013/06/27 21:30:32, Ami Fischman wrote: > > Is Unretained really necessary here?? > > I thought it won't compile without it because it will try addref it. But > apparently that's not true. That concern only applies to the |this| argument bound by Bind, if one exists. Since you're binding a free function here, there is no |this| argument. Other (non-|this|) arguments to Bind don't get addref'd unless you use Owned etc.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15969025/178002
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15969025/213001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15969025/178006
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15969025/227001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15969025/227001
Retried try job too often on win_rel for step(s) chrome_frame_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15969025/227001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15969025/227001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15969025/227001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15969025/268001
Message was sent while issue was closed.
Change committed as 209499 |
