mac: Make ScopedRlzValueStoreLock handle nested lock failures.

mac: Make ScopedRlzValueStoreLock handle nested lock failures.

I thought that lib_rlz always early-exits when it fails to acquire the rlz lock,
and so nested lock acquisition failures should never happen. That is true as
long as SupplementaryBranding isn't used, and that can even be used from
user code like so:

  {
    rlz_lib::SupplementaryBranding branding(reactivation_brand.c_str());
    ret &= rlz_lib::RecordProductEvent(product, point, event_id);
  }

Since SupplementaryBranding tries to acquire the rlz lock and keeps it while
it's on the scope, if RecordProductEvent fails to acquire the lock it'll be a
nested lock failure. Update the code to not CHECK on this case.

Add a test that covers this scenario.

BUG=chromium:121255
TEST=unit test. I verified the test crashes in the CHECK without my code change.

[thakis, 2012-04-02 19:22:54]

https://codereview.appspot.com/5966066/diff/4001/lib/rlz_lib_test.cc
File lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5966066/diff/4001/lib/rlz_lib_test.cc#newcode801
lib/rlz_lib_test.cc:801: TEST_F(ReadonlyRlzDirectoryTest, WriteFails) {
Both of the new tests take 5s to run (the time it takes the distributed lock to
time out). That's not great, but better than not having coverage for this I
think. I could be talked into deleting the first test.

[Mark Mentovai, 2012-04-02 19:25:52]

https://codereview.appspot.com/5966066/diff/4001/lib/rlz_lib_test.cc
File lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5966066/diff/4001/lib/rlz_lib_test.cc#newcode798
lib/rlz_lib_test.cc:798: chmod(temp_dir_.path().value().c_str(), 0400);
A directory with mode 400 is useless! Why haven’t you also set the eXecute bit?

https://codereview.appspot.com/5966066/diff/4001/lib/rlz_lib_test.cc#newcode801
lib/rlz_lib_test.cc:801: TEST_F(ReadonlyRlzDirectoryTest, WriteFails) {
thakis wrote:
> Both of the new tests take 5s to run (the time it takes the distributed lock
to
> time out). That's not great, but better than not having coverage for this I
> think. I could be talked into deleting the first test.

I think you should keep both tests.

[thakis, 2012-04-02 19:29:18]

https://codereview.appspot.com/5966066/diff/4001/lib/rlz_lib_test.cc
File lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5966066/diff/4001/lib/rlz_lib_test.cc#newcode798
lib/rlz_lib_test.cc:798: chmod(temp_dir_.path().value().c_str(), 0400);
On 2012/04/02 19:25:52, Mark Mentovai wrote:
> A directory with mode 400 is useless! Why haven’t you also set the eXecute
bit?

Done.

[Roger Tawa (Google), 2012-04-02 19:31:17]

One little typo :-)

https://codereview.appspot.com/5966066/diff/1004/lib/rlz_lib_test.cc
File lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5966066/diff/1004/lib/rlz_lib_test.cc#newcode816
lib/rlz_lib_test.cc:816: // Se the comment at the top of WriteFails.
Se --> See

[shess, 2012-04-02 19:34:50]

https://codereview.appspot.com/5966066/diff/4001/lib/rlz_lib_test.cc
File lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5966066/diff/4001/lib/rlz_lib_test.cc#newcode798
lib/rlz_lib_test.cc:798: chmod(temp_dir_.path().value().c_str(), 0400);
On 2012/04/02 19:29:18, thakis wrote:
> On 2012/04/02 19:25:52, Mark Mentovai wrote:
> > A directory with mode 400 is useless! Why haven’t you also set the eXecute
> bit?
> 
> Done.

You should (not) make it 100, allowing to read out files if you know their
names, but not list the directory.  You can't be too safe!

[Mark Mentovai, 2012-04-02 19:38:44]

LGTM

https://codereview.appspot.com/5966066/diff/1004/lib/rlz_lib_test.cc
File lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5966066/diff/1004/lib/rlz_lib_test.cc#newcode798
lib/rlz_lib_test.cc:798: chmod(temp_dir_.path().value().c_str(), 0500);
or 0100 if you don’t want to allow the owner to enumerate, per Scott.

[thakis, 2012-04-02 19:45:39]

Thanks!

https://codereview.appspot.com/5966066/diff/1004/lib/rlz_lib_test.cc
File lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5966066/diff/1004/lib/rlz_lib_test.cc#newcode798
lib/rlz_lib_test.cc:798: chmod(temp_dir_.path().value().c_str(), 0500);
On 2012/04/02 19:38:44, Mark Mentovai wrote:
> or 0100 if you don’t want to allow the owner to enumerate, per Scott.

At the moment nothing enumerates files, so this is fairly theoretical. But
changed it to 0100 for Scott anyway :-) (I checked that tests still pass, and
still fail if I undo my code change in the mm file.)

https://codereview.appspot.com/5966066/diff/1004/lib/rlz_lib_test.cc#newcode816
lib/rlz_lib_test.cc:816: // Se the comment at the top of WriteFails.
On 2012/04/02 19:31:17, rogerta wrote:
> Se --> See

Done.

[shess, 2012-04-02 19:46:29]

https://codereview.appspot.com/5966066/diff/1004/lib/rlz_lib_test.cc
File lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5966066/diff/1004/lib/rlz_lib_test.cc#newcode798
lib/rlz_lib_test.cc:798: chmod(temp_dir_.path().value().c_str(), 0500);
On 2012/04/02 19:38:44, Mark Mentovai wrote:
> or 0100 if you don’t want to allow the owner to enumerate, per Scott.

Err, before that offhand comment gets out of hand ... 0100 mostly just annoys
the owner, because they can't recursively delete the damned thing, but they can
chmod it.  1 for group or other is semi-useful for readable dropboxes, but
dollars to donuts is the wrong thing to do.

[thakis, 2012-04-02 19:47:48]

https://codereview.appspot.com/5966066/diff/1004/lib/rlz_lib_test.cc
File lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5966066/diff/1004/lib/rlz_lib_test.cc#newcode798
lib/rlz_lib_test.cc:798: chmod(temp_dir_.path().value().c_str(), 0500);
On 2012/04/02 19:46:29, shess wrote:
> On 2012/04/02 19:38:44, Mark Mentovai wrote:
> > or 0100 if you don’t want to allow the owner to enumerate, per Scott.
> 
> Err, before that offhand comment gets out of hand ... 0100 mostly just annoys
> the owner, because they can't recursively delete the damned thing, but they
can
> chmod it.  1 for group or other is semi-useful for readable dropboxes, but
> dollars to donuts is the wrong thing to do.

It's in a ScopedTempDir. But fine, changed it back to 0500 :-/