Enable Chrome OS to load the user's nssdb later.

Enable Chrome OS to load the user's nssdb later.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=45954

[Chris Masone, 2010-04-20 22:57:58]

on all non-chromiumos platforms, this should be a no-op.  on chromiumos, this
should delay the creation of the user's local nssdb.  A separate change creates
the db when the NotificationType::LOGIN_USER_CHANGED event fires.

BUG=None

TEST=base_unittests, net_unittests pass

[wtc, 2010-04-21 18:25:12]

Chris,

I suggest we do this:
- Leave the behavior of EnsureNSSInit() unchanged.
- Add a EnsureNSSInitNoDB() function that initializes NSS
  without local databases.  Define this function without
  the OS_CHROMEOS ifdef because I believe with refactoring
  we can add this function without increasing the code size
  much.

The following comments were written before I figured out
what you need.

http://codereview.chromium.org/1730001/diff/1/2
File base/nss_util.cc (right):

http://codereview.chromium.org/1730001/diff/1/2#newcode115
base/nss_util.cc:115: #if !defined(OS_CHROMEOS)
This changes the behavior of EnsureNSSInit() for Chrome OS
(it does less work on Chrome OS than on regular Linux),
but it's not described in the comments for EnsureNSSInit()
in the header file.

It's confusing for EnsureNSSInit() to have subtle differences
between Chrome OS and regular Linux.  We should come up
with a different solution.

http://codereview.chromium.org/1730001/diff/1/3
File base/nss_util.h (right):

http://codereview.chromium.org/1730001/diff/1/3#newcode28
base/nss_util.h:28: // Initialize NSS if it isn't already initialized, and for
This comment is incomplete.

Nit: rename this function EnsureNSSUserInit.  We use the
"EnsureFooInit" names for several functions that initialize
something if it isn't already initialized.

[Chris Masone, 2010-04-21 18:44:37]

On 2010/04/21 18:25:12, wtc wrote:
> Chris,
> 
> I suggest we do this:
> - Leave the behavior of EnsureNSSInit() unchanged.
> - Add a EnsureNSSInitNoDB() function that initializes NSS
>   without local databases.  Define this function without
>   the OS_CHROMEOS ifdef because I believe with refactoring
>   we can add this function without increasing the code size
>   much.

So, I don't know that that works for me, because I do want to enable the usage
of a real user DB after we've logged a person in and have access to his
encrypted home directory.  So, basically, I'd want EnsureNSSInitNoDB() on Chrome
OS before login, and EnsureNSSInit() on Chrome OS after login...in the scheme
you suggest, how would I do this?

> 
> The following comments were written before I figured out
> what you need.
> 
> http://codereview.chromium.org/1730001/diff/1/2
> File base/nss_util.cc (right):
> 
> http://codereview.chromium.org/1730001/diff/1/2#newcode115
> base/nss_util.cc:115: #if !defined(OS_CHROMEOS)
> This changes the behavior of EnsureNSSInit() for Chrome OS
> (it does less work on Chrome OS than on regular Linux),
> but it's not described in the comments for EnsureNSSInit()
> in the header file.
> 
> It's confusing for EnsureNSSInit() to have subtle differences
> between Chrome OS and regular Linux.  We should come up
> with a different solution.
> 
> http://codereview.chromium.org/1730001/diff/1/3
> File base/nss_util.h (right):
> 
> http://codereview.chromium.org/1730001/diff/1/3#newcode28
> base/nss_util.h:28: // Initialize NSS if it isn't already initialized, and for
> This comment is incomplete.
> 
> Nit: rename this function EnsureNSSUserInit.  We use the
> "EnsureFooInit" names for several functions that initialize
> something if it isn't already initialized.

[wtc, 2010-04-21 19:02:43]

I see.  I don't have a good solution, other than separating the
creation of the $HOME/.pki directory (the
file_util::CreateDirectory(dir) call in GetDefaultConfigDirectory())
from EnsureNSSInit().  Then EnsureNSSInit() will automatically
initialize with no DB if the $HOME/.pki directory doesn't exist.

Would that work for you?

