[cros] RlzValueStore made protected by a cross-process lock and not persisted over browser lifetime…

[cros] RlzValueStore made protected by a cross-process lock and not persisted over browser lifetime (like on Mac).

*) Moved RecursiveCrossProcessLock out of .mm file to a common _posix file.
*) Added static method to ImportantFileWriter that does blocking write on the current thread.
*) Dedicated RLZ thread gone, replaced back with shutdown-blocking worker pool.


BUG=157348, 62328
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170179

[Ivan Korotkov, 2012-11-26 13:09:28]

PTAL:
Roger: rlz/, chrome/browser/rlz
Mattias: base/files

Carlos: FYI (RLZ thread removed :)

[Mattias Nissler (ping if slow), 2012-11-26 15:48:57]

The base/ change looks reasonable to me, I just have a naming nit. I'm not an
OWNER of these files though to the best of my knowledge ;)

https://codereview.chromium.org/11308196/diff/3018/base/files/important_file_...
File base/files/important_file_writer.h (right):

https://codereview.chromium.org/11308196/diff/3018/base/files/important_file_...
base/files/important_file_writer.h:54: // Save |data| to |path|. Blocks and
writes data on the current thread.
Should mention that this uses save-to-tempfile-and-rename, so it's clear what is
different from file_util::WriteFile. You might also want to update the name to
something better. Maybe WriteFileAtomically? That's just from the top of my
head, feel free to come up with a better name.

[Roger Tawa OOO till Jul 10th, 2012-11-26 19:16:51]

Thanks Ivan.  A few questions below.

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
File rlz/chromeos/lib/rlz_value_store_chromeos.cc (right):

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:82: DCHECK(CalledOnValidThread());
why call CalledOnValidThread() in ctor?

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:87: DCHECK(CalledOnValidThread());
the base class dtor already calls CalledOnValidThread().

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:99: base::Int64ToString(time));
one extra space.

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:218: read_only_ = false;
why set read-only to false here?

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:259: //
rlz_store_->MarkNeedsEmptyValue(list_name);
remove line?

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:293: DCHECK(!g_store);
Maybe add DCHECK(g_lock_depth==1)?  Does not happen today, but if a recursive
ScopedRlzValueStoreLock were created when the first one failed to get the
process lock, seems like the mutex will not get release in the dtor.  A DCHECK
here could catch that.

https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
File rlz/lib/recursive_cross_process_lock_posix.cc (right):

https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
rlz/lib/recursive_cross_process_lock_posix.cc:66: return file_lock_ != -1;
why not a DCHECK as well?  Under what normal situation would we already have the
mutex, but not the file lock?

https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
File rlz/lib/recursive_cross_process_lock_posix.h (right):

https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
rlz/lib/recursive_cross_process_lock_posix.h:29: pthread_mutex_t
recursive_lock_;
should these members be made private?

[Ivan Korotkov, 2012-11-27 11:37:25]

Mattias, well, technically you're the only one listed in base/files/OWNERS :) If
you suggest someone else I'll add them but this seems a pretty trivial change.

https://codereview.chromium.org/11308196/diff/3018/base/files/important_file_...
File base/files/important_file_writer.h (right):

https://codereview.chromium.org/11308196/diff/3018/base/files/important_file_...
base/files/important_file_writer.h:54: // Save |data| to |path|. Blocks and
writes data on the current thread.
On 2012/11/26 15:48:57, Mattias Nissler wrote:
> Should mention that this uses save-to-tempfile-and-rename, so it's clear what
is
> different from file_util::WriteFile. You might also want to update the name to
> something better. Maybe WriteFileAtomically? That's just from the top of my
> head, feel free to come up with a better name.

WriteFileAtomically sounds good enough to me.

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
File rlz/chromeos/lib/rlz_value_store_chromeos.cc (right):

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:82: DCHECK(CalledOnValidThread());
On 2012/11/26 19:16:51, Roger Tawa wrote:
> why call CalledOnValidThread() in ctor?

Right, removed.

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:87: DCHECK(CalledOnValidThread());
On 2012/11/26 19:16:51, Roger Tawa wrote:
> the base class dtor already calls CalledOnValidThread().

