Extract ScopedTestNSSDB from nss_util.

Extract ScopedTestNSSDB from nss_util.

Before ScopedTestNSSDB affected several slot getters from nss_util.h .
This change reduces ScopedTestNSSDB to solely setup a temporary test DB and not influencing the global state in nss_util anymore.

As a replacement for some of its old behavior, a new ScopedTestSystemNSSKeySlot is added, which allows to override the slot returned by GetSystemNSSKeySlot().

With this change it's now possible to write tests that need both a user and system NSS DB by using ScopedTestSystemNSSKeySlot.

As a side-effect, GetPersistentNSSKeySlot() is now compiled on !OS_CHROMEOS only.

BUG=210525
(For include changes:)

R=rsleevi@chromium.org
TBR=nkostylev@chromium.org, stevenjb@chromium.org

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

[pneubeck (no reviews), 2014-07-21 15:16:37]

Finally, after over a week / seven CLs...

[Ryan Sleevi, 2014-07-22 01:25:32]

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util.h
File crypto/nss_util.h (right):

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util.h#newcod...
crypto/nss_util.h:156: class CRYPTO_EXPORT_PRIVATE ScopedTestSystemNSSKeySlot {
1) Document more what this does - and what it affects.
2) This belongs in a test-only file, not the main nss_util. We'd probably roll
it into a crypto_test_support target if it needed to be used outside of
crypto_unittests (which it probably does)

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util_internal...
crypto/nss_util_internal.h:26: // database. Returns NULL if the database couln't
be opened.
This is not really a "user" DB concept, and I think conflating Chrome and
ChromeOS terms.