This will require modifying all the programs (including the test
programs).  On the other hand, some of the unit tests should pass
when NSS is initialized with no DB.  So this may not be as bad as
I thought.

[Chris Masone, 2010-04-26 19:45:20]

Ok!  This has the approach we discussed last week, in which I store the slot of
the newly-opened DB in a member of NSSInitSingleton and then provide a util
function to return the appropriate slot on all platforms.

On 2010/04/21 19:02:43, wtc wrote:
> I see.  I don't have a good solution, other than separating the
> creation of the $HOME/.pki directory (the
> file_util::CreateDirectory(dir) call in GetDefaultConfigDirectory())
> from EnsureNSSInit().  Then EnsureNSSInit() will automatically
> initialize with no DB if the $HOME/.pki directory doesn't exist.
> 
> Would that work for you?
> 
> This will require modifying all the programs (including the test
> programs).  On the other hand, some of the unit tests should pass
> when NSS is initialized with no DB.  So this may not be as bad as
> I thought.

[wtc, 2010-04-27 00:14:29]

Hi Chris,

Thanks for the patch.  I will review your patch again
tomorrow morning (got a headache now).  Here are my
first comments.

http://codereview.chromium.org/1730001/diff/22001/23004
File base/nss_slot_util.h (right):

http://codereview.chromium.org/1730001/diff/22001/23004#newcode17
base/nss_slot_util.h:17: PK11SlotInfo* NSS_GetDefaultKeySlot();
We only need NSS_GetDefaultKeySlot().  Please remove the
underscore '_' from the function name to conform to the
Style Guide.  How about GetDefaultNSSKeySlot()?

We don't need NSS_GetDefaultSlot().  We can continue to
use PK11_GetInternalSlot().

http://codereview.chromium.org/1730001/diff/22001/23006
File base/nss_util.h (right):

http://codereview.chromium.org/1730001/diff/22001/23006#newcode29
base/nss_util.h:29: void NSSPivot();
We need a better name for this function.  I have trouble
understanding what "NSS pivot" or "Pivot NSS" means.

[Chris Masone, 2010-04-27 15:42:26]

http://codereview.chromium.org/1730001/diff/22001/23004
File base/nss_slot_util.h (right):

http://codereview.chromium.org/1730001/diff/22001/23004#newcode17
base/nss_slot_util.h:17: PK11SlotInfo* NSS_GetDefaultKeySlot();
On 2010/04/27 00:14:29, wtc wrote:
> We only need NSS_GetDefaultKeySlot().  Please remove the
> underscore '_' from the function name to conform to the
> Style Guide.  How about GetDefaultNSSKeySlot()?
> 
> We don't need NSS_GetDefaultSlot().  We can continue to
> use PK11_GetInternalSlot().

Done.

http://codereview.chromium.org/1730001/diff/22001/23006
File base/nss_util.h (right):

http://codereview.chromium.org/1730001/diff/22001/23006#newcode29
base/nss_util.h:29: void NSSPivot();
On 2010/04/27 00:14:29, wtc wrote:
> We need a better name for this function.  I have trouble
> understanding what "NSS pivot" or "Pivot NSS" means.

How about OpenPersistentNSSDB?

[wtc, 2010-04-27 18:11:26]

LGTM.  Please note the comments and suggested changes below.

http://codereview.chromium.org/1730001/diff/22001/23006
File base/nss_util.h (right):

http://codereview.chromium.org/1730001/diff/22001/23006#newcode29
base/nss_util.h:29: void NSSPivot();
On 2010/04/27 15:42:26, cmasone wrote:
> 
> How about OpenPersistentNSSDB?

"OpenPersistentNSSDB" sounds good.  Or just "OpenNSSDB"
because there is no temporary NSS DB.

http://codereview.chromium.org/1730001/diff/31001/32005
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/1730001/diff/31001/32005#newcode17
base/crypto/rsa_private_key_nss.cc:17: #include "base/nss_slot_util.h"
Please remove the #include "base/nss_slot_util.h" line and
then remove rsa_private_key_nss.cc from this changelist.

