Initialize NSS with databases....

Initialize NSS with databases.

BUG=12348
TEST=rm -rf ~/.pki/nssdb; visit https://bugs.webkit.org/ then ~/.pki/nssdb is created.


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

[wtc, 2009-05-21 14:29:04]

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

http://codereview.chromium.org/115626/diff/1/2#newcode26
Line 26: const char *home = getenv("HOME");
Nit: put * next to char instead of home.

http://codereview.chromium.org/115626/diff/1/2#newcode31
Line 31: CHECK(file_util::CreateDirectory(dir));
We should fail gracefully if file_util::CreateDirectory(dir).
CHECK or DCHECK should be used to detect bugs in our code.
It is not a bug if we can't create the ~/.pki/nssdb directory.

http://codereview.chromium.org/115626/diff/1/2#newcode32
Line 32: return dir.value();
We need to add "sql:" to the beginning of dir.value() so
that we use the sqlite version of NSS databases, which
can be shared by multiple processes safely.

By default, NSS uses Berkeley DB 1.85 databases, which
can be used by a single process at a time.

http://codereview.chromium.org/115626/diff/1/2#newcode58
Line 58: status = NSS_Initialize(database_dir.c_str(), "", "", SECMOD_DB, 0);
Use NSS_InitReadWrite instead.  See
https://wiki.mozilla.org/NSS_Shared_DB_And_LINUX#Application_Programmer.27s_S...

NSS_Initialize should only be used if you need to specify
non-default flags.

http://codereview.chromium.org/115626/diff/1/2#newcode70
Line 70: NOTREACHED() << "Error calling NSS_Initialize: " << buffer;
We can just say "Error initializing NSS: ".

[ukai, 2009-05-22 06:16:51]

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

http://codereview.chromium.org/115626/diff/1/2#newcode26
Line 26: const char *home = getenv("HOME");
On 2009/05/21 14:29:04, wtc wrote:
> Nit: put * next to char instead of home.

Done.

http://codereview.chromium.org/115626/diff/1/2#newcode31
Line 31: CHECK(file_util::CreateDirectory(dir));
On 2009/05/21 14:29:04, wtc wrote:
> We should fail gracefully if file_util::CreateDirectory(dir).
> CHECK or DCHECK should be used to detect bugs in our code.
> It is not a bug if we can't create the ~/.pki/nssdb directory.

Done.

http://codereview.chromium.org/115626/diff/1/2#newcode32
Line 32: return dir.value();
On 2009/05/21 14:29:04, wtc wrote:
> We need to add "sql:" to the beginning of dir.value() so
> that we use the sqlite version of NSS databases, which
> can be shared by multiple processes safely.
> 
> By default, NSS uses Berkeley DB 1.85 databases, which
> can be used by a single process at a time.

Ok. Add "sql:" when calling NSS_Initialize later.

http://codereview.chromium.org/115626/diff/1/2#newcode58
Line 58: status = NSS_Initialize(database_dir.c_str(), "", "", SECMOD_DB, 0);
On 2009/05/21 14:29:04, wtc wrote:
> Use NSS_InitReadWrite instead.  See
>
https://wiki.mozilla.org/NSS_Shared_DB_And_LINUX#Application_Programmer.27s_S...
> 
> NSS_Initialize should only be used if you need to specify
> non-default flags.

Done.

http://codereview.chromium.org/115626/diff/1/2#newcode70
Line 70: NOTREACHED() << "Error calling NSS_Initialize: " << buffer;
On 2009/05/21 14:29:04, wtc wrote:
> We can just say "Error initializing NSS: ".

Done.

[wtc, 2009-05-22 14:45:39]

LGTM.  Thanks!

http://codereview.chromium.org/115626/diff/1007/7
File base/nss_init.cc (right):

http://codereview.chromium.org/115626/diff/1007/7#newcode62
Line 62: // Use "sql:" which can be shared by multiprocess safely.
Nit: "multiprocess" => "multiple processes"

http://codereview.chromium.org/115626/diff/1007/7#newcode66
Line 66: LOG(WARNING) << "$HOME is not set.  Initialize NSS without using a "
"$HOME is not set." is not the only reason the database
directory is empty.  Another reason is that we can't create
the database directory.

So perhaps we should move the "$HOME is not set." message
to the GetDefaultConfigDirectory() function?

[ukai, 2009-05-25 02:14:06]

Thanks for review!

http://codereview.chromium.org/115626/diff/1007/7
File base/nss_init.cc (right):

http://codereview.chromium.org/115626/diff/1007/7#newcode62
Line 62: // Use "sql:" which can be shared by multiprocess safely.
On 2009/05/22 14:45:39, wtc wrote:
> Nit: "multiprocess" => "multiple processes"

Done.

http://codereview.chromium.org/115626/diff/1007/7#newcode66
Line 66: LOG(WARNING) << "$HOME is not set.  Initialize NSS without using a "
On 2009/05/22 14:45:39, wtc wrote:
> "$HOME is not set." is not the only reason the database
> directory is empty.  Another reason is that we can't create
> the database directory.
> 
> So perhaps we should move the "$HOME is not set." message
> to the GetDefaultConfigDirectory() function?

Done.

[ukai, 2009-05-25 06:27:09]

Patch laneded.
But it causes memory leak.  Does NSS_InitReadWrite have memory leak bug?

On 2009/05/25 02:14:06, ukai wrote:
> Thanks for review!
> 
> http://codereview.chromium.org/115626/diff/1007/7
> File base/nss_init.cc (right):
> 
> http://codereview.chromium.org/115626/diff/1007/7#newcode62
> Line 62: // Use "sql:" which can be shared by multiprocess safely.
> On 2009/05/22 14:45:39, wtc wrote:
> > Nit: "multiprocess" => "multiple processes"
> 
> Done.
> 
> http://codereview.chromium.org/115626/diff/1007/7#newcode66
> Line 66: LOG(WARNING) << "$HOME is not set.  Initialize NSS without using a "
> On 2009/05/22 14:45:39, wtc wrote:
> > "$HOME is not set." is not the only reason the database
> > directory is empty.  Another reason is that we can't create
> > the database directory.
> > 
> > So perhaps we should move the "$HOME is not set." message
> > to the GetDefaultConfigDirectory() function?
> 
> Done.

[wtc, 2009-05-26 21:47:20]

On 2009/05/25 06:27:09, ukai wrote:
> Patch laneded.
> But it causes memory leak.  Does NSS_InitReadWrite have memory leak bug?

NSS_InitReadWrite may have memory leaks, but I won't know for
sure unless I look at the call stacks of the memory leaks.
I'll be happy to look at the leaks for you and help get them
fixed.  If the leaks are one-time leaks, it is fine to ignore
them in our Purify filters.