Make ONCCertificateImporter async.

Make ONCCertificateImporter async.

This prepares for the new CertDatabase keyed service, which will have stricter threading restrictions. https://codereview.chromium.org/419013003/

Before, ONCCertificateImporter accessed the NSSCertDatabase from the UI thread and blocked on certificate store operations.

Now, the import itself is asychronous and calls back on completion.

While there, this also removes the GMock of the importer.

BUG=413219
Committed: https://crrev.com/bc656c0e7b7bd67fb28e5a880d21b9510ebd3e3a
Cr-Commit-Position: refs/heads/master@{#295534}

[pneubeck (no reviews), 2014-09-12 12:07:13]

Patchset #1 (id:1) has been deleted

[pneubeck (no reviews), 2014-09-12 14:38:48]

Patchset #1 (id:20001) has been deleted

[pneubeck (no reviews), 2014-09-12 14:41:20]

pneubeck@chromium.org changed reviewers:
+ joaodasilva@chromium.org

[pneubeck (no reviews), 2014-09-12 14:41:20]

ptal

[pneubeck (no reviews), 2014-09-12 17:19:47]

Patchset #2 (id:60001) has been deleted

[pneubeck (no reviews), 2014-09-12 17:23:31]

Patchset #2 (id:80001) has been deleted

[pneubeck (no reviews), 2014-09-12 17:26:10]

Patchset #2 (id:90012) has been deleted

[Joao da Silva, 2014-09-15 12:38:24]

Looks good to me. The external interface is fine but the callbacks-in-callbacks
code can be improved.

https://codereview.chromium.org/547553005/diff/110001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc
(right):

https://codereview.chromium.org/547553005/diff/110001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:134:
unsigned int call_count_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/mo...
File chromeos/network/onc/mock_certificate_importer.h (left):

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/mo...
chromeos/network/onc/mock_certificate_importer.h:31: #endif  //
CHROMEOS_NETWORK_ONC_MOCK_CERTIFICATE_IMPORTER_H_
The file is marked as "M" instead of "D" by rietveld

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer.h (right):

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer.h:38: // Never calls
|done_callback| after this import is destructed.
import -> importer

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl.cc (right):

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl.cc:75: target_nssdb_));
This is essentially PostTaskAndReply, can you use that instead?

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl.h (right):

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl.h:45: explicit
CertificateImporterImpl(
no need for explicit

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl.h:46: const
scoped_refptr<base::SequencedTaskRunner>& task_runner,
Call it io_task_runner or net_task_runner to make that clearer

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl.h:103:
scoped_refptr<base::SequencedTaskRunner> task_runner_;
io_task_runner_ or net_task_runner_?

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl_unittest.cc (right):

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:69: virtual void
SetUp() {
OVERRIDE

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:88: task_runner_
= NULL;
Do this in TearDown(). If something fails there then the test will fail.

[pneubeck (no reviews), 2014-09-17 12:43:36]

Patchset #3 (id:130001) has been deleted

[pneubeck (no reviews), 2014-09-17 12:44:22]

https://codereview.chromium.org/547553005/diff/110001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc
(right):

https://codereview.chromium.org/547553005/diff/110001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:134:
unsigned int call_count_;
On 2014/09/15 12:38:24, Joao da Silva wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/mo...
File chromeos/network/onc/mock_certificate_importer.h (left):

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/mo...
chromeos/network/onc/mock_certificate_importer.h:31: #endif  //
CHROMEOS_NETWORK_ONC_MOCK_CERTIFICATE_IMPORTER_H_
On 2014/09/15 12:38:24, Joao da Silva wrote:
> The file is marked as "M" instead of "D" by rietveld

Acknowledged.

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer.h (right):

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer.h:38: // Never calls
|done_callback| after this import is destructed.
On 2014/09/15 12:38:24, Joao da Silva wrote:
> import -> importer

Done.

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl.cc (right):

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl.cc:75: target_nssdb_));
On 2014/09/15 12:38:24, Joao da Silva wrote:
> This is essentially PostTaskAndReply, can you use that instead?

Indeed, in this case something like

PostTaskAndReplyWithResult(
      io_task_runner_.get(),
      FROM_HERE,
      base::Bind(&ParseAndStoreCertificates,
                 source,
                 base::Owned(certificates.DeepCopy()),
                 target_nssdb_),
      base::Bind(&CertificateImporterImpl::RunDoneCallback,
                 weak_factory_.GetWeakPtr(),
                 done_callback));