"Opens an NSS software database in folder |path|, with the (potentially)
user-visible description |description|. Returns the slot for the opened
database, or NULL if the database could not be opened."

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util_internal...
crypto/nss_util_internal.h:34: // be fixed first.
Guess you can remove lines 33-34 :)

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_d...
File crypto/scoped_test_nss_db.cc (right):

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_d...
crypto/scoped_test_nss_db.cc:25: const char kTestTPMTokenName[] = "Test DB";
Fix variable name. It's not a TPM :)

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_db.h
File crypto/scoped_test_nss_db.h (right):

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_d...
crypto/scoped_test_nss_db.h:15: #if defined(USE_NSS)
Use source controls to exclude this. Don't #ifdef.

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_d...
crypto/scoped_test_nss_db.h:21: // opened user DB is not automatically closed.
Update this comment to 3.15.1 (we require 3.14.3, but another bug existed that
wasn't fixed until 3.15.1)

[pneubeck (no reviews), 2014-07-22 06:23:27]

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_db.h
File crypto/scoped_test_nss_db.h (right):

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_d...
crypto/scoped_test_nss_db.h:21: // opened user DB is not automatically closed.
On 2014/07/22 01:25:32, Ryan Sleevi wrote:
> Update this comment to 3.15.1 (we require 3.14.3, but another bug existed that
> wasn't fixed until 3.15.1)

Actually, I'm not sure that I understand the part
  'switch back to using a separate user DB for each test'
correctly.
Do you have a bit more context about what Matt could have meant?
If a test is using this Scoped*DB in several test cases (or the fixture), this
should already either open multiple DBs (if the previous is not closed) or close
the old, delete the tempdir, and create a new DB.
How can it be more 'separate'?

[pneubeck (no reviews), 2014-07-22 08:34:30]

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util.h
File crypto/nss_util.h (right):

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util.h#newcod...
crypto/nss_util.h:156: class CRYPTO_EXPORT_PRIVATE ScopedTestSystemNSSKeySlot {
On 2014/07/22 01:25:32, Ryan Sleevi wrote:
> 1) Document more what this does - and what it affects.
> 2) This belongs in a test-only file, not the main nss_util. We'd probably roll
> it into a crypto_test_support target if it needed to be used outside of
> crypto_unittests (which it probably does)
ACK. I only placed it here because of ScopedTestNSSChromeOSUser above. But this
is indeed the wrong place for both.


Considering that this sets a member (test_system_slot_) in the NSSInitSingleton
and doesn't store/restore the old state of that member, I'd like to add a DCHECK
if that member was already set before.
Sounds good?

[Ryan Sleevi, 2014-07-22 08:35:09]

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_db.h
File crypto/scoped_test_nss_db.h (right):

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_d...
crypto/scoped_test_nss_db.h:21: // opened user DB is not automatically closed.
On 2014/07/22 06:23:27, pneubeck wrote:
> On 2014/07/22 01:25:32, Ryan Sleevi wrote:
> > Update this comment to 3.15.1 (we require 3.14.3, but another bug existed
that
> > wasn't fixed until 3.15.1)
> 
> Actually, I'm not sure that I understand the part
>   'switch back to using a separate user DB for each test'
> correctly.
> Do you have a bit more context about what Matt could have meant?
> If a test is using this Scoped*DB in several test cases (or the fixture), this
> should already either open multiple DBs (if the previous is not closed) or
close
> the old, delete the tempdir, and create a new DB.
> How can it be more 'separate'?

Followed-up offline, but for posterity:

Prior to NSS 3.14.3, we opened once and only one, and never closed.
We changed that with NSS 3.15, and discovered a new bug with closing that was
fixed in NSS 3.15.1. We still left it to always-open though (and just leaks).

With NSS 3.15.1+, we can open & close. But we still require NSS 3.14.3+ (due to
being the lowest common denominator across all OS's the last time we bumped NSS
revs)

If the Chrome Linux system versions have been updated, or distros have
consistent newer versions of NSS in their repos (around 8 weeks ago they
didn't), we can bump this rev higher, to make it easier. That's mostly a
research activity (find all documented first class Linux OS's, check their
package repos for (main+security), confirm it's stable)

[pneubeck (no reviews), 2014-07-22 14:11:06]

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util.h
File crypto/nss_util.h (right):

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util.h#newcod...
crypto/nss_util.h:156: class CRYPTO_EXPORT_PRIVATE ScopedTestSystemNSSKeySlot {
One other reason why I put it here was that it access the NSSInitSingleton
(similary like ScopedTestNSSChromeOSUser accesses it). NSSInitSingleton however
is .cc local.
Thus, I added *ForTesting functions to expose the required functions of the
singleton.

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util_internal...
crypto/nss_util_internal.h:26: // database. Returns NULL if the database couln't
be opened.
On 2014/07/22 01:25:32, Ryan Sleevi wrote:
> This is not really a "user" DB concept, and I think conflating Chrome and
> ChromeOS terms.
> 
> "Opens an NSS software database in folder |path|, with the (potentially)
> user-visible description |description|. Returns the slot for the opened
> database, or NULL if the database could not be opened."

Done.

https://codereview.chromium.org/401623006/diff/80001/crypto/nss_util_internal...
crypto/nss_util_internal.h:34: // be fixed first.
On 2014/07/22 01:25:32, Ryan Sleevi wrote:
> Guess you can remove lines 33-34 :)

Done.

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_d...
File crypto/scoped_test_nss_db.cc (right):

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_d...
crypto/scoped_test_nss_db.cc:25: const char kTestTPMTokenName[] = "Test DB";
On 2014/07/22 01:25:32, Ryan Sleevi wrote:
> Fix variable name. It's not a TPM :)

Done.

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_db.h
File crypto/scoped_test_nss_db.h (right):

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_d...
crypto/scoped_test_nss_db.h:15: #if defined(USE_NSS)
On 2014/07/22 01:25:32, Ryan Sleevi wrote:
> Use source controls to exclude this. Don't #ifdef.

Done. I also added it to a new target crypto_test_support and removed this from
the non-test target 'crypto'.

https://codereview.chromium.org/401623006/diff/80001/crypto/scoped_test_nss_d...
crypto/scoped_test_nss_db.h:21: // opened user DB is not automatically closed.
This comment didn't apply anymore.
Updated the comment to the remaining relevant NSS bug (which matches now the
comment in the .cc).

On 2014/07/22 08:35:09, Ryan Sleevi wrote:
> On 2014/07/22 06:23:27, pneubeck wrote:
> > On 2014/07/22 01:25:32, Ryan Sleevi wrote:
> > > Update this comment to 3.15.1 (we require 3.14.3, but another bug existed
> that
> > > wasn't fixed until 3.15.1)
> > 
> > Actually, I'm not sure that I understand the part
> >   'switch back to using a separate user DB for each test'
> > correctly.
> > Do you have a bit more context about what Matt could have meant?
> > If a test is using this Scoped*DB in several test cases (or the fixture),
this
> > should already either open multiple DBs (if the previous is not closed) or
> close
> > the old, delete the tempdir, and create a new DB.
> > How can it be more 'separate'?
> 
> Followed-up offline, but for posterity:
> 
> Prior to NSS 3.14.3, we opened once and only one, and never closed.
> We changed that with NSS 3.15, and discovered a new bug with closing that was
> fixed in NSS 3.15.1. We still left it to always-open though (and just leaks).
> 
> With NSS 3.15.1+, we can open & close. But we still require NSS 3.14.3+ (due
to
> being the lowest common denominator across all OS's the last time we bumped
NSS
> revs)
> 
> If the Chrome Linux system versions have been updated, or distros have
> consistent newer versions of NSS in their repos (around 8 weeks ago they
> didn't), we can bump this rev higher, to make it easier. That's mostly a
> research activity (find all documented first class Linux OS's, check their
> package repos for (main+security), confirm it's stable)

[pneubeck (no reviews), 2014-07-22 17:10:25]

This is still missing the update to the BUILD.gn files, which I will do soon.

[Ryan Sleevi, 2014-07-23 02:15:29]

I think this is looking pretty good! The NSSCertDatabase changes I think are
fine, but could always benefit from documentation.

In fact, that's the biggest concern with the remaining. Just nailing down some
documentation to explain :)

https://codereview.chromium.org/401623006/diff/140001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/401623006/diff/140001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:498: '../crypto/crypto.gyp:crypto_test_support',
Should this be moved into a condition for use_nss=1? Probably.

https://codereview.chromium.org/401623006/diff/140001/crypto/crypto.gyp
File crypto/crypto.gyp (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/crypto.gyp#newco...
crypto/crypto.gyp:167: 'conditions': [
I don't think this will compile on Windows/Mac/OpenSSL.

You probably need to wrap the target in a conditional, where it's a 'none'
target on !use_nss && !chromeos, and a static_library case (like you've got
setup here).

https://codereview.chromium.org/401623006/diff/140001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:1008: void SetTestSystemKeySlot(ScopedPK11Slot slot) {
SetSystemKeySlotForTesting - that way presubmit scripts will verify it's only
called from unittests.

https://codereview.chromium.org/401623006/diff/140001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/nss_util_interna...
crypto/nss_util_internal.h:24: #if defined(USE_NSS)
I don't think you need this #ifdef - see lines 19/20, it's guaranteed we're
using NSS here (also lines 39-40)

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_nss_...
File crypto/scoped_test_nss_chromeos_user.cc (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_nss_...
crypto/scoped_test_nss_chromeos_user.cc:33:
InitializePrivateSoftwareSlotForChromeOSUser(username_hash_);
This doesn't look like testing code. Trying to understand a bit what's going on
here, if you can explain.

That is, I thought the goal was to create a ScopedTestNSSDB, then set that as
the TPM slot for that user. But this seems very different.

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_nss_...
File crypto/scoped_test_nss_chromeos_user.h (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_nss_...
crypto/scoped_test_nss_chromeos_user.h:16: class CRYPTO_EXPORT_PRIVATE
ScopedTestNSSChromeOSUser {
Document? :)

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_nss_...
crypto/scoped_test_nss_chromeos_user.h:18: explicit
ScopedTestNSSChromeOSUser(const std::string& username_hash);
Document? :)

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_syst...
File crypto/scoped_test_system_nss_key_slot.h (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_syst...
crypto/scoped_test_system_nss_key_slot.h:16: class CRYPTO_EXPORT_PRIVATE
ScopedTestSystemNSSKeySlot {
Document ;)

[pneubeck (no reviews), 2014-07-23 14:34:17]

chrome_browser.gypi and chrome_tests_unit.gypi seem not to have a .gn
equivalent.

https://codereview.chromium.org/401623006/diff/140001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/401623006/diff/140001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:498: '../crypto/crypto.gyp:crypto_test_support',
On 2014/07/23 02:15:28, Ryan Sleevi wrote:
> Should this be moved into a condition for use_nss=1? Probably.

it's a none target now if use_nss==0.

https://codereview.chromium.org/401623006/diff/140001/crypto/crypto.gyp
File crypto/crypto.gyp (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/crypto.gyp#newco...
crypto/crypto.gyp:167: 'conditions': [
On 2014/07/23 02:15:28, Ryan Sleevi wrote:
> I don't think this will compile on Windows/Mac/OpenSSL.
> 
> You probably need to wrap the target in a conditional, where it's a 'none'
> target on !use_nss && !chromeos, and a static_library case (like you've got
> setup here).

Done.

https://codereview.chromium.org/401623006/diff/140001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:1008: void SetTestSystemKeySlot(ScopedPK11Slot slot) {
On 2014/07/23 02:15:28, Ryan Sleevi wrote:
> SetSystemKeySlotForTesting - that way presubmit scripts will verify it's only
> called from unittests.

Done.

https://codereview.chromium.org/401623006/diff/140001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/nss_util_interna...
crypto/nss_util_internal.h:24: #if defined(USE_NSS)
On 2014/07/23 02:15:28, Ryan Sleevi wrote:
> I don't think you need this #ifdef - see lines 19/20, it's guaranteed we're
> using NSS here (also lines 39-40)

Done.

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_nss_...
File crypto/scoped_test_nss_chromeos_user.cc (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_nss_...
crypto/scoped_test_nss_chromeos_user.cc:33:
InitializePrivateSoftwareSlotForChromeOSUser(username_hash_);
On 2014/07/23 02:15:29, Ryan Sleevi wrote:
> This doesn't look like testing code. Trying to understand a bit what's going
on
> here, if you can explain.
> 
> That is, I thought the goal was to create a ScopedTestNSSDB, then set that as
> the TPM slot for that user. But this seems very different.

This is moved unmodified from nss_util.cc . If necessary, I'd like to modify it
in a follow up.
Added some explanation of what's going on.

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_nss_...
File crypto/scoped_test_nss_chromeos_user.h (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_nss_...
crypto/scoped_test_nss_chromeos_user.h:16: class CRYPTO_EXPORT_PRIVATE
ScopedTestNSSChromeOSUser {
On 2014/07/23 02:15:29, Ryan Sleevi wrote:
> Document? :)

Yes, the documentation was very sparse before :-)

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_nss_...
crypto/scoped_test_nss_chromeos_user.h:18: explicit
ScopedTestNSSChromeOSUser(const std::string& username_hash);
On 2014/07/23 02:15:29, Ryan Sleevi wrote:
> Document? :)

Done.

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_syst...
File crypto/scoped_test_system_nss_key_slot.h (right):

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_syst...
crypto/scoped_test_system_nss_key_slot.h:15: 
Is CRYPTO_EXPORT_PRIVATE the right export annotation for test support that is
used outside of this component?

https://codereview.chromium.org/401623006/diff/140001/crypto/scoped_test_syst...
crypto/scoped_test_system_nss_key_slot.h:16: class CRYPTO_EXPORT_PRIVATE
ScopedTestSystemNSSKeySlot {
On 2014/07/23 02:15:29, Ryan Sleevi wrote:
> Document ;)

Done.

[Ryan Sleevi, 2014-07-24 23:14:02]

LGTM % nits

https://codereview.chromium.org/401623006/diff/220001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/401623006/diff/220001/crypto/nss_util_interna...
crypto/nss_util_internal.h:60: // is NULL, the test system slot is unset.
To confirm:

Is the following valid

Set(!=NULL)
Set(NULL)
Set(!=NULL) ?

https://codereview.chromium.org/401623006/diff/220001/crypto/scoped_test_nss_...
File crypto/scoped_test_nss_chromeos_user.h (right):

https://codereview.chromium.org/401623006/diff/220001/crypto/scoped_test_nss_...
crypto/scoped_test_nss_chromeos_user.h:17: // user with |username_hash|. This
database will be use for both the user's
typo: used

https://codereview.chromium.org/401623006/diff/220001/crypto/scoped_test_syst...
File crypto/scoped_test_system_nss_key_slot.h (right):

https://codereview.chromium.org/401623006/diff/220001/crypto/scoped_test_syst...
crypto/scoped_test_system_nss_key_slot.h:24: // At most one instance of this
helper must be used at a time.
wrapping on the comment.

[pneubeck (no reviews), 2014-07-25 08:35:14]

Thanks for reviewing!

https://codereview.chromium.org/401623006/diff/220001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/401623006/diff/220001/crypto/nss_util_interna...
crypto/nss_util_internal.h:60: // is NULL, the test system slot is unset.
On 2014/07/24 23:14:02, Ryan Sleevi wrote:
> To confirm:
> 
> Is the following valid
> 
> Set(!=NULL)
> Set(NULL)
> Set(!=NULL) ?
> 
In principal yes. Although, in tests nobody should ever have need for this.
The Scoped helper should be used anyways.

https://codereview.chromium.org/401623006/diff/220001/crypto/scoped_test_nss_...
File crypto/scoped_test_nss_chromeos_user.h (right):

https://codereview.chromium.org/401623006/diff/220001/crypto/scoped_test_nss_...
crypto/scoped_test_nss_chromeos_user.h:17: // user with |username_hash|. This
database will be use for both the user's
On 2014/07/24 23:14:02, Ryan Sleevi wrote:
> typo: used

Done.

https://codereview.chromium.org/401623006/diff/220001/crypto/scoped_test_syst...
File crypto/scoped_test_system_nss_key_slot.h (right):

https://codereview.chromium.org/401623006/diff/220001/crypto/scoped_test_syst...
crypto/scoped_test_system_nss_key_slot.h:24: // At most one instance of this
helper must be used at a time.
On 2014/07/24 23:14:02, Ryan Sleevi wrote:
> wrapping on the comment.

Done.

[pneubeck (no reviews), 2014-07-25 08:35:51]

@Nikita, needs owner lgtm for include changes in tests.

[pneubeck (no reviews), 2014-07-25 08:36:56]

@Nikita, only necessary to look at chrome/browser/chromeos

[pneubeck (no reviews), 2014-07-25 11:56:41]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-07-25 11:57:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/401623006/240001

[pneubeck (no reviews), 2014-07-25 15:12:27]

+Steven for chromeos/ include/build target changes

[pneubeck (no reviews), 2014-07-25 15:12:46]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-07-25 15:13:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/401623006/260001

[commit-bot: I haz the power, 2014-07-25 15:26:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/21...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33341)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/38377)

[commit-bot: I haz the power, 2014-07-25 15:28:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-25 15:28:29]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33344)

[pneubeck (no reviews), 2014-07-26 13:11:48]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-07-26 13:13:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/401623006/260001

[pneubeck (no reviews), 2014-07-26 13:18:33]

The CQ bit was unchecked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-07-26 13:30:37]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-07-26 13:32:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/401623006/280001

[pneubeck (no reviews), 2014-07-26 14:17:13]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-07-26 14:19:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/401623006/300001

[commit-bot: I haz the power, 2014-07-26 18:23:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-07-26 18:27:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-26 18:27:36]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pneubeck (no reviews), 2014-07-27 15:43:42]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-07-27 15:44:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/401623006/320001

[commit-bot: I haz the power, 2014-07-27 18:41:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-07-27 18:44:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-27 18:44:15]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pneubeck (no reviews), 2014-07-28 09:56:59]

Committed patchset #15 manually as r285881.

[mattm, 2014-07-29 23:12:54]

https://codereview.chromium.org/401623006/diff/360001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/401623006/diff/360001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:580: test_system_slot_ = slot.Pass();
since the value may have already been referenced by tpm_slot_, just NULLing
test_system_slot_ here seems like it would usually have no noticeable effect.

[pneubeck (no reviews), 2014-07-30 07:51:27]

https://codereview.chromium.org/401623006/diff/360001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/401623006/diff/360001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:580: test_system_slot_ = slot.Pass();
On 2014/07/29 23:12:54, mattm wrote:
> since the value may have already been referenced by tpm_slot_, just NULLing
> test_system_slot_ here seems like it would usually have no noticeable effect.
Yeah, I realized this finally too. (it wasn't used in this CL yet)
In the current pending CL this is addressed:
https://codereview.chromium.org/424523002/diff/180001/crypto/nss_util.cc