Linux: Fix NSSDecryptor to import Firefox passwords from the user database correctly.

Linux: Fix NSSDecryptor to decrypt Firefox passwords using correct user database.
Note that we're deliberately leaving NSS user databases open until NSS shutdown since closing and reopening it does not work correctly (NSS bug).
Also, enable the firefox_importer_unittest.cc for linux.  Move out some tests to firefox_profile_unittest.cc.
Fix lock file deletion in the posix implementation of FirefoxProfileLock.
BUG=http://crbug.com/17490
TEST=Import bookmark and settings, verify that passwords are indeed imported.

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

[willchan no longer on Chromium, 2009-07-24 00:45:00]

kuchhal: please review the FirefoxProfileLock related files
wtc: Please review everything else.

[wtc, 2009-07-24 19:01:45]

LGTM after fixing the following issues.  Thanks a lot for
fixing this bug and moving the FirefoxProfileLock unit tests
to the right file.

Some of the things you said in the Description of this CL
are not accurate.

1. Firefox passwords are not saved in the user database.
Only the key used to encrypt Firefox passwords is saved
in the user database.  Perhaps you can say:
  Fix NSSDecryptor to decrypt Firefox passwords using the
  correct user database.

2. We're not leaking NSS user databases because they
will be closed when NSS is shut down.  We're just
leaving the NSS user databases open.  The downside is
that there is a limit on the number of NSS user databases
that we can open, so we may hit that limit if the user
keeps importing Firefox settings.  Have you tried that?

3. I don't think we should delete the lock file used
for fcntl locking.  More on this below.

http://codereview.chromium.org/160077/diff/1/2
File chrome/browser/importer/firefox_importer_unittest.cc (right):

http://codereview.chromium.org/160077/diff/1/2#newcode12
Line 12: #include "chrome/browser/importer/firefox_profile_lock.h"
Can we also remove the firefox_profile_lock.h inclusion?

http://codereview.chromium.org/160077/diff/1/4
File chrome/browser/importer/firefox_profile_lock_posix.cc (right):

http://codereview.chromium.org/160077/diff/1/4#newcode85
Line 85: file_util::Delete(lock_file_, false);
We must use the same file locking protocol that Firefox
uses.  So are you sure you want to change this file?

I think it is fine to leave behind the lock file when
we do fcntl locking.  So perhaps we just need the "if"
part of the change but not the "else" part.

http://codereview.chromium.org/160077/diff/1/5
File chrome/browser/importer/firefox_profile_lock_unittest.cc (right):

http://codereview.chromium.org/160077/diff/1/5#newcode95
Line 95: // This is broken on POSIX since the same process is allowed to
reacquire a
We should file a bug about this problem...

http://codereview.chromium.org/160077/diff/1/7
File chrome/browser/importer/nss_decryptor_linux.cc (right):

http://codereview.chromium.org/160077/diff/1/7#newcode15
Line 15: NSSDecryptor::NSSDecryptor() : is_nss_initialized_(false) {}
The constructor should initialize db_slot_ to NULL.

http://codereview.chromium.org/160077/diff/1/7#newcode18
Line 18: // Deliberately leak the user db, just in case we need to open more
than
Let's change "leak" to "don't close" or "leave ... open".

http://codereview.chromium.org/160077/diff/1/7#newcode34
Line 34: return db_slot_;
Nit: it'd be more clear to say
  return db_slot_ != NULL;
because this function returns bool.

http://codereview.chromium.org/160077/diff/1/7#newcode37
Line 37: // This method is based on some Firefox code in
Nit: change "Firefox" to "NSS".

Please explain why we need to copy the NSS code (because
PK11SDR_Decrypt is hardcoded to use the internal key slot
for decryption, but we need to decrypt using another slot).

http://codereview.chromium.org/160077/diff/1/8
File chrome/browser/importer/nss_decryptor_linux.h (right):

http://codereview.chromium.org/160077/diff/1/8#newcode44
Line 44: SECStatus PK11SDR_Decrypt_Slot(
Nit: name this method PK11SDR_DecryptWithSlot and make
PK11SlotInfo* slot the first parameter.

http://codereview.chromium.org/160077/diff/1/9
File chrome/browser/importer/nss_decryptor_mac.h (right):

http://codereview.chromium.org/160077/diff/1/9#newcode141
Line 141: void FreeSlot(PK11SlotInfo* slot) const { return PK11_FreeSlot(slot);
}
Remove 'return'.  This function returns void.

http://codereview.chromium.org/160077/diff/1/10
File chrome/browser/importer/nss_decryptor_win.h (right):

http://codereview.chromium.org/160077/diff/1/10#newcode141
Line 141: void FreeSlot(PK11SlotInfo* slot) const { return PK11_FreeSlot(slot);
}
Remove 'return'.  This function returns void.

[willchan no longer on Chromium, 2009-07-24 19:45:04]

I've fixed all your comments except the FirefoxProfileLock ones.  I admit I
don't know what the intended behavior is, but the unittest seems to think we're
supposed to delete the file.  Either the test is wrong or the code is wrong. 
I'm happy to fix whichever's wrong if you can tell me what the intended behavior
is supposed to be.

[kuchhal, 2009-07-24 20:01:43]

Yes we are not supposed to delete the original lock file where we only
use fcntl lock. But if that fails firefox falls back to old_lock_file
that is supposed to be deleted when profile is unlocked. If unit test
is failing it must be wrong.

On Fri, Jul 24, 2009 at 12:45 PM, <willchan@chromium.org> wrote:
> I've fixed all your comments except the FirefoxProfileLock ones. =A0I
> admit I don't know what the intended behavior is, but the unittest seems
> to think we're supposed to delete the file. =A0Either the test is wrong o=
r
> the code is wrong. =A0I'm happy to fix whichever's wrong if you can tell
> me what the intended behavior is supposed to be.
>
> http://codereview.chromium.org/160077
>

[wtc, 2009-07-24 20:56:50]

LGTM on Patch Set 2.

The remaining issue is the deletion of the fcntl lock file.

[willchan no longer on Chromium, 2009-07-27 20:50:25]

I've undone the change to firefox_profile_lock_posix.cc and deleted the
incorrect assertions in the test.

[wtc, 2009-07-27 22:05:04]

LGTM.  Thanks!

http://codereview.chromium.org/160077/diff/1016/19
File chrome/browser/importer/firefox_profile_lock_unittest.cc (right):

http://codereview.chromium.org/160077/diff/1016/19#newcode61
Line 61: EXPECT_FALSE(file_util::PathExists(lock_file_path));
Another option is to add #if !defined(OS_LINUX)
around these two assertions and a comment to explain
why they're commented out for fcntl locking.

The assertions are somewhat valuable for the old lock
file style locking...

[willchan no longer on Chromium, 2009-07-28 01:07:49]

http://codereview.chromium.org/160077/diff/1016/19
File chrome/browser/importer/firefox_profile_lock_unittest.cc (right):

http://codereview.chromium.org/160077/diff/1016/19#newcode61
Line 61: EXPECT_FALSE(file_util::PathExists(lock_file_path));
On 2009/07/27 22:05:04, wtc wrote:
> Another option is to add #if !defined(OS_LINUX)
> around these two assertions and a comment to explain
> why they're commented out for fcntl locking.

I've added a comment, and added an #if guard for !OS_POSIX (instead of
!OS_LINUX).

> 
> The assertions are somewhat valuable for the old lock
> file style locking...

Yeah...that should probably be added at some point.

[wtc, 2009-07-28 01:18:35]

LGTM.  Thanks!