would work.
The problem is, I have a follow up where I have to run an async function on the
IO thread to obtain the actual NSSCertDB and that is sadly not compatible with
PostTaskAndReply :-/

I went ahead and documented the individual Bind calls. I hope that makes it
sufficiently clear.

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl.h (right):

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl.h:45: explicit
CertificateImporterImpl(
On 2014/09/15 12:38:24, Joao da Silva wrote:
> no need for explicit

Done.

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl.h:46: const
scoped_refptr<base::SequencedTaskRunner>& task_runner,
On 2014/09/15 12:38:24, Joao da Silva wrote:
> Call it io_task_runner or net_task_runner to make that clearer

Done.

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl.h:103:
scoped_refptr<base::SequencedTaskRunner> task_runner_;
On 2014/09/15 12:38:24, Joao da Silva wrote:
> io_task_runner_ or net_task_runner_?

Done.

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl_unittest.cc (right):

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:69: virtual void
SetUp() {
On 2014/09/15 12:38:24, Joao da Silva wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/547553005/diff/110001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:88: task_runner_
= NULL;
On 2014/09/15 12:38:24, Joao da Silva wrote:
> Do this in TearDown(). If something fails there then the test will fail.

Done.

[pneubeck (no reviews), 2014-09-17 12:47:35]

pneubeck@chromium.org changed reviewers:
+ eroman@chromium.org

[pneubeck (no reviews), 2014-09-17 12:47:36]

@Eric, please owner review net_internals_ui.cc . Thanks.

https://codereview.chromium.org/547553005/diff/150001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/547553005/diff/150001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1478:
GetNSSCertDatabaseForProfile(
Note that this async call is supposed to go away in a follow up:
https://codereview.chromium.org/419013003/

[pneubeck (no reviews), 2014-09-17 12:48:15]

On 2014/09/17 12:47:36, pneubeck wrote:
> @Eric, please owner review net_internals_ui.cc . Thanks.
> 
>
https://codereview.chromium.org/547553005/diff/150001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):
> 
>
https://codereview.chromium.org/547553005/diff/150001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1478:
> GetNSSCertDatabaseForProfile(
> Note that this async call is supposed to go away in a follow up:
> https://codereview.chromium.org/419013003/

oh, and chromeos.gyp please.

[Joao da Silva, 2014-09-17 13:00:46]

lgtm

[eroman, 2014-09-18 18:35:14]

net_ internals LGTM

https://codereview.chromium.org/547553005/diff/170001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/547553005/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:262: // |error|
contains earlier errors during this import.
|error| --> |previous_error|

https://codereview.chromium.org/547553005/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1422: LOG(ERROR) <<
error;
Is logging necessary or useful here? (given that the UI will reflect it)

https://codereview.chromium.org/547553005/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1464: LOG(ERROR) <<
error;
Same comment about logging.

[pneubeck (no reviews), 2014-09-18 18:52:03]

https://codereview.chromium.org/547553005/diff/170001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/547553005/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:262: // |error|
contains earlier errors during this import.
On 2014/09/18 18:35:13, eroman wrote:
> |error| --> |previous_error|

Done.

https://codereview.chromium.org/547553005/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1422: LOG(ERROR) <<
error;
On 2014/09/18 18:35:13, eroman wrote:
> Is logging necessary or useful here? (given that the UI will reflect it)

Done.

https://codereview.chromium.org/547553005/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1464: LOG(ERROR) <<
error;
On 2014/09/18 18:35:13, eroman wrote:
> Same comment about logging.

Done.

[pneubeck (no reviews), 2014-09-18 18:52:08]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-09-18 18:54:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/547553005/190001

[commit-bot: I haz the power, 2014-09-18 19:51:41]

Committed patchset #5 (id:190001) as f08303014b165f6013fe33198cd798ebd9a4e925

[commit-bot: I haz the power, 2014-09-18 19:52:42]

Patchset 5 (id:??) landed as
https://crrev.com/bc656c0e7b7bd67fb28e5a880d21b9510ebd3e3a
Cr-Commit-Position: refs/heads/master@{#295534}

[eugenis, 2014-09-19 09:14:47]

A revert of this CL (patchset #5 id:190001) has been created in
https://codereview.chromium.org/580283005/ by eugenis@chromium.org.

The reason for reverting is: Use-after-free.
https://code.google.com/p/chromium/issues/detail?id=415916
.