http://codereview.chromium.org/1730001/diff/31001/32004
File base/nss_slot_util.h (right):

http://codereview.chromium.org/1730001/diff/31001/32004#newcode10
base/nss_slot_util.h:10: // These functions return a type defined in an NSS
header, and so cannot be
Yeah... I should fix this dependency problem some time.  I
hate it that nss_util.h can't include any NSS header.

Nit: can we come up with a more generic name for this header
so that it can be used for other NSS utility functions that
are unrelated to PK11SlotInfo?  Perhaps something like
nss_util_internal.h or nss_util2.h? :-)

http://codereview.chromium.org/1730001/diff/31001/32001
File base/nss_util.cc (right):

http://codereview.chromium.org/1730001/diff/31001/32001#newcode47
base/nss_util.cc:47: // On chromeos, return a read-only directory with fake
testing certs (which
It would be nice to elaborate on the ChromeOS case.  For
example, what are the fake testing certs for?  Are they root
CA certs??  If this read-only directory doesn't exist on
non-testing images, what happens when you pass this directory
to NSS_Init()?  Does NSS_Init() fail?

http://codereview.chromium.org/1730001/diff/31001/32001#newcode50
base/nss_util.cc:50: #if !defined(OS_CHROMEOS)
Nit: use #if defined(OS_CHROMEOS), to avoid the ! operator.
This requires reordering the two cases.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode53
base/nss_util.cc:53: static const char read_only_certdb[] =
"/etc/fake_root_ca/nssdb";
Nit: this probably should use the naming convention for
constants:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names

http://codereview.chromium.org/1730001/diff/31001/32001#newcode57
base/nss_util.cc:57: 
Nit: just one blank line between functions.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode142
base/nss_util.cc:142: #endif  // defined(OS_CHROMEOS)
Nit: omit the "// defined(OS_CHROMEOS)" comments for short
ifdef blocks to reduce the visual clutter.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode201
base/nss_util.cc:201: StringPrintf("configDir='%s' tokenDescription='User NSS
database'",
Nit: let's describe this token as "Real NSS database".

http://codereview.chromium.org/1730001/diff/31001/32001#newcode218
base/nss_util.cc:218: else
Nit: omit "else" after a return statement.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode223
base/nss_util.cc:223: PK11SlotInfo* db_slot_;
Please document db_slot_.  It overrides the internal key slot
if it is not null.  Perhaps we can call it real_db_slot_ (as
opposed to the "fake" certdb directory).

http://codereview.chromium.org/1730001/diff/31001/32001#newcode227
base/nss_util.cc:227: SECStatus ShutdownNSS() {
The ShutdownNSS method is not being used.  We should either
remove it or use it in the destructor.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode278
base/nss_util.cc:278: // Though this function is declared in nss_slot_util.h,
its
It is best to consider nss_util.h and nss_slot_util.h,
combined, as the header for this .cc file.  We need to
split the header into two files to avoid a dependency problem.

http://codereview.chromium.org/1730001/diff/31001/32006
File base/nss_util.h (right):

http://codereview.chromium.org/1730001/diff/31001/32006#newcode28
base/nss_util.h:28: // Pivot to the user's nssdb that's stored inside his
encrypted home directory.
Please update the comment to reflect the new function name.

http://codereview.chromium.org/1730001/diff/31001/32007
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/1730001/diff/31001/32007#newcode69
chrome/browser/chromeos/login/login_utils.cc:69: LoginUtilsWrapper() :
ptr_(NULL) {
Nit: the default scoped_ptr constructor initializes the
internal pointer to NULL, so you don't need the ptr_(NULL)
initializer.

http://codereview.chromium.org/1730001/diff/31001/32007#newcode73
chrome/browser/chromeos/login/login_utils.cc:73: if (!ptr_.get())
Please make sure LoginUtilsWrapper is used on a single thread.

http://codereview.chromium.org/1730001/diff/31001/32002
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/1730001/diff/31001/32002#newcode34
net/base/keygen_handler_unittest.cc:34: base::OpenPersistentNSSDB();
Ideally we should call base::OpenPersistentNSSDB() in
the constructor or the SetUp() method of the unit test's
class.  Do you know how to do that?

You can look at net/http/http_network_transaction_unittest.cc
and net/base/file_stream_unittest.cc as examples.  This
seems to require changing TEST to TEST_F for the unit
tests.

[Chris Masone, 2010-04-28 04:26:12]

I ran this through the trybots again after addressing your comments.  I'll land
it on thursday AM so I can monitor the situation throughout the day (I'm out
tomorrow).

I also won't be in friday, so if it winds up needing to be reverted, please feel
free.

http://codereview.chromium.org/1730001/diff/31001/32005
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/1730001/diff/31001/32005#newcode17
base/crypto/rsa_private_key_nss.cc:17: #include "base/nss_slot_util.h"
On 2010/04/27 18:11:26, wtc wrote:
> Please remove the #include "base/nss_slot_util.h" line and
> then remove rsa_private_key_nss.cc from this changelist.

Done.

http://codereview.chromium.org/1730001/diff/31001/32004
File base/nss_slot_util.h (right):

http://codereview.chromium.org/1730001/diff/31001/32004#newcode10
base/nss_slot_util.h:10: // These functions return a type defined in an NSS
header, and so cannot be
On 2010/04/27 18:11:26, wtc wrote:
> Yeah... I should fix this dependency problem some time.  I
> hate it that nss_util.h can't include any NSS header.
> 
> Nit: can we come up with a more generic name for this header
> so that it can be used for other NSS utility functions that
> are unrelated to PK11SlotInfo?  Perhaps something like
> nss_util_internal.h or nss_util2.h? :-)

Done.

http://codereview.chromium.org/1730001/diff/31001/32001
File base/nss_util.cc (right):

http://codereview.chromium.org/1730001/diff/31001/32001#newcode47
base/nss_util.cc:47: // On chromeos, return a read-only directory with fake
testing certs (which
On 2010/04/27 18:11:26, wtc wrote:
> It would be nice to elaborate on the ChromeOS case.  For
> example, what are the fake testing certs for?  Are they root
> CA certs??  If this read-only directory doesn't exist on
> non-testing images, what happens when you pass this directory
> to NSS_Init()?  Does NSS_Init() fail?

Done.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode50
base/nss_util.cc:50: #if !defined(OS_CHROMEOS)
On 2010/04/27 18:11:26, wtc wrote:
> Nit: use #if defined(OS_CHROMEOS), to avoid the ! operator.
> This requires reordering the two cases.

Done.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode53
base/nss_util.cc:53: static const char read_only_certdb[] =
"/etc/fake_root_ca/nssdb";
On 2010/04/27 18:11:26, wtc wrote:
> Nit: this probably should use the naming convention for
> constants:
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names

Done.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode57
base/nss_util.cc:57: 
On 2010/04/27 18:11:26, wtc wrote:
> Nit: just one blank line between functions.

Done.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode142
base/nss_util.cc:142: #endif  // defined(OS_CHROMEOS)
On 2010/04/27 18:11:26, wtc wrote:
> Nit: omit the "// defined(OS_CHROMEOS)" comments for short
> ifdef blocks to reduce the visual clutter.

Done.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode201
base/nss_util.cc:201: StringPrintf("configDir='%s' tokenDescription='User NSS
database'",
On 2010/04/27 18:11:26, wtc wrote:
> Nit: let's describe this token as "Real NSS database".

Done.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode218
base/nss_util.cc:218: else
On 2010/04/27 18:11:26, wtc wrote:
> Nit: omit "else" after a return statement.

Done.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode223
base/nss_util.cc:223: PK11SlotInfo* db_slot_;
On 2010/04/27 18:11:26, wtc wrote:
> Please document db_slot_.  It overrides the internal key slot
> if it is not null.  Perhaps we can call it real_db_slot_ (as
> opposed to the "fake" certdb directory).

Done.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode227
base/nss_util.cc:227: SECStatus ShutdownNSS() {
On 2010/04/27 18:11:26, wtc wrote:
> The ShutdownNSS method is not being used.  We should either
> remove it or use it in the destructor.

Done.

http://codereview.chromium.org/1730001/diff/31001/32001#newcode278
base/nss_util.cc:278: // Though this function is declared in nss_slot_util.h,
its
On 2010/04/27 18:11:26, wtc wrote:
> It is best to consider nss_util.h and nss_slot_util.h,
> combined, as the header for this .cc file.  We need to
> split the header into two files to avoid a dependency problem.

Ok.  I deleted this comment, then.

http://codereview.chromium.org/1730001/diff/31001/32006
File base/nss_util.h (right):

http://codereview.chromium.org/1730001/diff/31001/32006#newcode28
base/nss_util.h:28: // Pivot to the user's nssdb that's stored inside his
encrypted home directory.
On 2010/04/27 18:11:26, wtc wrote:
> Please update the comment to reflect the new function name.

Done.

http://codereview.chromium.org/1730001/diff/31001/32007
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/1730001/diff/31001/32007#newcode69
chrome/browser/chromeos/login/login_utils.cc:69: LoginUtilsWrapper() :
ptr_(NULL) {
On 2010/04/27 18:11:26, wtc wrote:
> Nit: the default scoped_ptr constructor initializes the
> internal pointer to NULL, so you don't need the ptr_(NULL)
> initializer.

Done.

http://codereview.chromium.org/1730001/diff/31001/32007#newcode73
chrome/browser/chromeos/login/login_utils.cc:73: if (!ptr_.get())
On 2010/04/27 18:11:26, wtc wrote:
> Please make sure LoginUtilsWrapper is used on a single thread.

Is putting a lock around this sufficient?

http://codereview.chromium.org/1730001/diff/31001/32002
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/1730001/diff/31001/32002#newcode34
net/base/keygen_handler_unittest.cc:34: base::OpenPersistentNSSDB();
On 2010/04/27 18:11:26, wtc wrote:
> Ideally we should call base::OpenPersistentNSSDB() in
> the constructor or the SetUp() method of the unit test's
> class.  Do you know how to do that?
> 
> You can look at net/http/http_network_transaction_unittest.cc
> and net/base/file_stream_unittest.cc as examples.  This
> seems to require changing TEST to TEST_F for the unit
> tests.

Done.

[wtc, 2010-04-29 21:35:28]

http://codereview.chromium.org/1730001/diff/31001/32007
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/1730001/diff/31001/32007#newcode73
chrome/browser/chromeos/login/login_utils.cc:73: if (!ptr_.get())
On 2010/04/28 04:26:13, cmasone wrote:
>
> Is putting a lock around this sufficient?

Yes, it is.  I am not sure if multiple threads may call
get() on the same LoginUtilsWrapper object, which is why
I asked you to doublecheck.

Why don't you call new LoginUtilsImpl in the constructor,
as the original code did?

[Chris Masone, 2010-04-29 21:56:26]

On Thu, Apr 29, 2010 at 2:35 PM, <wtc@chromium.org> wrote:

>
> http://codereview.chromium.org/1730001/diff/31001/32007
> File chrome/browser/chromeos/login/login_utils.cc (right):
>
> http://codereview.chromium.org/1730001/diff/31001/32007#newcode73
> chrome/browser/chromeos/login/login_utils.cc:73: if (!ptr_.get())
> On 2010/04/28 04:26:13, cmasone wrote:
>
>  Is putting a lock around this sufficient?
>>
>
> Yes, it is.  I am not sure if multiple threads may call
> get() on the same LoginUtilsWrapper object, which is why
> I asked you to doublecheck.
>
> Why don't you call new LoginUtilsImpl in the constructor,
> as the original code did?
>
>
It was creating a problem in the browser_tests.  They create a
LoginUtilsWrapper so that a Mock can be stuck into it -- but the act of
creating the LoginUtilsWrapper created a LoginUtilsImpl, which tries to
register an Observer.  But, whatever thread they were creating this object
on had no NotificationRegistrar, so it was crashing.


>
> http://codereview.chromium.org/1730001/show
>