Removed.

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:99: base::Int64ToString(time));
On 2012/11/26 19:16:51, Roger Tawa wrote:
> one extra space.

Done.

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:218: read_only_ = false;
On 2012/11/26 19:16:51, Roger Tawa wrote:
> why set read-only to false here?

NO_SUCH_FILE means RLZ store hasn't been created yet and we're starting with an
empty one. But it should be writable, otherwise we never create one :)

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:259: //
rlz_store_->MarkNeedsEmptyValue(list_name);
On 2012/11/26 19:16:51, Roger Tawa wrote:
> remove line?

Done.

https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
rlz/chromeos/lib/rlz_value_store_chromeos.cc:293: DCHECK(!g_store);
On 2012/11/26 19:16:51, Roger Tawa wrote:
> Maybe add DCHECK(g_lock_depth==1)?  Does not happen today, but if a recursive
> ScopedRlzValueStoreLock were created when the first one failed to get the
> process lock, seems like the mutex will not get release in the dtor.  A DCHECK
> here could catch that.

See my comment in Lock

https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
File rlz/lib/recursive_cross_process_lock_posix.cc (right):

https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
rlz/lib/recursive_cross_process_lock_posix.cc:66: return file_lock_ != -1;
On 2012/11/26 19:16:51, Roger Tawa wrote:
> why not a DCHECK as well?  Under what normal situation would we already have
the
> mutex, but not the file lock?

I think the point of this line was that if the top-level Lock hasn't acquired
file lock, then all Locks down the stack will return false as well.

https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
File rlz/lib/recursive_cross_process_lock_posix.h (right):

https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
rlz/lib/recursive_cross_process_lock_posix.h:29: pthread_mutex_t
recursive_lock_;
On 2012/11/26 19:16:51, Roger Tawa wrote:
> should these members be made private?

Nah, unfortunately, that would make the RCPLock non-POD type.

[Mattias Nissler (ping if slow), 2012-11-27 12:12:21]

On 2012/11/27 11:37:25, Ivan Korotkov wrote:
> Mattias, well, technically you're the only one listed in base/files/OWNERS :)
If
> you suggest someone else I'll add them but this seems a pretty trivial change.

Hm, interesting indeed ;) My name originally ended up there for
file_path_watcher* and that's what it still says in the comment. You might want
to add somebody for base/OWNERS for good measure, for the record you have my
LGTM.

