Issue 61643007: Update keygen to use correct NSS slot on ChromeOS multiprofile.

mattm

joi: content and chrome_content_browser_client rsleevi: resst (depends on https://codereview.chromium.org/53763003/) This is a little bigger than ...

7 years, 1 month ago (2013-11-07 04:10:26 UTC) #1

Ryan Sleevi

https://codereview.chromium.org/61643007/diff/510001/chrome/browser/ui/crypto_module_delegate_nss.cc File chrome/browser/ui/crypto_module_delegate_nss.cc (right): https://codereview.chromium.org/61643007/diff/510001/chrome/browser/ui/crypto_module_delegate_nss.cc#newcode34 chrome/browser/ui/crypto_module_delegate_nss.cc:34: password_.replace(0, password_.size(), password_.size(), 0); This seems... highly unnecessary. We ...

7 years, 1 month ago (2013-11-22 00:03:29 UTC) #2

mattm

https://codereview.chromium.org/61643007/diff/510001/chrome/browser/ui/crypto_module_delegate_nss.cc File chrome/browser/ui/crypto_module_delegate_nss.cc (right): https://codereview.chromium.org/61643007/diff/510001/chrome/browser/ui/crypto_module_delegate_nss.cc#newcode34 chrome/browser/ui/crypto_module_delegate_nss.cc:34: password_.replace(0, password_.size(), password_.size(), 0); On 2013/11/22 00:03:30, Ryan Sleevi ...

7 years, 1 month ago (2013-11-22 00:30:31 UTC) #3

https://codereview.chromium.org/61643007/diff/510001/chrome/browser/ui/crypto...
File chrome/browser/ui/crypto_module_delegate_nss.cc (right):

https://codereview.chromium.org/61643007/diff/510001/chrome/browser/ui/crypto...
chrome/browser/ui/crypto_module_delegate_nss.cc:34: password_.replace(0,
password_.size(), password_.size(), 0);
On 2013/11/22 00:03:30, Ryan Sleevi wrote:
> This seems... highly unnecessary.
> 
> We should either be doing an optimizer-unfriendly memory erase (which we can't
> really do, when using an std::string), or we should just remove this entirely,
> because it implies we care about the password at rest, but we've just
liberally
> copied it around as a string (line 57) that we've given up all hope of
managing
> that buffer.

Done.

https://codereview.chromium.org/61643007/diff/510001/chrome/browser/ui/crypto...
chrome/browser/ui/crypto_module_delegate_nss.cc:46: // Caller is responsible for
keeping us alive.
On 2013/11/22 00:03:30, Ryan Sleevi wrote:
> Alive until when? How can the caller know when things are safe to tear down?

Until we call the callback. I'll update the comment.

> Should we be using an inner WeakPtr'd helper-core instead?

I think the approach of the caller keeping it alive using base::Passed in the
callback works correctly. We could change it to use a weakptr to make the
interface simpler, though I don't think it would actually change the way
RenderMessageFilter uses it.
If we did use weakptr, I don't think the inner helper approach would be
necessary here, a simple weakptrfactory member should do.

https://codereview.chromium.org/61643007/diff/510001/chrome/browser/ui/crypto...
File chrome/browser/ui/crypto_module_delegate_nss.h (right):

https://codereview.chromium.org/61643007/diff/510001/chrome/browser/ui/crypto...
chrome/browser/ui/crypto_module_delegate_nss.h:25:
crypto::NSSCryptoModuleDelegate* NewNSSCryptoModuleDelegate(
On 2013/11/22 00:03:30, Ryan Sleevi wrote:
> naming nit: Don't we normally call these factory functions "Create*" instead
of
> "New*"
> 
> eg: CreateSSLClientSocket

Done.

https://codereview.chromium.org/61643007/diff/510001/crypto/nss_crypto_module...
File crypto/nss_crypto_module_delegate.h (right):

https://codereview.chromium.org/61643007/diff/510001/crypto/nss_crypto_module...
crypto/nss_crypto_module_delegate.h:41: class NSSCryptoModuleDelegate : public
CryptoModuleBlockingPasswordDelegate {
On 2013/11/22 00:03:30, Ryan Sleevi wrote:
> I don't understand why the separate class with the inheritance here. These are
> both NSS-only classes. The comments don't indicate the distinction in expected
> uses.

I wasn't entirely sure about this, but it's because
CryptoModuleBlockingPasswordDelegate could theoretically be used for other cases
where requesting a slot doesn't make sense. Like if we moved other NSS
operations to the worker thread and passed a
CryptoModuleBlockingPasswordDelegate to use as the wincx, it might be weird to
have to construct a delegate that also has the ability to get a PK11Slot. I'm
not too wedded to this separation, so I could combine them or just add some more
comments, what do you think?

Ryan Sleevi

Still a bit uncomfortable with the separation, especially the imposition by the higher-layer implementation of ...

7 years ago (2013-11-26 20:13:17 UTC) #4

mattm

Refactored the whole KeygenHandler and crypto module delegate creation and initialization process. Now the RenderMessageFilter ...

7 years ago (2013-12-03 00:40:53 UTC) #5

Ryan Sleevi

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles/profile_io_data.cc File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles/profile_io_data.cc#newcode865 chrome/browser/profiles/profile_io_data.cc:865: DCHECK(!rv); I still find the async setup here a ...

7 years ago (2013-12-05 00:23:19 UTC) #6

mattm

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles/profile_io_data.cc File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles/profile_io_data.cc#newcode865 chrome/browser/profiles/profile_io_data.cc:865: DCHECK(!rv); On 2013/12/05 00:23:19, Ryan Sleevi wrote: > I ...

7 years ago (2013-12-05 00:42:43 UTC) #7

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:865: DCHECK(!rv);
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> I still find the async setup here a little weird.
> 
> I understand you want to support both synchronous (already-initialized) and
> async cases, but the "calling InitializeSlot twice" paradigm feels... wrong.

Yeah, this is weird. The problem is the base::Passed(&keygen_handler) in the
callback. With only a single call to InitializeSlot, the keygen_handler is
already owned by the callback so can't be returned on synchronous completion.

> Can we not use the net/ pattern of "true / positive value = completed
> synchronously, false / negative value = completing async"?
> 
> I'm also not sure I understand the line 866 below. Minimally, a comment
> explaining why returning NULL (and yet initializing one asynchronously above)
is
> somehow OK.

In the async case, the keygen_handler is passed to the callback, so it returns
NULL on 866.

I guess I could address both by changing the signature to:
bool CreateKeygenHandler(
       uint32 key_size_in_bits,
       const std::string& challenge_string,
       const GURL& url,
       const base::Closure& callback,
       scoped_ptr<KeygenHandler>* keygen_handler);

Then it would always set the keygen_handler, and in the sync case return true,
and the async case return false and use the callback on completion. I guess I
found the current way a little safer since it never returns a partially
initialized KeygenHandler, but it does make the implementation here weirder.
WDYT?

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/gtk/s...
File chrome/browser/ui/gtk/ssl_client_certificate_selector.cc (right):

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/gtk/s...
chrome/browser/ui/gtk/ssl_client_certificate_selector.cc:358: #if
defined(USE_NSS)
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> Unnecessary.
> 
> There should be no world in which we'd be in a GTK file and !USE_NSS (redux
> build for UI isn't supported, IIRC)

the linux_redux bot does build UI stuff, so it seems to be implicitly supported.

mattm

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles/profile_io_data.cc File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles/profile_io_data.cc#newcode865 chrome/browser/profiles/profile_io_data.cc:865: DCHECK(!rv); On 2013/12/05 00:42:44, mattm wrote: > On 2013/12/05 ...

7 years ago (2013-12-05 00:46:48 UTC) #8

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:865: DCHECK(!rv);
On 2013/12/05 00:42:44, mattm wrote:
> On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> > I still find the async setup here a little weird.
> > 
> > I understand you want to support both synchronous (already-initialized) and
> > async cases, but the "calling InitializeSlot twice" paradigm feels... wrong.
> 
> Yeah, this is weird. The problem is the base::Passed(&keygen_handler) in the
> callback. With only a single call to InitializeSlot, the keygen_handler is
> already owned by the callback so can't be returned on synchronous completion.
> 
> > Can we not use the net/ pattern of "true / positive value = completed
> > synchronously, false / negative value = completing async"?
> > 
> > I'm also not sure I understand the line 866 below. Minimally, a comment
> > explaining why returning NULL (and yet initializing one asynchronously
above)
> is
> > somehow OK.
> 
> In the async case, the keygen_handler is passed to the callback, so it returns
> NULL on 866.
> 
> 
> I guess I could address both by changing the signature to:
> bool CreateKeygenHandler(
>        uint32 key_size_in_bits,
>        const std::string& challenge_string,
>        const GURL& url,
>        const base::Closure& callback,
>        scoped_ptr<KeygenHandler>* keygen_handler);
> 
> Then it would always set the keygen_handler, and in the sync case return true,
> and the async case return false and use the callback on completion. I guess I
> found the current way a little safer since it never returns a partially
> initialized KeygenHandler, but it does make the implementation here weirder.
> WDYT?
> 

Oh yeah, the problem with that approach is that then the caller
(RenderMessageFilter) still somehow needs to get the KeygenHandler into the
callback so that it can do the magic once it is initialized.  If the
CreateKeygenHandler always returns the keygen handler, then there isn't a good
way to get it into the callback that I see.

Ryan Sleevi

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles/profile_io_data.cc File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles/profile_io_data.cc#newcode865 chrome/browser/profiles/profile_io_data.cc:865: DCHECK(!rv); On 2013/12/05 00:46:48, mattm wrote: > On 2013/12/05 ...

7 years ago (2013-12-05 01:05:53 UTC) #9

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:865: DCHECK(!rv);
On 2013/12/05 00:46:48, mattm wrote:
> On 2013/12/05 00:42:44, mattm wrote:
> > On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> > > I still find the async setup here a little weird.
> > > 
> > > I understand you want to support both synchronous (already-initialized)
and
> > > async cases, but the "calling InitializeSlot twice" paradigm feels...
wrong.
> > 
> > Yeah, this is weird. The problem is the base::Passed(&keygen_handler) in the
> > callback. With only a single call to InitializeSlot, the keygen_handler is
> > already owned by the callback so can't be returned on synchronous
completion.
> > 
> > > Can we not use the net/ pattern of "true / positive value = completed
> > > synchronously, false / negative value = completing async"?
> > > 
> > > I'm also not sure I understand the line 866 below. Minimally, a comment
> > > explaining why returning NULL (and yet initializing one asynchronously
> above)
> > is
> > > somehow OK.
> > 
> > In the async case, the keygen_handler is passed to the callback, so it
returns
> > NULL on 866.
> > 
> > 
> > I guess I could address both by changing the signature to:
> > bool CreateKeygenHandler(
> >        uint32 key_size_in_bits,
> >        const std::string& challenge_string,
> >        const GURL& url,
> >        const base::Closure& callback,
> >        scoped_ptr<KeygenHandler>* keygen_handler);
> > 
> > Then it would always set the keygen_handler, and in the sync case return
true,
> > and the async case return false and use the callback on completion. I guess
I
> > found the current way a little safer since it never returns a partially
> > initialized KeygenHandler, but it does make the implementation here weirder.
> > WDYT?
> > 
> 
> Oh yeah, the problem with that approach is that then the caller
> (RenderMessageFilter) still somehow needs to get the KeygenHandler into the
> callback so that it can do the magic once it is initialized.  If the
> CreateKeygenHandler always returns the keygen handler, then there isn't a good
> way to get it into the callback that I see.
> 

I feel like you can avoid this by avoiding the base::Passed(&keygen_handler) in
currying, since you don't "always" want it passed (sync vs async)

struct ISuckAtNaming {
  scoped_ptr<KeygenHandler> handler;
};

void MyHelper(const base::Callback<void(scoped_ptr<net::KeygenHandler>)>&
callback,
ISuckAtNaming* foo) {
  callback.Run(foo->handler.Pass());
};

ISuckAtNaming* foo = new ISuckAtNaming;
foo->handler = keygen_handler.Pass();

// Keep |cb| on the stack, since ownership of |foo| is transferred to |cb|.
// This allows |foo| to still be accessed if InitializeSlot returns
// with synchronous success.
base::Closure cb = base::Bind(MyHelper, callback, base::Owned(foo));
bool rv = delegate_ptr->InitializeSlot(this, cb);
if (rv)
  return foo->handler.Pass();
return scoped_ptr<net::KeygenHandler>();

mattm

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles/profile_io_data.cc File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles/profile_io_data.cc#newcode865 chrome/browser/profiles/profile_io_data.cc:865: DCHECK(!rv); On 2013/12/05 01:05:54, Ryan Sleevi wrote: > On ...

7 years ago (2013-12-05 04:41:25 UTC) #10

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:865: DCHECK(!rv);
On 2013/12/05 01:05:54, Ryan Sleevi wrote:
> On 2013/12/05 00:46:48, mattm wrote:
> > On 2013/12/05 00:42:44, mattm wrote:
> > > On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> > > > I still find the async setup here a little weird.
> > > > 
> > > > I understand you want to support both synchronous (already-initialized)
> and
> > > > async cases, but the "calling InitializeSlot twice" paradigm feels...
> wrong.
> > > 
> > > Yeah, this is weird. The problem is the base::Passed(&keygen_handler) in
the
> > > callback. With only a single call to InitializeSlot, the keygen_handler is
> > > already owned by the callback so can't be returned on synchronous
> completion.
> > > 
> > > > Can we not use the net/ pattern of "true / positive value = completed
> > > > synchronously, false / negative value = completing async"?
> > > > 
> > > > I'm also not sure I understand the line 866 below. Minimally, a comment
> > > > explaining why returning NULL (and yet initializing one asynchronously
> > above)
> > > is
> > > > somehow OK.
> > > 
> > > In the async case, the keygen_handler is passed to the callback, so it
> returns
> > > NULL on 866.
> > > 
> > > 
> > > I guess I could address both by changing the signature to:
> > > bool CreateKeygenHandler(
> > >        uint32 key_size_in_bits,
> > >        const std::string& challenge_string,
> > >        const GURL& url,
> > >        const base::Closure& callback,
> > >        scoped_ptr<KeygenHandler>* keygen_handler);
> > > 
> > > Then it would always set the keygen_handler, and in the sync case return
> true,
> > > and the async case return false and use the callback on completion. I
guess
> I
> > > found the current way a little safer since it never returns a partially
> > > initialized KeygenHandler, but it does make the implementation here
weirder.
> > > WDYT?
> > > 
> > 
> > Oh yeah, the problem with that approach is that then the caller
> > (RenderMessageFilter) still somehow needs to get the KeygenHandler into the
> > callback so that it can do the magic once it is initialized.  If the
> > CreateKeygenHandler always returns the keygen handler, then there isn't a
good
> > way to get it into the callback that I see.
> > 
> 
> I feel like you can avoid this by avoiding the base::Passed(&keygen_handler)
in
> currying, since you don't "always" want it passed (sync vs async)
> 
> struct ISuckAtNaming {
>   scoped_ptr<KeygenHandler> handler;
> };
> 
> void MyHelper(const base::Callback<void(scoped_ptr<net::KeygenHandler>)>&
> callback,
> ISuckAtNaming* foo) {
>   callback.Run(foo->handler.Pass());
> };
> 
> ISuckAtNaming* foo = new ISuckAtNaming;
> foo->handler = keygen_handler.Pass();
> 
> // Keep |cb| on the stack, since ownership of |foo| is transferred to |cb|.
> // This allows |foo| to still be accessed if InitializeSlot returns
> // with synchronous success.
> base::Closure cb = base::Bind(MyHelper, callback, base::Owned(foo));
> bool rv = delegate_ptr->InitializeSlot(this, cb);
> if (rv)
>   return foo->handler.Pass();
> return scoped_ptr<net::KeygenHandler>();

Ah, yeah. I've done that (though just using Owned directly on a scoped_ptr*, did
you want the struct for readability?)

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/crypt...
File chrome/browser/ui/crypto_module_delegate_nss.cc (right):

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/crypt...
chrome/browser/ui/crypto_module_delegate_nss.cc:35: return slot_ != NULL;
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> "slot_.get() != NULL"

done

> or simply "return slot_"

I found that confusing since you have to remember what the method prototype is
to realize that it's being converted to a bool. There's also the !!slot_ style,
what are your feelings about that?

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/crypt...
chrome/browser/ui/crypto_module_delegate_nss.cc:47:
DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> Is this necessary, given that event_.Wait() will check
> base::ThreadRestrictions::AssertWaitAllowed() ?

Good point. done.

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/crypt...
chrome/browser/ui/crypto_module_delegate_nss.cc:56: // there's no need to
ref-count.
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> comment nit: pronouns

Done.

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/crypt...
File chrome/browser/ui/crypto_module_delegate_nss.h (right):

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/crypt...
chrome/browser/ui/crypto_module_delegate_nss.h:19: class
ChromeNSSCryptoModuleDelegate
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> Why is the free function in namespace chrome, but not this class?

hm, I guess I didn't like chrome::NSSCryptoModuleDelegate since it could be
confusing to have the same named class in multiple namespaces. I'll just move
the function out of the namespace.

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/crypt...
chrome/browser/ui/crypto_module_delegate_nss.h:26: const std::string& server);
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> I feel like we definitely should be using the GURL here if it's a user-visible
> string. The KeygenHandler has the GURL (since Safari uses it w/ Keychain
Access
> in the WebKit impl, for example)

This can also be used with client auth, where the SSLCertRequestInfo only has a
std::string host_and_port. We should probably switch that to a HostPortPair at
least, then this could also take a HostPortPair. Mind if I do all that in a
separate CL?

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/crypt...
chrome/browser/ui/crypto_module_delegate_nss.h:36: WARN_UNUSED_RESULT;
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> Unnecessary W_U_R

I think this one is necessary. If you pass a callback and then don't check the
result, you can't know if it's safe to use or not.

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/crypt...
File chrome/browser/ui/crypto_module_password_dialog_nss.h (right):

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/crypt...
chrome/browser/ui/crypto_module_password_dialog_nss.h:26: // only be called on
UI thread.
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> comment nit: Single spaces follow periods, not double. Same with 33-35.

Done.

https://codereview.chromium.org/61643007/diff/1190001/chrome/browser/ui/crypt...
chrome/browser/ui/crypto_module_password_dialog_nss.h:29: const std::string&
server,
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> I didn't follow your comment. Are you saying different callers call
> UnlockSlotsIfNecessary, sometimes with a hostname and sometimes with a
> host_and_port?

yeah, basically. (I don't recall if it was through UnlockSlotsIfNecessary in
different ways, or some go UnlockSlotsIfNecessary is one way and the password
delegate users went a different way.  But ShowCryptoModulePasswordDialog ends up
getting called in different ways.)

> If so, rather than renaming this variable, the callers should be made to be
> consistent. I'd lean on "hostname" as the preferred form.

I'd like to do that all in a separate CL if it's ok with you.

https://codereview.chromium.org/61643007/diff/1190001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/61643007/diff/1190001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2944:
'browser/ui/webui/options/certificate_manager_handler.cc',
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> nit: In a future CL, we should add _nss suffix to these two files, so it's
clear
> they're NSS only.

yeah, will do.

https://codereview.chromium.org/61643007/diff/1190001/content/public/browser/...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/61643007/diff/1190001/content/public/browser/...
content/public/browser/resource_context.h:53: virtual
scoped_ptr<net::KeygenHandler> CreateKeygenHandler(
On 2013/12/05 00:23:19, Ryan Sleevi wrote:
> Document this function. I had to look this up when trying to find out why
> "return NULL" was acceptable, and I realized this is not at all documented.

Done.

Ryan Sleevi

I reviewed Patchset 9, since Patchset 10 was a rebase. LGTM. Thanks for simplifying (admittedly, ...

7 years ago (2013-12-10 22:02:36 UTC) #12

sky

I know next to nothing about NSS. Exactly what files do you need me to ...

7 years ago (2013-12-11 00:12:31 UTC) #14

mattm

On 2013/12/11 00:12:31, sky wrote: > I know next to nothing about NSS. Exactly what ...

7 years ago (2013-12-11 00:18:11 UTC) #15

sky

https://codereview.chromium.org/61643007/diff/1220001/chrome/browser/profiles/profile_io_data.cc File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/61643007/diff/1220001/chrome/browser/profiles/profile_io_data.cc#newcode858 chrome/browser/profiles/profile_io_data.cc:858: scoped_ptr<net::KeygenHandler>* keygen_handler = Can't you use a scoped_ptr rather ...

7 years ago (2013-12-11 00:25:59 UTC) #16

sky

https://codereview.chromium.org/61643007/diff/1220001/chrome/browser/profiles/profile_io_data.cc File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/61643007/diff/1220001/chrome/browser/profiles/profile_io_data.cc#newcode858 chrome/browser/profiles/profile_io_data.cc:858: scoped_ptr<net::KeygenHandler>* keygen_handler = Can't you use a scoped_ptr rather ...

7 years ago (2013-12-11 00:26:00 UTC) #17

mattm

https://codereview.chromium.org/61643007/diff/1220001/chrome/browser/profiles/profile_io_data.cc File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/61643007/diff/1220001/chrome/browser/profiles/profile_io_data.cc#newcode858 chrome/browser/profiles/profile_io_data.cc:858: scoped_ptr<net::KeygenHandler>* keygen_handler = On 2013/12/11 00:26:00, sky wrote: > ...

7 years ago (2013-12-11 01:10:45 UTC) #18

sky

https://codereview.chromium.org/61643007/diff/1220001/chrome/browser/profiles/profile_io_data.cc File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/61643007/diff/1220001/chrome/browser/profiles/profile_io_data.cc#newcode858 chrome/browser/profiles/profile_io_data.cc:858: scoped_ptr<net::KeygenHandler>* keygen_handler = On 2013/12/11 01:10:46, mattm wrote: > ...

7 years ago (2013-12-11 14:20:41 UTC) #19

Ryan Sleevi

On Dec 11, 2013 6:20 AM, <sky@chromium.org> wrote: > > > https://codereview.chromium.org/61643007/diff/1220001/chrome/browser/profiles/profile_io_data.cc > File chrome/browser/profiles/profile_io_data.cc ...

7 years ago (2013-12-11 17:06:48 UTC) #20

mattm

https://codereview.chromium.org/61643007/diff/1280001/chrome/browser/profiles/profile_io_data.cc File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/61643007/diff/1280001/chrome/browser/profiles/profile_io_data.cc#newcode357 chrome/browser/profiles/profile_io_data.cc:357: }; On 2013/12/11 14:20:42, sky wrote: > nit: no ...

7 years ago (2013-12-11 23:12:08 UTC) #21

sky

https://codereview.chromium.org/61643007/diff/1280001/chrome/browser/ui/crypto_module_delegate_nss.cc File chrome/browser/ui/crypto_module_delegate_nss.cc (right): https://codereview.chromium.org/61643007/diff/1280001/chrome/browser/ui/crypto_module_delegate_nss.cc#newcode50 chrome/browser/ui/crypto_module_delegate_nss.cc:50: if (BrowserThread::PostTask( On 2013/12/11 23:12:09, mattm wrote: > On ...

7 years ago (2013-12-12 00:25:12 UTC) #22

mattm

https://codereview.chromium.org/61643007/diff/1280001/chrome/browser/ui/crypto_module_delegate_nss.cc File chrome/browser/ui/crypto_module_delegate_nss.cc (right): https://codereview.chromium.org/61643007/diff/1280001/chrome/browser/ui/crypto_module_delegate_nss.cc#newcode50 chrome/browser/ui/crypto_module_delegate_nss.cc:50: if (BrowserThread::PostTask( On 2013/12/12 00:25:13, sky wrote: > On ...

7 years ago (2013-12-12 00:28:57 UTC) #23

jam

lgtm with comment https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/content_browser_client.cc File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/content_browser_client.cc#newcode11 content/public/browser/content_browser_client.cc:11: #if defined(USE_NSS) ditto https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/content_browser_client.h File content/public/browser/content_browser_client.h ...

7 years ago (2013-12-12 06:42:49 UTC) #25

mattm

https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/content_browser_client.cc File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/content_browser_client.cc#newcode11 content/public/browser/content_browser_client.cc:11: #if defined(USE_NSS) On 2013/12/12 06:42:50, jam wrote: > ditto ...

7 years ago (2013-12-12 21:49:17 UTC) #26

jam

https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/resource_context.h File content/public/browser/resource_context.h (right): https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/resource_context.h#newcode57 content/public/browser/resource_context.h:57: virtual scoped_ptr<net::KeygenHandler> CreateKeygenHandler( On 2013/12/12 21:49:18, mattm wrote: > ...

7 years ago (2013-12-13 03:16:38 UTC) #27

Ryan Sleevi

On 2013/12/13 03:16:38, jam wrote: > https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/resource_context.h > File content/public/browser/resource_context.h (right): > > https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/resource_context.h#newcode57 > ...

7 years ago (2013-12-13 18:48:34 UTC) #28

On 2013/12/13 03:16:38, jam wrote:
>
https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/...
> File content/public/browser/resource_context.h (right):
> 
>
https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/...
> content/public/browser/resource_context.h:57: virtual
> scoped_ptr<net::KeygenHandler> CreateKeygenHandler(
> On 2013/12/12 21:49:18, mattm wrote:
> > On 2013/12/12 06:42:50, jam wrote:
> > > nit: we prefer one way of returning data. so if it can be async, just have
a
> > > callback and no return value. in the comment you can mention that it's
safe
> > for
> > > the embedder to run the callback in this method
> > 
> > Sleevi had a concern that that style of callback which can be run
> synchronously
> > can be surprising or dangerous
> >
>
(https://codereview.chromium.org/53763003/diff/840001/crypto/nss_util.h#newcod...)
> > Thoughts?
> 
> I can understand the concern, but personally I think there are numerous ways
> that someone can mess up any contract if they don't read it carefully. i.e.
what
> if someone returns the object and then also calls the callback? or returns
NULL
> and calls the callback synchronously? I also tend to really prefer one way of
> doing things.

jam@: There are a couple concerns here:

- Always returning asynchronously requires using a PostTask, which has enough
overhead on Android to be undesirable when possible
- Always returning synchronously requires blocking, which we agree is nasty
- Running the callback synchronously when it completes sync, and via PostTask
when async seems desirable, but I've seen regular bugs related to either
recursion depths (eg: bad processing loops) or deleting the object when it
shouldn't be that I strongly avoid against this pattern, because it's frequently
abused.

Within all of net/, we follow the pattern of sync-returns-for-sync-results and
async-callbacks-for-async-results, and this avoids a lot of the issues. This is
especially important because net/ is made up of many state machine loops, and
running callbacks synchronously just hoists even more complexity on the caller
to break out while memoizing results.

jam

On 2013/12/13 18:48:34, Ryan Sleevi wrote: > On 2013/12/13 03:16:38, jam wrote: > > > ...

7 years ago (2013-12-13 20:54:40 UTC) #29

On 2013/12/13 18:48:34, Ryan Sleevi wrote:
> On 2013/12/13 03:16:38, jam wrote:
> >
>
https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/...
> > File content/public/browser/resource_context.h (right):
> > 
> >
>
https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/...
> > content/public/browser/resource_context.h:57: virtual
> > scoped_ptr<net::KeygenHandler> CreateKeygenHandler(
> > On 2013/12/12 21:49:18, mattm wrote:
> > > On 2013/12/12 06:42:50, jam wrote:
> > > > nit: we prefer one way of returning data. so if it can be async, just
have
> a
> > > > callback and no return value. in the comment you can mention that it's
> safe
> > > for
> > > > the embedder to run the callback in this method
> > > 
> > > Sleevi had a concern that that style of callback which can be run
> > synchronously
> > > can be surprising or dangerous
> > >
> >
>
(https://codereview.chromium.org/53763003/diff/840001/crypto/nss_util.h#newcod...)
> > > Thoughts?
> > 
> > I can understand the concern, but personally I think there are numerous ways
> > that someone can mess up any contract if they don't read it carefully. i.e.
> what
> > if someone returns the object and then also calls the callback? or returns
> NULL
> > and calls the callback synchronously? I also tend to really prefer one way
of
> > doing things.
> 
> jam@: There are a couple concerns here:
> 
> - Always returning asynchronously requires using a PostTask, which has enough
> overhead on Android to be undesirable when possible
> - Always returning synchronously requires blocking, which we agree is nasty
> - Running the callback synchronously when it completes sync, and via PostTask
> when async seems desirable, but I've seen regular bugs related to either
> recursion depths (eg: bad processing loops) or deleting the object when it
> shouldn't be that I strongly avoid against this pattern, because it's
frequently
> abused.
> 
> Within all of net/, we follow the pattern of sync-returns-for-sync-results and
> async-callbacks-for-async-results, and this avoids a lot of the issues. This
is
> especially important because net/ is made up of many state machine loops, and
> running callbacks synchronously just hoists even more complexity on the caller
> to break out while memoizing results.

If net owners want to add this rule to all code in net, that's up to you. But in
content API I strongly lean towards simplicity, and having two ways here goes
against that. Especially that in this specific case the pointer is posted to a
worker pool anyways so calling the callback synchronously is not an issue.

mattm

https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/resource_context.h File content/public/browser/resource_context.h (right): https://codereview.chromium.org/61643007/diff/1360001/content/public/browser/resource_context.h#newcode57 content/public/browser/resource_context.h:57: virtual scoped_ptr<net::KeygenHandler> CreateKeygenHandler( On 2013/12/12 06:42:50, jam wrote: > ...

7 years ago (2013-12-14 01:58:28 UTC) #30