Fix import Firefox passwords on Windows (silent failure); plds4.dll and nspr4.dll no longer exist

Fix import Firefox passwords on Windows (silent failure); plds4.dll and nspr4.dll no longer exist
BUG=256125
TEST=Try import passwords from Firefox 22 or 23

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223574

[willchan no longer on Chromium, 2013-09-05 02:31:43]

I'm redirecting to an OWNER. I don't really know this code, I just did some
minor fixups ages ago.

[Miranda Callahan, 2013-09-05 12:05:17]

On 2013/09/05 02:31:43, willchan wrote:
> I'm redirecting to an OWNER. I don't really know this code, I just did some
> minor fixups ages ago.

This seems ok to me, but I'm going to double-bounce over to gab, as he is the
current importer master. I need to remove myself from the OWNERS list.

[gab, 2013-09-06 18:06:12]

I don't know anything about the NSS stuff in import; Ilya maybe?

I remember talking with scottmg about perhaps being able to remove this as it
didn't look like some _nss.cc files were used by any configuration we use (in
gyp)... other than that I have no idea..

[Ilya Sherman, 2013-09-06 20:28:22]

I vaguely recall Avi looking into Firefox importing on Windows recently.  Avi,
am I remembering that correctly?  If so, mebbe you have enough context to review
this CL?  I'm not familiar with this part of the code at all.

[Avi (use Gerrit), 2013-09-06 20:43:47]

I know nothing about the NSS stuff. I looked into Firefox importing when I was
hunting down the cause of this very bug, but bailed as soon as I found that it
was a problem in NSS.

But this change looks reasonable, and I trust that drbasic has verified that it
works. I'm willing to give it an LGTM.

https://codereview.chromium.org/23799008/diff/1/chrome/utility/importer/nss_d...
File chrome/utility/importer/nss_decryptor_win.cc (right):

https://codereview.chromium.org/23799008/diff/1/chrome/utility/importer/nss_d...
chrome/utility/importer/nss_decryptor_win.cc:125: // FireFox 22 and higher
PL_ArenaFinish moved from plds4.dll to nss3.dll
It's "Firefox" rather than "FireFox".

[Ilya Sherman, 2013-09-06 21:32:47]

Just a style nit from me:

https://chromiumcodereview.appspot.com/23799008/diff/1/chrome/utility/importe...
File chrome/utility/importer/nss_decryptor_win.cc (right):

https://chromiumcodereview.appspot.com/23799008/diff/1/chrome/utility/importe...
chrome/utility/importer/nss_decryptor_win.cc:129: :
base::GetFunctionPointerFromNativeLibrary(nss3_dll_, "PL_ArenaFinish"));
nit: Please define a local variable for the DLL, rather than repeating a good
chunk of code in the ternary stmt.  Ditto below.

