Provide lazy CommitPendingWrites in addition to eager SavePersistentPrefs.

Provide lazy CommitPendingWrites in addition to eager SavePersistentPrefs.

We need to land pending writes to disk before doing chromeos::RestartJob.
However SavePersistentPrefs is not lazy and takes time even if there is nothing to write.
Fix it.

BUG=chromium-os:13102, chromium-os:6924
TEST=Manual

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

[Denis Lagno, 2011-03-22 14:00:52]

ping

[Mattias Nissler (ping if slow), 2011-03-22 14:36:57]

What does that RestartJob do? Restart the chrome process? In this case, the pref
file writing logic should work without the additional commit (through the
JSONPrefStore destructor). Can you elaborate on why this is not enough?

[Denis Lagno, 2011-03-22 14:50:13]

On 2011/03/22 14:36:57, Mattias Nissler wrote:
> What does that RestartJob do? Restart the chrome process? In this case, the
pref
> file writing logic should work without the additional commit (through the
> JSONPrefStore destructor). Can you elaborate on why this is not enough?

Fact is that it does not work.
I do not know exactly what it does (it is buried deeply in chromeos internals)
but I suspect it just kills it by signal and skips usual stuff for sake of
speed.

[Mattias Nissler (ping if slow), 2011-03-22 14:56:00]

On 2011/03/22 14:50:13, Denis Lagno wrote:
> On 2011/03/22 14:36:57, Mattias Nissler wrote:
> > What does that RestartJob do? Restart the chrome process? In this case, the
> pref
> > file writing logic should work without the additional commit (through the
> > JSONPrefStore destructor). Can you elaborate on why this is not enough?
> 
> Fact is that it does not work.
> I do not know exactly what it does (it is buried deeply in chromeos internals)
> but I suspect it just kills it by signal and skips usual stuff for sake of
> speed.

It seems to me that curing a symptom without knowing exactly what is wrong is a
bad idea in general. Just killing the chrome binary may also have other
undesired consequences (we have a lot of sqlite databases for example), so I
think it's worth investigating this.

It should be pretty easy to attach a debugger, set a few breakpoints and then
trigger the restart and see what happens to the Chrome binary (i.e. whether it
gets killed or terminates gracefully).

[Denis Lagno, 2011-03-22 14:57:10]

it sends SIGKILL.

src/platform/login_manager/session_manager_service.cc in chromeos code

[Nikita (slow), 2011-03-22 15:05:49]

cc:cmasone for session_manager.

What we do for Guest mode is send SIGKILL immediately which is not good of
course but there's a issue to fix root cause i.e. make Guest mode launch working
without restarting browser.

This approach was used because
1) Guest mode requires restart to make sure we start from really fresh profile
etc.
2) Waiting Chrome to shutdown is quite long time so we send SIGKILL. In case of
sign out there's a 3 sec timeout for that.

During OOBE/login we use empty profile and for most of the prefs modified during
OOBE/login we call forced save but I'm not sure whether sqlite databases you're
referring have an issue here.

[Mattias Nissler (ping if slow), 2011-03-22 15:35:00]

On 2011/03/22 15:05:49, Nikita Kostylev wrote:
> cc:cmasone for session_manager.
> 
> What we do for Guest mode is send SIGKILL immediately which is not good of
> course but there's a issue to fix root cause i.e. make Guest mode launch
working
> without restarting browser.

But for guest mode, we shouldn't require writing local state prefs either,
correct?

> 
> This approach was used because
> 1) Guest mode requires restart to make sure we start from really fresh profile
> etc.
> 2) Waiting Chrome to shutdown is quite long time so we send SIGKILL. In case
of
> sign out there's a 3 sec timeout for that.

I see, so does that mean there's no problem for regular login?

> 
> During OOBE/login we use empty profile and for most of the prefs modified
during
> OOBE/login we call forced save but I'm not sure whether sqlite databases
you're
> referring have an issue here.

That was just an example. My point was that there might be other moving parts
that get shot down on KILL that may or may not be a problem in the future.

[Denis Lagno, 2011-03-22 15:54:42]

On 2011/03/22 15:35:00, Mattias Nissler wrote:
> But for guest mode, we shouldn't require writing local state prefs either,
correct?

this RestartJob is called when we transition from login screen into guest mode. 
Login screen keeps some of its settings in local state prefs.

> I see, so does that mean there's no problem for regular login?

problems are always possible.
There is CL to decrease commit interval for stable builds:
http://codereview.chromium.org/6713033/
Current commit interval is insane 10 seconds.

[Mattias Nissler (ping if slow), 2011-03-22 16:52:47]

