Get rid of PrefService::GetMutableDictionary/GetMutableList

Get rid of PrefService::GetMutableDictionary/GetMutableList

BUG=77914
TEST=none, trybots remain green

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

[battre, 2011-04-04 12:38:11]

Hi.

You seem to have written large parts of chrome/browser/net/predictor_api.cc. May
I ask you to review this small CL?

The motivation is outlined in the bug.

Thanks,
Dominic

[jar (doing other things), 2011-04-04 19:18:33]

http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_ap...
File chrome/browser/net/predictor_api.cc (right):

http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_ap...
chrome/browser/net/predictor_api.cc:495: update_startup_list.Get(),
This makes very little sense to me.  I'm *guessing* that Get() pulls out the
underlying list (since you don't have any other changes... such as changing the
called function).  Hence your destructor for lies 488 and 489 will fire, despite
the fact that the changes have not been made to the list (they will be made deep
in the post tasks... which won't happen or a while... and will be on another
thread).

Bottom line: This does not appear to address the concern of the bug (other than
adding odd complexity).

What am I missing??

[jar (doing other things), 2011-04-05 06:18:39]

You can either fix the identified bug (see below) as part of this CL, or file a
bug against me (jar) to fix this prior to your change.

http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_ap...
File chrome/browser/net/predictor_api.cc (right):

http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_ap...
chrome/browser/net/predictor_api.cc:495: update_startup_list.Get(),
I suspect this may actually be the source of a larger bug.  The list is being
modified on a second thread (presumably the IO thread). This mutable list is
probably being scanned asynchronously on the UI thread to persist the values. 
The resulting race may be very dangerous.   I assume the fix is to call to the
IO thread to get the data, but than transcribe it onto the MutableList back on
the UI thread.  

Your change merely highlights the fact that the list value is bein manipulated
on another thread.  This larger bug should be fixed, and then you can wrap the
UI thread changes in your planned ListPrefUpdate notifier.

On 2011/04/04 19:18:33, jar wrote:
> This makes very little sense to me.  I'm *guessing* that Get() pulls out the
> underlying list (since you don't have any other changes... such as changing
the
> called function).  Hence your destructor for lies 488 and 489 will fire,
despite
> the fact that the changes have not been made to the list (they will be made
deep
> in the post tasks... which won't happen or a while... and will be on another
> thread).
> 
> Bottom line: This does not appear to address the concern of the bug (other
than
> adding odd complexity).
> 
> What am I missing??

[battre, 2011-04-05 14:09:50]

http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_ap...
File chrome/browser/net/predictor_api.cc (right):

http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_ap...
chrome/browser/net/predictor_api.cc:495: update_startup_list.Get(),
I thought about this.

I think the code is safe because of the "completion.Wait()" below. We grab the
dictionaries on the UI thread, post the task that does the modifications and
halt the UI thread until the modifications are completed. As only the UI thread
can access the preferences and this thread is blocked, nothing serious can
happen.

The scope of update_startup_list and update_referral_list ends after the Wait()
returns. Therefore, the "preference changed" signal is sent after the changes
have been done.

I wonder however about the impact of the Wait() on the UI thread, which would be
nice to avoid. OTOH, the only caller of SavePredictorStateForNextStartupAndTrim
is the browser shutdown code.

How do you suggest to proceed?


On 2011/04/05 06:18:39, jar wrote:
> I suspect this may actually be the source of a larger bug.  The list is being
> modified on a second thread (presumably the IO thread). This mutable list is
> probably being scanned asynchronously on the UI thread to persist the values. 
> The resulting race may be very dangerous.   I assume the fix is to call to the
> IO thread to get the data, but than transcribe it onto the MutableList back on
> the UI thread.  
> 
> Your change merely highlights the fact that the list value is bein manipulated
> on another thread.  This larger bug should be fixed, and then you can wrap the
> UI thread changes in your planned ListPrefUpdate notifier.
> 
> On 2011/04/04 19:18:33, jar wrote:
> > This makes very little sense to me.  I'm *guessing* that Get() pulls out the
> > underlying list (since you don't have any other changes... such as changing
> the
> > called function).  Hence your destructor for lies 488 and 489 will fire,
> despite
> > the fact that the changes have not been made to the list (they will be made
> deep
> > in the post tasks... which won't happen or a while... and will be on another
> > thread).
> > 
> > Bottom line: This does not appear to address the concern of the bug (other
> than
> > adding odd complexity).
> > 
> > What am I missing??
>

[jar (doing other things), 2011-04-05 16:03:11]

LGTM on your change...

but I have a LOT of concerns about the underlying code.  I really think it is
prone to participating in a shutdown-crash (via a deadlock).  See comments
below.

http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_ap...
File chrome/browser/net/predictor_api.cc (right):

http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_ap...
chrome/browser/net/predictor_api.cc:495: update_startup_list.Get(),
<sigh>  Ok... I'm convinced your change is correct, for this context.

This approach of pausing a thread, and waiting for another thread to complete
(which existed before you touched the code) is fraught with peril... and is
indeed the thing that deadlocks are made of.  If another programmer on the IO
thread decided to do the same thing, waiting for a UI completion... then both
threads would block.  :-(.

We need to look closer and see who else uses this questionable pattern.  It may
be that such are required at shutdown... but there needs to be a LOT more
coordination about who can do such things IMO.  This could easily be a major
source of hung (dirty/deadlocked) shutdowns.

On 2011/04/05 14:09:50, battre wrote:
> I thought about this.
> 
> I think the code is safe because of the "completion.Wait()" below. We grab the
> dictionaries on the UI thread, post the task that does the modifications and
> halt the UI thread until the modifications are completed. As only the UI
thread
> can access the preferences and this thread is blocked, nothing serious can
> happen.
> 
> The scope of update_startup_list and update_referral_list ends after the
Wait()
> returns. Therefore, the "preference changed" signal is sent after the changes
> have been done.
> 
> I wonder however about the impact of the Wait() on the UI thread, which would
be
> nice to avoid. OTOH, the only caller of
SavePredictorStateForNextStartupAndTrim
> is the browser shutdown code.
> 
> How do you suggest to proceed?
> 
> 
> On 2011/04/05 06:18:39, jar wrote:
> > I suspect this may actually be the source of a larger bug.  The list is
being
> > modified on a second thread (presumably the IO thread). This mutable list is
> > probably being scanned asynchronously on the UI thread to persist the
values. 
> > The resulting race may be very dangerous.   I assume the fix is to call to
the
> > IO thread to get the data, but than transcribe it onto the MutableList back
on
> > the UI thread.  
> > 
> > Your change merely highlights the fact that the list value is bein
manipulated
> > on another thread.  This larger bug should be fixed, and then you can wrap
the
> > UI thread changes in your planned ListPrefUpdate notifier.
> > 
> > On 2011/04/04 19:18:33, jar wrote:
> > > This makes very little sense to me.  I'm *guessing* that Get() pulls out
the
> > > underlying list (since you don't have any other changes... such as
changing
> > the
> > > called function).  Hence your destructor for lies 488 and 489 will fire,
> > despite
> > > the fact that the changes have not been made to the list (they will be
made
> > deep
> > > in the post tasks... which won't happen or a while... and will be on
another
> > > thread).
> > > 
> > > Bottom line: This does not appear to address the concern of the bug (other
> > than
> > > adding odd complexity).
> > > 
> > > What am I missing??
> > 
>