[commit-bot: I haz the power, 2013-09-11 03:45:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/drbasic@yandex-team.ru/23799008/14001

[commit-bot: I haz the power, 2013-09-11 04:00:03]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[drbasic, 2013-09-11 04:06:23]

On 2013/09/06 20:43:47, Avi wrote:
> I know nothing about the NSS stuff. I looked into Firefox importing when I was
> hunting down the cause of this very bug, but bailed as soon as I found that it
> was a problem in NSS.
> 
> But this change looks reasonable, and I trust that drbasic has verified that
it
> works. I'm willing to give it an LGTM.
> 
>
https://codereview.chromium.org/23799008/diff/1/chrome/utility/importer/nss_d...
> File chrome/utility/importer/nss_decryptor_win.cc (right):
> 
>
https://codereview.chromium.org/23799008/diff/1/chrome/utility/importer/nss_d...
> chrome/utility/importer/nss_decryptor_win.cc:125: // FireFox 22 and higher
> PL_ArenaFinish moved from plds4.dll to nss3.dll
> It's "Firefox" rather than "FireFox".

I changed the code in compliance with remarks. I need new LGTM?

[Avi (use Gerrit), 2013-09-11 04:41:07]

On 2013/09/11 04:06:23, drbasic wrote:
> On 2013/09/06 20:43:47, Avi wrote:
> > I know nothing about the NSS stuff. I looked into Firefox importing when I
was
> > hunting down the cause of this very bug, but bailed as soon as I found that
it
> > was a problem in NSS.
> > 
> > But this change looks reasonable, and I trust that drbasic has verified that
> it
> > works. I'm willing to give it an LGTM.
> > 
> >
>
https://codereview.chromium.org/23799008/diff/1/chrome/utility/importer/nss_d...
> > File chrome/utility/importer/nss_decryptor_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/23799008/diff/1/chrome/utility/importer/nss_d...
> > chrome/utility/importer/nss_decryptor_win.cc:125: // FireFox 22 and higher
> > PL_ArenaFinish moved from plds4.dll to nss3.dll
> > It's "Firefox" rather than "FireFox".
> 
> I changed the code in compliance with remarks. I need new LGTM?

The trybot should be more explicit, but if you click on the red result you see:

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/utility/importer/nss_decryptor_win.cc

You need someone who is an OWNER for that file to approve, not just me.

http://dev.chromium.org/developers/owners-files

[drbasic, 2013-09-11 04:47:23]

On 2013/09/06 21:32:47, Ilya Sherman wrote:
> Just a style nit from me:
> 
>
https://chromiumcodereview.appspot.com/23799008/diff/1/chrome/utility/importe...
> File chrome/utility/importer/nss_decryptor_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/23799008/diff/1/chrome/utility/importe...
> chrome/utility/importer/nss_decryptor_win.cc:129: :
> base::GetFunctionPointerFromNativeLibrary(nss3_dll_, "PL_ArenaFinish"));
> nit: Please define a local variable for the DLL, rather than repeating a good
> chunk of code in the ternary stmt.  Ditto below.

Please check updated patch.

[Peter Kasting, 2013-09-11 05:18:43]

+CC wtc as someone more likely to know about NSS than anyone else on the team.

https://codereview.chromium.org/23799008/diff/14001/chrome/utility/importer/n...
File chrome/utility/importer/nss_decryptor_win.cc (right):

https://codereview.chromium.org/23799008/diff/14001/chrome/utility/importer/n...
chrome/utility/importer/nss_decryptor_win.cc:125: // Firefox 22 and higher
PL_ArenaFinish and PR_Cleanup moved from plds4.dll
Nit: Please use complete grammatical sentences:

On Firefox 22 and higher, PL_ArenaFinish() and PR_Cleanup() are found in
nss3.dll rather than plds4.dll.

[Avi (use Gerrit), 2013-09-11 05:19:38]

On 2013/09/11 04:47:23, drbasic wrote:
> On 2013/09/06 21:32:47, Ilya Sherman wrote:
> > Just a style nit from me:
> > 
> >
>
https://chromiumcodereview.appspot.com/23799008/diff/1/chrome/utility/importe...
> > File chrome/utility/importer/nss_decryptor_win.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/23799008/diff/1/chrome/utility/importe...
> > chrome/utility/importer/nss_decryptor_win.cc:129: :
> > base::GetFunctionPointerFromNativeLibrary(nss3_dll_, "PL_ArenaFinish"));
> > nit: Please define a local variable for the DLL, rather than repeating a
good
> > chunk of code in the ternary stmt.  Ditto below.
> 
> Please check updated patch.

Don't ask *everyone* from the OWNERS file. Pick one person (probably with the
help of svn blame to see who's been most active) and ask only them.

[Ilya Sherman, 2013-09-11 23:39:40]

Whoops, sorry, didn't realize this review was blocked on me.

https://codereview.chromium.org/23799008/diff/29001/chrome/utility/importer/n...
File chrome/utility/importer/nss_decryptor_win.cc (right):

https://codereview.chromium.org/23799008/diff/29001/chrome/utility/importer/n...
chrome/utility/importer/nss_decryptor_win.cc:134:
base::GetFunctionPointerFromNativeLibrary(nspr4_dll, "PL_ArenaFinish"));
Unless I'm misreading something, this was plds4_dll previously.  Did you
intentionally change it?

[wtc, 2013-09-11 23:44:02]

Review comments on patch set 5:

https://codereview.chromium.org/23799008/diff/29001/chrome/utility/importer/n...
File chrome/utility/importer/nss_decryptor_win.cc (right):