On 2011/03/22 15:54:42, Denis Lagno wrote:
> On 2011/03/22 15:35:00, Mattias Nissler wrote:
> > But for guest mode, we shouldn't require writing local state prefs either,
> correct?
> 
> this RestartJob is called when we transition from login screen into guest
mode. 
> Login screen keeps some of its settings in local state prefs.

I see, so we restart chrome upon entering guest mode? I still think the right
solution would be what Nikita says (i.e. not hard-killing the process). That
said, I'm fine with introducing this as a temporary workaround with a TODO
saying why we need it and when it can be removed.

> 
> > I see, so does that mean there's no problem for regular login?
> 
> problems are always possible.
> There is CL to decrease commit interval for stable builds:
> http://codereview.chromium.org/6713033/
> Current commit interval is insane 10 seconds.

I wouldn't consider the 10 seconds interval insane, but rather the expectation
of killing a process and still expecting it to be able to persist its state ;)

[Mattias Nissler (ping if slow), 2011-03-22 16:56:20]

http://codereview.chromium.org/6713032/diff/11/chrome/browser/chromeos/cros/l...
File chrome/browser/chromeos/cros/login_library.cc (right):

http://codereview.chromium.org/6713032/diff/11/chrome/browser/chromeos/cros/l...
chrome/browser/chromeos/cros/login_library.cc:140: return
chromeos::RestartJob(pid, command_line.c_str());
On which thread does this run? The CommitPendingWrites runs on the file thread,
so we could race here, killing chrome before the write task finishes.

[Denis Lagno, 2011-03-22 16:58:12]

http://codereview.chromium.org/6713032/diff/11/chrome/browser/chromeos/cros/l...
File chrome/browser/chromeos/cros/login_library.cc (right):

http://codereview.chromium.org/6713032/diff/11/chrome/browser/chromeos/cros/l...
chrome/browser/chromeos/cros/login_library.cc:140: return
chromeos::RestartJob(pid, command_line.c_str());
On 2011/03/22 16:56:20, Mattias Nissler wrote:
> On which thread does this run? The CommitPendingWrites runs on the file
thread,
> so we could race here, killing chrome before the write task finishes.

absoulutely.  I am adding notification to fix this.

[Denis Lagno, 2011-03-22 17:08:40]

http://codereview.chromium.org/6713032/diff/11/chrome/browser/chromeos/cros/l...
File chrome/browser/chromeos/cros/login_library.cc (right):

http://codereview.chromium.org/6713032/diff/11/chrome/browser/chromeos/cros/l...
chrome/browser/chromeos/cros/login_library.cc:140: return
chromeos::RestartJob(pid, command_line.c_str());
OTOH it looks like RestartJob is invoked by sending dbus message, so there is
chance it is safe:)

[Mattias Nissler (ping if slow), 2011-03-22 17:27:02]

http://codereview.chromium.org/6713032/diff/11/chrome/browser/chromeos/cros/l...
File chrome/browser/chromeos/cros/login_library.cc (right):

http://codereview.chromium.org/6713032/diff/11/chrome/browser/chromeos/cros/l...
chrome/browser/chromeos/cros/login_library.cc:140: return
chromeos::RestartJob(pid, command_line.c_str());
On 2011/03/22 17:08:40, Denis Lagno wrote:
> OTOH it looks like RestartJob is invoked by sending dbus message, so there is
> chance it is safe:)

Does that mean you're willing to take chances? ;)

I don't feel strong about what we end up with as long as we actually fix the
problem in a reliable way.

[Denis Lagno, 2011-03-23 14:37:43]

adding reviewer from content/OWNERS

http://codereview.chromium.org/6713032/diff/11/chrome/browser/chromeos/cros/l...
File chrome/browser/chromeos/cros/login_library.cc (right):

http://codereview.chromium.org/6713032/diff/11/chrome/browser/chromeos/cros/l...
chrome/browser/chromeos/cros/login_library.cc:140: return
chromeos::RestartJob(pid, command_line.c_str());
On 2011/03/22 17:27:02, Mattias Nissler wrote:
> On 2011/03/22 17:08:40, Denis Lagno wrote:
> > OTOH it looks like RestartJob is invoked by sending dbus message, so there
is
> > chance it is safe:)
> 
> Does that mean you're willing to take chances? ;)
> 
> I don't feel strong about what we end up with as long as we actually fix the
> problem in a reliable way.

Done.

[Chris Masone, 2011-03-23 14:49:07]

login_library.cc changes LGTM

Am I not an OWNER of that?  I should be...

[Denis Lagno, 2011-03-23 14:55:49]