> 
>
https://codereview.chromium.org/11308196/diff/3018/base/files/important_file_...
> File base/files/important_file_writer.h (right):
> 
>
https://codereview.chromium.org/11308196/diff/3018/base/files/important_file_...
> base/files/important_file_writer.h:54: // Save |data| to |path|. Blocks and
> writes data on the current thread.
> On 2012/11/26 15:48:57, Mattias Nissler wrote:
> > Should mention that this uses save-to-tempfile-and-rename, so it's clear
what
> is
> > different from file_util::WriteFile. You might also want to update the name
to
> > something better. Maybe WriteFileAtomically? That's just from the top of my
> > head, feel free to come up with a better name.
> 
> WriteFileAtomically sounds good enough to me.
> 
>
https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
> File rlz/chromeos/lib/rlz_value_store_chromeos.cc (right):
> 
>
https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
> rlz/chromeos/lib/rlz_value_store_chromeos.cc:82:
DCHECK(CalledOnValidThread());
> On 2012/11/26 19:16:51, Roger Tawa wrote:
> > why call CalledOnValidThread() in ctor?
> 
> Right, removed.
> 
>
https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
> rlz/chromeos/lib/rlz_value_store_chromeos.cc:87:
DCHECK(CalledOnValidThread());
> On 2012/11/26 19:16:51, Roger Tawa wrote:
> > the base class dtor already calls CalledOnValidThread().
> 
> Removed.
> 
>
https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
> rlz/chromeos/lib/rlz_value_store_chromeos.cc:99: base::Int64ToString(time));
> On 2012/11/26 19:16:51, Roger Tawa wrote:
> > one extra space.
> 
> Done.
> 
>
https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
> rlz/chromeos/lib/rlz_value_store_chromeos.cc:218: read_only_ = false;
> On 2012/11/26 19:16:51, Roger Tawa wrote:
> > why set read-only to false here?
> 
> NO_SUCH_FILE means RLZ store hasn't been created yet and we're starting with
an
> empty one. But it should be writable, otherwise we never create one :)
> 
>
https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
> rlz/chromeos/lib/rlz_value_store_chromeos.cc:259: //
> rlz_store_->MarkNeedsEmptyValue(list_name);
> On 2012/11/26 19:16:51, Roger Tawa wrote:
> > remove line?
> 
> Done.
> 
>
https://codereview.chromium.org/11308196/diff/3018/rlz/chromeos/lib/rlz_value...
> rlz/chromeos/lib/rlz_value_store_chromeos.cc:293: DCHECK(!g_store);
> On 2012/11/26 19:16:51, Roger Tawa wrote:
> > Maybe add DCHECK(g_lock_depth==1)?  Does not happen today, but if a
recursive
> > ScopedRlzValueStoreLock were created when the first one failed to get the
> > process lock, seems like the mutex will not get release in the dtor.  A
DCHECK
> > here could catch that.
> 
> See my comment in Lock
> 
>
https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
> File rlz/lib/recursive_cross_process_lock_posix.cc (right):
> 
>
https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
> rlz/lib/recursive_cross_process_lock_posix.cc:66: return file_lock_ != -1;
> On 2012/11/26 19:16:51, Roger Tawa wrote:
> > why not a DCHECK as well?  Under what normal situation would we already have
> the
> > mutex, but not the file lock?
> 
> I think the point of this line was that if the top-level Lock hasn't acquired
> file lock, then all Locks down the stack will return false as well.
> 
>
https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
> File rlz/lib/recursive_cross_process_lock_posix.h (right):
> 
>
https://codereview.chromium.org/11308196/diff/3018/rlz/lib/recursive_cross_pr...
> rlz/lib/recursive_cross_process_lock_posix.h:29: pthread_mutex_t
> recursive_lock_;
> On 2012/11/26 19:16:51, Roger Tawa wrote:
> > should these members be made private?
> 
> Nah, unfortunately, that would make the RCPLock non-POD type.

[Ivan Korotkov, 2012-11-28 09:00:04]

+willchan for base/files approval

Rogert, PTAL. This one's ready for submission.

[Roger Tawa OOO till Jul 10th, 2012-11-28 15:04:42]

lgtm

[commit-bot: I haz the power, 2012-11-29 11:25:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/11308196/6002

[commit-bot: I haz the power, 2012-11-29 12:09:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/11308196/3045

[commit-bot: I haz the power, 2012-11-29 14:00:14]

Change committed as 170179

[willchan no longer on Chromium, 2012-12-01 18:01:35]

Hey guys, this particular instance isn't that big of a deal, but in the future
can you make sure to get a proper review here? mnissler said that he was in
base/files/OWNERS for file_path_watcher (btw, now that we can specify per-file
OWNERS, we should probably add that granularity) and it'd be good to ask a base
OWNER for review, yet this change was submitted anyway with his lgtm. I can see
that it was unclear, and it's just one instance, so no big deal, but the area
that I'm really concerned about is the API surface of base, and this changelist
affects that, so I'd really like an appropriate reviewer to examine this.

I've glanced at this change and haven't figured out yet how I feel about the
base/ part of the change. I'd in general like to have a smaller API, and since
this exposes more direct functionality to consumers, I'd like to hear a
justification of why this is necessary. Specifically, why is it necessary to
expose the WriteFileAtomically() function rather than use the
ImportantFileWriter interface like everyone else?

[Ivan Korotkov, 2012-12-02 11:25:03]

Hi Will,

sorry for that -- I must have missed that there's still no LGTM from you and the
CQ didn't complain, obviously.

The reason was to able to use ImportantFileWriter (specifically, atomical file
write operation) from a worker pool task synchronously.

[Ivan Korotkov, 2012-12-02 13:58:32]

Hmmm, BTW, it seems that there is per-file OWNERS support:
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/OWNERS&exa...
, for example