https://codereview.chromium.org/23799008/diff/29001/chrome/utility/importer/n...
chrome/utility/importer/nss_decryptor_win.cc:88: HMODULE nspr4_dll =
GetModuleHandle(kNSPR4Library);

We can also handle the problem here:

  // On Firefox 22 and higher, NSPR is part of nss3.dll rather than separate
DLLs.
  if (plds4_dll == NULL) {
    plds4_dll = nss3_dll_;
    nspr4_dll = nss3_dll_;
  }

And then in NSSDecryptor::InitNSS you just need to delete
  // NSPR DLLs are already loaded now.
  if (plds4_dll == NULL || nspr4_dll == NULL) {
    Free();
    return false;
  }

https://codereview.chromium.org/23799008/diff/29001/chrome/utility/importer/n...
chrome/utility/importer/nss_decryptor_win.cc:126: // nss3.dll rather than
plds4.dll.

plds4.dll => plds4.dll and nspr4.dll.

This is a recent change in Firefox, although I don't know if the change was
made in Firefox 22. Several NSPR and NSS DLLs are combined into a single
nss3.dll.

https://codereview.chromium.org/23799008/diff/29001/chrome/utility/importer/n...
chrome/utility/importer/nss_decryptor_win.cc:134:
base::GetFunctionPointerFromNativeLibrary(nspr4_dll, "PL_ArenaFinish"));

We need to use plds4_dll here.

[wtc, 2013-09-17 03:23:04]

Patch set 8 LGTM.

[commit-bot: I haz the power, 2013-09-17 03:37:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/drbasic@yandex-team.ru/23799008/41001

[drbasic, 2013-09-17 03:50:02]

Patch need an owner LGTM.

[commit-bot: I haz the power, 2013-09-17 03:50:14]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Ilya Sherman, 2013-09-17 06:14:35]

LGTM, thanks.  (For future reference, it's helpful for reviewers if you send out
an email saying that you've uploaded a new patch -- otherwise, there's no
notification to us that you've done so, so we don't know to take another look.)

https://chromiumcodereview.appspot.com/23799008/diff/41001/chrome/utility/imp...
File chrome/utility/importer/nss_decryptor_win.cc (left):

https://chromiumcodereview.appspot.com/23799008/diff/41001/chrome/utility/imp...
chrome/utility/importer/nss_decryptor_win.cc:113: }
nit: Are you sure that this code is no longer necessary?

[commit-bot: I haz the power, 2013-09-17 06:18:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/drbasic@yandex-team.ru/23799008/41001

[commit-bot: I haz the power, 2013-09-17 06:24:54]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-17 08:51:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/drbasic@yandex-team.ru/23799008/41001

[commit-bot: I haz the power, 2013-09-17 09:39:16]

Change committed as 223574

[wtc, 2013-09-18 00:03:47]

https://chromiumcodereview.appspot.com/23799008/diff/41001/chrome/utility/imp...
File chrome/utility/importer/nss_decryptor_win.cc (left):

https://chromiumcodereview.appspot.com/23799008/diff/41001/chrome/utility/imp...
chrome/utility/importer/nss_decryptor_win.cc:113: }

On 2013/09/17 06:14:36, Ilya Sherman wrote:
> nit: Are you sure that this code is no longer necessary?

Yes. This function (InitNSS) is only called on line 96, and we
make sure plds4_dll and nspr4_dll are both non-null there.

[wtc, 2013-09-18 18:29:04]

https://codereview.chromium.org/23799008/diff/41001/chrome/utility/importer/n...
File chrome/utility/importer/nss_decryptor_win.cc (right):

https://codereview.chromium.org/23799008/diff/41001/chrome/utility/importer/n...
chrome/utility/importer/nss_decryptor_win.cc:91: // DLLs.

For future reference:

In Firefox 22, the NSPR and NSS DLLs were consolidated into fewer
DLLs. Only these DLLs remained:

1. The main NSS DLL (contains NSPR and NSSUTIL)
    nss3.dll

2. The NSS softoken and its dependents

    softokn3.dll
    freebl3.dll
    nssdbm3.dll

3. The NSS built-in root CA certificates

    nssckbi.dll