On 2011/03/23 14:49:07, Chris Masone wrote:
> Am I not an OWNER of that?  I should be...

no at this moment

[Mattias Nissler (ping if slow), 2011-03-23 15:12:37]

I think the approach for generating the notification is not the correct one (see
comment line), therefore I didn't look in detail at the entire patch. I hope
what I have to say makes sense, feel free to bug me with questions if it doesn't
;)

http://codereview.chromium.org/6713032/diff/12007/chrome/common/json_pref_sto...
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/6713032/diff/12007/chrome/common/json_pref_sto...
chrome/common/json_pref_store.h:51: void SetNotificationSource(const
NotificationSource& source);
This is not well-defined. Note that a PrefStore can be hooked up to multiple
PrefServices, thus pushing a source down and sending the notification from there
is not a good idea.

Note that PrefStore already has an observer interface to you could use to
propagate the notification back up. Why not add your stuff there?

We have invested considerable time recently to detangle all Pref classes. One of
the goals was to concentrate sending notifications in PrefNotifierImpl, and your
CL adds back in a notification from deep within the pref system, which we like
to avoid since it decouples the innards of the implementation from the
interface, which is implemented in PrefService.

[brettw, 2011-03-23 15:49:02]

I agree with Mattias. I don't see why the important file write is sending pref
notifications. This component looks more general than just for the pref system.
If it's going to be bound only to the pref system, it should be the
PrefFileWriter.

[Mattias Nissler (ping if slow), 2011-03-23 15:56:53]

On 2011/03/23 15:49:02, brettw wrote:
> I agree with Mattias. I don't see why the important file write is sending pref
> notifications. This component looks more general than just for the pref
system.
> If it's going to be bound only to the pref system, it should be the
> PrefFileWriter.

My request is actually even more profound: The point in code where the pref
subsystem interfaces with the outside world is PrefService and PrefNotifier, so
that's where sending the notification belongs. There's already an observer
interface in place for PrefStore that you can extend to propagate the
information to the proper place.

[Denis Lagno, 2011-03-23 20:46:42]

I do not agree that it is not well-defined or flawed.  Just need to rename
PREF_COMMITTED to more general sounding DATA_COMMMITTED.  SetNotificationSource
just sets generic notification source (you can use pointer of any type for it,
it is not limited to PrefService*) just for purpose of matching/comparison. 
ImportantFileWriter just sends general notification.

However I strongly agree that CL is overcomplicated and is not worth committing
in its previous form.  So I switched to simple solution: after calling
CommitPendingWrite post task on file thread, so it will be executed after write
(this logic would fail if CommitPendingWrite will ever post chain of tasks on
file thread but I assume it will never happen (and added assert for that)).

[Mattias Nissler (ping if slow), 2011-03-23 21:25:25]

On 2011/03/23 20:46:42, Denis Lagno wrote:
> I do not agree that it is not well-defined or flawed.  Just need to rename
> PREF_COMMITTED to more general sounding DATA_COMMMITTED. 
SetNotificationSource
> just sets generic notification source (you can use pointer of any type for it,
> it is not limited to PrefService*) just for purpose of matching/comparison. 

That is exactly the problem. In general, the JsonPrefStore might be hooked up
with several PrefServices, thus the PrefService pointer cannot be used for
matching the notifications you receive. This is what I was referring to when I
said the approach was not well-defined.

> ImportantFileWriter just sends general notification.
> 
> However I strongly agree that CL is overcomplicated and is not worth
committing
> in its previous form.  So I switched to simple solution: after calling
> CommitPendingWrite post task on file thread, so it will be executed after
write
> (this logic would fail if CommitPendingWrite will ever post chain of tasks on
> file thread but I assume it will never happen (and added assert for that)).

I think this is a reasonable solution given that the real problem is the hard
kill of the Chrome process which I understand is supposed to be reworked.

[Denis Lagno, 2011-03-23 22:09:59]

On 2011/03/23 21:25:25, Mattias Nissler wrote:
> On 2011/03/23 20:46:42, Denis Lagno wrote:
> > it is not limited to PrefService*) just for purpose of matching/comparison. 
> That is exactly the problem. In general, the JsonPrefStore might be hooked up
> with several PrefServices, thus the PrefService pointer cannot be used for
> matching the notifications you receive. This is what I was referring to when I
> said the approach was not well-defined.

Oh, I see.

[Mattias Nissler (ping if slow), 2011-03-25 14:59:18]

