Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(275)

Issue 6720016: Get rid of PrefService::GetMutableDictionary/GetMutableList (Closed)

Created:
9 years, 8 months ago by battre
Modified:
9 years, 7 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, ramant (doing other things)
Visibility:
Public.

Description

Get rid of PrefService::GetMutableDictionary/GetMutableList BUG=77914 TEST=none, trybots remain green Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80469

Patch Set 1 #

Total comments: 4

Patch Set 2 : Added todo for resolving synchronous wait #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -6 lines) Patch
M chrome/browser/net/predictor_api.cc View 1 4 chunks +12 lines, -6 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
battre
Hi. You seem to have written large parts of chrome/browser/net/predictor_api.cc. May I ask you to ...
9 years, 8 months ago (2011-04-04 12:38:11 UTC) #1
jar (doing other things)
http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_api.cc File chrome/browser/net/predictor_api.cc (right): http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_api.cc#newcode495 chrome/browser/net/predictor_api.cc:495: update_startup_list.Get(), This makes very little sense to me. I'm ...
9 years, 8 months ago (2011-04-04 19:18:33 UTC) #2
jar (doing other things)
You can either fix the identified bug (see below) as part of this CL, or ...
9 years, 8 months ago (2011-04-05 06:18:39 UTC) #3
battre
http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_api.cc File chrome/browser/net/predictor_api.cc (right): http://codereview.chromium.org/6720016/diff/1/chrome/browser/net/predictor_api.cc#newcode495 chrome/browser/net/predictor_api.cc:495: update_startup_list.Get(), I thought about this. I think the code ...
9 years, 8 months ago (2011-04-05 14:09:50 UTC) #4
jar (doing other things)
9 years, 8 months ago (2011-04-05 16:03:11 UTC) #5
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??
> > 
>

Powered by Google App Engine
This is Rietveld 408576698