Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
795773002:
Un-refcount SSLClientAuthHandler. (Closed)
DescriptionUn-refcount SSLClientAuthHandler.
That class doesn't really need to be ref-counted externally. There is one
catch: the caller is responsible for ensuring ClientCertStore is alive for the
duration of its async operation.
Resolve this by detaching it into a ref-counted Core internal to
SSLClientAuthHandler. Add a test to test this case.
Also make ContentBrowserClient's default client auth hook always select no
certificate so content_shell doesn't hang.
This relands the rest of https://codereview.chromium.org/596873002
BUG=439134
Committed: https://crrev.com/6cd57dd5cbca53c416f099b985444225d676362a
Cr-Commit-Position: refs/heads/master@{#308142}
Patch Set 1 #
Total comments: 10
Patch Set 2 : mmenke comments #
Total comments: 20
Messages
Total messages: 21 (5 generated)
davidben@chromium.org changed reviewers: + mmenke@chromium.org
Here's the rest of the reverted CL, now with the crash fixed. pneubeck: FYI
https://codereview.chromium.org/795773002/diff/1/content/browser/loader/resou... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/795773002/diff/1/content/browser/loader/resou... content/browser/loader/resource_loader_unittest.cc:454: // Ownership of the |test_store| is about to be turned over to ResourceLoader. Could you add some docs to ClientCertStore? The name sounds to me like "this is the store of all client certs", which would imply a global object, not something that should be owned by an individual request. Makes sense to do that in another CL. https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... File content/browser/ssl/ssl_client_auth_handler.cc (right): https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... content/browser/ssl/ssl_client_auth_handler.cc:44: class SSLClientAuthHandler::Core : public base::RefCountedThreadSafe<Core> { Should give a description of this class, and what it's needed (To keep client_cert_store_ and cert_request_info_ until GetClientCerts completes asynchronously) https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... content/browser/ssl/ssl_client_auth_handler.cc:55: void Detach() { handler_ = nullptr; } Using a WeakPtr here seems more natural, and it already has a weak factory, too, conveniently enough. https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... content/browser/ssl/ssl_client_auth_handler.cc:60: *cert_request_info_, &cert_request_info_->client_certs, Eeee...This is ugly. It takes a const SSLCertRequestInfo&, and then a non-const member of that same const object. Beyond the scope of this CL, but this makes me sad. https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... File content/browser/ssl/ssl_client_auth_handler.h (right): https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... content/browser/ssl/ssl_client_auth_handler.h:52: scoped_refptr<Core> core_; Should include the ref counted header.
https://codereview.chromium.org/795773002/diff/1/content/browser/loader/resou... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/795773002/diff/1/content/browser/loader/resou... content/browser/loader/resource_loader_unittest.cc:454: // Ownership of the |test_store| is about to be turned over to ResourceLoader. On 2014/12/11 21:02:20, mmenke wrote: > Could you add some docs to ClientCertStore? The name sounds to me like "this is > the store of all client certs", which would imply a global object, not something > that should be owned by an individual request. > > Makes sense to do that in another CL. https://codereview.chromium.org/802563002 https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... File content/browser/ssl/ssl_client_auth_handler.cc (right): https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... content/browser/ssl/ssl_client_auth_handler.cc:44: class SSLClientAuthHandler::Core : public base::RefCountedThreadSafe<Core> { On 2014/12/11 21:02:20, mmenke wrote: > Should give a description of this class, and what it's needed (To keep > client_cert_store_ and cert_request_info_ until GetClientCerts completes > asynchronously) Done. https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... content/browser/ssl/ssl_client_auth_handler.cc:55: void Detach() { handler_ = nullptr; } On 2014/12/11 21:02:20, mmenke wrote: > Using a WeakPtr here seems more natural, and it already has a weak factory, too, > conveniently enough. Done. https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... content/browser/ssl/ssl_client_auth_handler.cc:60: *cert_request_info_, &cert_request_info_->client_certs, On 2014/12/11 21:02:20, mmenke wrote: > Eeee...This is ugly. It takes a const SSLCertRequestInfo&, and then a non-const > member of that same const object. > > Beyond the scope of this CL, but this makes me sad. Completely agreed. This is crazy. Even making this part of SSLClientAuthHandler at all is weird. We may as well farm that out to the chrome/ layer, thread ownership of various pieces aside. It's already the case that ClientCertStore is not used on one platform because the Android platform API we happen to call out to in chrome/ happens to work differently... ...except that everyone down to net/ needs to know about that platform API because the consumer only passes the certificate down and net/ digs up the key from some platform global which is one of the reasons it's impossible to unit test... I started trying to fix this but that ought to be for another CL. :-) There's a lot of stuff to tease out of this codepath. https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... File content/browser/ssl/ssl_client_auth_handler.h (right): https://codereview.chromium.org/795773002/diff/1/content/browser/ssl/ssl_clie... content/browser/ssl/ssl_client_auth_handler.h:52: scoped_refptr<Core> core_; On 2014/12/11 21:02:20, mmenke wrote: > Should include the ref counted header. Already included.
LGTM
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/795773002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/795773002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6cd57dd5cbca53c416f099b985444225d676362a Cr-Commit-Position: refs/heads/master@{#308142}
Message was sent while issue was closed.
pneubeck@chromium.org changed reviewers: + pneubeck@chromium.org
Message was sent while issue was closed.
sorry, already to late for this CL. Please consider my comments nonetheless. https://codereview.chromium.org/795773002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/795773002/diff/20001/content/browser/loader/r... content/browser/loader/resource_loader.cc:854: ssl_client_auth_handler_.reset(); please don't do this in a call stack containing ssl_client_auth_handler_ as a this ptr. you could probably reuse the existing weak_ptr_factory_ to delay the deletion. https://codereview.chromium.org/795773002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/795773002/diff/20001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:470: EXPECT_EQ(1, raw_ptr_to_store->request_count()); how do you ensure that the raw_ptr_to_store is still valid (same for all similar tests)? wouldn't it be more reliable if the ClientCertStoreStub would write its results to some variables that outlive the previous calls for sure? https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_client_auth_handler.cc (right): https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:61: base::Bind(&SSLClientAuthHandler::Core::DidGetClientCerts, this)); this seems still to be a rather weird ownership: Core lives as long client_cert_store_ keeps this callback that owns the Core object. Core object owns client_cert_store_. By the definition of 'Ownership = Trigger of destruction', this is a cyclic dependency and we should fix this. I looked into this a while ago and could take care of it. It's not a tiny change however. https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:88: : core_(nullptr), not required? https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:99: SSLClientAuthHandler::~SSLClientAuthHandler() { this should still somehow trigger a "shutdown" of the Core object. Currently, core_ might continue with an arbitrary number of steps. https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:145: // |this| may be deleted at this point. ^^ shouldn't the implementor of the callback prevent this case? it seems to be in general a very bad idea to delete your caller. otherwise, we would have to put such comments behind nearly every callback.Run() where the callback was provided from some external code. https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_client_auth_handler.h (right): https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.h:46: // Called when the user has selected a cert. wouldn't |cert| be null if the selection was aborted? https://codereview.chromium.org/795773002/diff/20001/content/public/browser/c... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/795773002/diff/20001/content/public/browser/c... content/public/browser/content_browser_client.h:396: // Selects a SSL client certificate and returns it to the |callback|. If no might be worth documenting that callback can happen to be called both synchronously and asynchronously.
Message was sent while issue was closed.
https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_client_auth_handler.cc (right): https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:144: callback_.Run(cert); Think this should use reset and return on the callback, for it to be cleaned up correctly. https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:145: // |this| may be deleted at this point. On 2014/12/14 16:45:13, pneubeck wrote: > ^^ > shouldn't the implementor of the callback prevent this case? it seems to be in > general a very bad idea to delete your caller. > otherwise, we would have to put such comments behind nearly every callback.Run() > where the callback was provided from some external code. In general, I think deletion tasks are uglier. Deleting your caller on completion is a pretty common design pattern.
Message was sent while issue was closed.
https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_client_auth_handler.cc (right): https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:145: // |this| may be deleted at this point. On 2014/12/14 21:04:51, mmenke wrote: > On 2014/12/14 16:45:13, pneubeck wrote: > > ^^ > > shouldn't the implementor of the callback prevent this case? it seems to be in > > general a very bad idea to delete your caller. > > otherwise, we would have to put such comments behind nearly every > callback.Run() > > where the callback was provided from some external code. > > In general, I think deletion tasks are uglier. Deleting your caller on > completion is a pretty common design pattern. There also several definitions available that you can use to make it easily readable: BrowserThread::DeleteSoon or scoped_ptr<Foo, BrowserThread::DeleteOnIOThread> Is there a more concrete reason why these aren't preferable?
Message was sent while issue was closed.
On 2014/12/15 21:11:44, pneubeck wrote: > https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... > File content/browser/ssl/ssl_client_auth_handler.cc (right): > > https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... > content/browser/ssl/ssl_client_auth_handler.cc:145: // |this| may be deleted at > this point. > On 2014/12/14 21:04:51, mmenke wrote: > > On 2014/12/14 16:45:13, pneubeck wrote: > > > ^^ > > > shouldn't the implementor of the callback prevent this case? it seems to be > in > > > general a very bad idea to delete your caller. > > > otherwise, we would have to put such comments behind nearly every > > callback.Run() > > > where the callback was provided from some external code. > > > > In general, I think deletion tasks are uglier. Deleting your caller on > > completion is a pretty common design pattern. > > There also several definitions available that you can use to make it easily > readable: > BrowserThread::DeleteSoon > or > scoped_ptr<Foo, BrowserThread::DeleteOnIOThread> > > Is there a more concrete reason why these aren't preferable? The cross thread implications of "DeleteOnIOThread" seem ugly, unless you need them. Beyond that, though, they obscure object lifecycles. If the object has pointers to any other objects it doesn't own (Which isn't the case here, admittedly), you're just asking for trouble. You also now need to make delete operations more complicated: You need to cancel pending callbacks from the object, if any, in addition to posting the delete task.
Message was sent while issue was closed.
On 2014/12/15 21:27:37, mmenke wrote: > On 2014/12/15 21:11:44, pneubeck wrote: > > > https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... > > File content/browser/ssl/ssl_client_auth_handler.cc (right): > > > > > https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... > > content/browser/ssl/ssl_client_auth_handler.cc:145: // |this| may be deleted > at > > this point. > > On 2014/12/14 21:04:51, mmenke wrote: > > > On 2014/12/14 16:45:13, pneubeck wrote: > > > > ^^ > > > > shouldn't the implementor of the callback prevent this case? it seems to > be > > in > > > > general a very bad idea to delete your caller. > > > > otherwise, we would have to put such comments behind nearly every > > > callback.Run() > > > > where the callback was provided from some external code. > > > > > > In general, I think deletion tasks are uglier. Deleting your caller on > > > completion is a pretty common design pattern. > > > > There also several definitions available that you can use to make it easily > > readable: > > BrowserThread::DeleteSoon > > or > > scoped_ptr<Foo, BrowserThread::DeleteOnIOThread> > > > > Is there a more concrete reason why these aren't preferable? > > The cross thread implications of "DeleteOnIOThread" seem ugly, unless you need > them. > > Beyond that, though, they obscure object lifecycles. If the object has pointers > to any other objects it doesn't own (Which isn't the case here, admittedly), > you're just asking for trouble. > > You also now need to make delete operations more complicated: You need to > cancel pending callbacks from the object, if any, in addition to posting the > delete task. Thanks, that makes sense. So there is not a good solution if the notification is callback from Core to Owner is synchronous. Either we have an invalid this pointer in the call stack or we have potentially invalid pointers in Core because of the delayed destruction. To make it more obvious to readers, we could at least turn the member function of Core into a static function, so that we don't have an invalid 'this' pointer in the call stack. Instead an explicit Core ptr will then be invalidated after the notification. I.e. this static function of Core would check the WeakPtr<Core>, notify Owner and afterwards the Core ptr might be invalid.
Message was sent while issue was closed.
(I'm OOO until after the holidays, so don't take my silence as ignoring this thread. I'll read through it and put together a follow-up if appropriate when I get back.)
Message was sent while issue was closed.
Apologies that took a while. Follow-up here. https://codereview.chromium.org/873723003/ https://codereview.chromium.org/795773002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/795773002/diff/20001/content/browser/loader/r... content/browser/loader/resource_loader.cc:854: ssl_client_auth_handler_.reset(); On 2014/12/14 16:45:12, pneubeck wrote: > please don't do this in a call stack containing ssl_client_auth_handler_ as a > this ptr. > you could probably reuse the existing weak_ptr_factory_ to delay the deletion. Made it a DeleteSoon. https://codereview.chromium.org/795773002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/795773002/diff/20001/content/browser/loader/r... content/browser/loader/resource_loader_unittest.cc:470: EXPECT_EQ(1, raw_ptr_to_store->request_count()); On 2014/12/14 16:45:12, pneubeck wrote: > how do you ensure that the raw_ptr_to_store is still valid (same for all similar > tests)? wouldn't it be more reliable if the ClientCertStoreStub would write its > results to some variables that outlive the previous calls for sure? Yeah, that does seem better. There's a lot of other raw_ptr_to_*s in this file, but let's resolve that one to start with. Done. https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_client_auth_handler.cc (right): https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:61: base::Bind(&SSLClientAuthHandler::Core::DidGetClientCerts, this)); On 2014/12/14 16:45:12, pneubeck wrote: > this seems still to be a rather weird ownership: > Core lives as long client_cert_store_ keeps this callback that owns the Core > object. > Core object owns client_cert_store_. > > By the definition of 'Ownership = Trigger of destruction', this is a cyclic > dependency and we should fix this. > > I looked into this a while ago and could take care of it. It's not a tiny change > however. Agreed. It's still the same issue that we had before, but pushed behind the SSLClientAuthHandler interface. So some progress, but the problem hasn't gone away. The fix you're thinking of, I assume, is to make it safe to kill client_cert_store_ while GetClientCerts is in flight? And then we'd have to sort out the craziness around cert_request_info_ and cert_request_info_->client_certs. Then we won't need this silly Core at all. I've added a TODO for now. https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:88: : core_(nullptr), On 2014/12/14 16:45:12, pneubeck wrote: > not required? Done. https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:99: SSLClientAuthHandler::~SSLClientAuthHandler() { On 2014/12/14 16:45:12, pneubeck wrote: > this should still somehow trigger a "shutdown" of the Core object. Currently, > core_ might continue with an arbitrary number of steps. I think this is also blocking on sorting out GetClientCerts' requirements, otherwise there's nothing to shut down. https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:144: callback_.Run(cert); On 2014/12/14 21:04:51, mmenke wrote: > Think this should use reset and return on the callback, for it to be cleaned up > correctly. Hrm. Does it need to be? SelectCertificate should only get used once and such. https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.cc:145: // |this| may be deleted at this point. On 2014/12/15 21:11:44, pneubeck wrote: > scoped_ptr<Foo, BrowserThread::DeleteOnIOThread> That actually doesn't do the same thing. If you're currently on the IO thread, it'll delete it immediately rather than post a task. But I've done a DeleteSoon. https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_client_auth_handler.h (right): https://codereview.chromium.org/795773002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_client_auth_handler.h:46: // Called when the user has selected a cert. On 2014/12/14 16:45:13, pneubeck wrote: > wouldn't |cert| be null if the selection was aborted? We actually don't ever plumb through aborting a selection (a bug I need to fix crbug.com/417092). Pressing cancel explicitly selects no certificate, which is a different operation. Added a note in the comment. https://codereview.chromium.org/795773002/diff/20001/content/public/browser/c... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/795773002/diff/20001/content/public/browser/c... content/public/browser/content_browser_client.h:396: // Selects a SSL client certificate and returns it to the |callback|. If no On 2014/12/14 16:45:13, pneubeck wrote: > might be worth documenting that callback can happen to be called both > synchronously and asynchronously. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