On 2011/03/23 22:09:59, Denis Lagno wrote:
> On 2011/03/23 21:25:25, Mattias Nissler wrote:
> > On 2011/03/23 20:46:42, Denis Lagno wrote:
> > > it is not limited to PrefService*) just for purpose of
matching/comparison. 
> > That is exactly the problem. In general, the JsonPrefStore might be hooked
up
> > with several PrefServices, thus the PrefService pointer cannot be used for
> > matching the notifications you receive. This is what I was referring to when
I
> > said the approach was not well-defined.
> 
> Oh, I see.

Why has this been committed without LGTM?

[Denis Lagno, 2011-03-25 15:13:50]

On 2011/03/25 14:59:18, Mattias Nissler wrote:
> Why has this been committed without LGTM?

Ehhr, I thought you are OK with it: I waited more than day.

Comment on code, I will fix them in followup CL or revert.

[Mattias Nissler (ping if slow), 2011-03-25 15:25:51]

On 2011/03/25 15:13:50, Denis Lagno wrote:
> On 2011/03/25 14:59:18, Mattias Nissler wrote:
> > Why has this been committed without LGTM?
> 
> Ehhr, I thought you are OK with it: I waited more than day.

Huh? You had only a partial LGTM from Chris for login_library.cc. Our process
says that you cannot commit without LGTM for all of your code. No matter how
long you wait.

I think one of the problems here is that I didn't realize that in addition to
the conversation we had in comments you also made changes I should have looked
at. I'm sorry for not noticing. However, people usually indicate in their
comments that there's an update to the code. Also, if somebody doesn't reply in
time, feel free to ping them.

> Comment on code, I will fix them in followup CL or revert.

OK, I'll look at it now.

[Mattias Nissler (ping if slow), 2011-03-25 15:29:57]

LGTM with a nit for everything except login_library.cc. If you haven't already,
you should make sure whoever feels responsible for that is happy with the latest
revision (I believe you made changes since Chris checked).

http://codereview.chromium.org/6713032/diff/18001/chrome/common/important_fil...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/6713032/diff/18001/chrome/common/important_fil...
chrome/common/important_file_writer.cc:110: FROM_HERE, new
WriteToDiskTask(path_, data))) {
any reason for the formatting change here? I found the previous version easier
to read.

[Denis Lagno, 2011-03-25 15:41:22]

> I think one of the problems here is that I didn't realize that in addition to
the conversation we had in comments you also made changes I should have looked
at. I'm sorry for not noticing. However, people usually indicate in their
comments that there's an update to the code.

I've clearly indicated "I switched to simple solution" -- it is past tense.

http://codereview.chromium.org/6713032/diff/18001/chrome/common/important_fil...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/6713032/diff/18001/chrome/common/important_fil...
chrome/common/important_file_writer.cc:110: FROM_HERE, new
WriteToDiskTask(path_, data))) {
On 2011/03/25 15:29:57, Mattias Nissler wrote:
> any reason for the formatting change here? I found the previous version easier
> to read.

In old versions of CL there was argument added, now it is gone so it remained in
CL by mistake.

However at least one reason to prefer indentation on the right is that if you
ever need to rename some identifiers then indentation on the right will just
remain correct while indentation on the left needs to be reformatted.

[Mattias Nissler (ping if slow), 2011-03-25 18:14:58]

On 2011/03/25 15:41:22, Denis Lagno wrote:
> > I think one of the problems here is that I didn't realize that in addition
to
> the conversation we had in comments you also made changes I should have looked
> at. I'm sorry for not noticing. However, people usually indicate in their
> comments that there's an update to the code.
> 
> I've clearly indicated "I switched to simple solution" -- it is past tense.

I agree, and I'm sorry I missed the code update. However, a ping would have
nicely resolved the uncertainty.

> 
>
http://codereview.chromium.org/6713032/diff/18001/chrome/common/important_fil...
> File chrome/common/important_file_writer.cc (right):
> 
>
http://codereview.chromium.org/6713032/diff/18001/chrome/common/important_fil...
> chrome/common/important_file_writer.cc:110: FROM_HERE, new
> WriteToDiskTask(path_, data))) {
> On 2011/03/25 15:29:57, Mattias Nissler wrote:
> > any reason for the formatting change here? I found the previous version
easier
> > to read.
> 
> In old versions of CL there was argument added, now it is gone so it remained
in
> CL by mistake.
> 
> However at least one reason to prefer indentation on the right is that if you
> ever need to rename some identifiers then indentation on the right will just
> remain correct while indentation on the left needs to be reformatted.

I guess usually we optimize code for readability in the state it is currently
in, and not for possible future modifications. Anyway, it's just a nit and I
think it's not worth the effort of a follow-up CL to change it.