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

Issue 215011: Fixed a few data races on reference counters. (Closed)

Created:
11 years, 3 months ago by Timur Iskhodzhanov
Modified:
9 years, 6 months ago
CC:
chromium-reviews_googlegroups.com, darin (slow to review), pam+watch_chromium.org, Erik does not do reviews, Ben Goodger (Google)
Visibility:
Public.

Description

Fixed a few data races on reference counters. BUG=18488 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26476

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -3 lines) Patch
M chrome/browser/extensions/user_script_master.h View 1 chunk +1 line, -1 line 2 comments Download
M chrome/browser/history/history_marshaling.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M net/base/host_resolver.h View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 14 (0 generated)
brettw
LGTM
11 years, 3 months ago (2009-09-17 18:16:18 UTC) #1
Timur Iskhodzhanov
On 2009/09/17 18:16:18, brettw wrote: > LGTM Do I need more LGTMs? Waiting for the ...
11 years, 3 months ago (2009-09-17 18:21:46 UTC) #2
cpu_(ooo_6.6-7.5)
LGTM On 2009/09/17 18:21:46, Timur Iskhodzhanov wrote: > On 2009/09/17 18:16:18, brettw wrote: > > ...
11 years, 3 months ago (2009-09-17 18:22:37 UTC) #3
eroman
The refcountedthreadsafe for HostResolver is unfortunately necessary because although it is supposed to live on ...
11 years, 3 months ago (2009-09-17 18:57:30 UTC) #4
laforge
Please ping me as soon as this gets committed, I'd like to pull it on ...
11 years, 3 months ago (2009-09-17 19:05:31 UTC) #5
Timur Iskhodzhanov
On 2009/09/17 18:57:30, eroman wrote: > However I don't understand why you have made LoadLog ...
11 years, 3 months ago (2009-09-17 19:08:44 UTC) #6
eroman
> If this is performance critical I can cancel the LoadLog change and (once I ...
11 years, 3 months ago (2009-09-17 19:14:41 UTC) #7
Timur Iskhodzhanov
On 2009/09/17 19:14:41, eroman wrote: > > If this is performance critical I can cancel ...
11 years, 3 months ago (2009-09-17 19:18:50 UTC) #8
eroman
Sounds good, thanks!
11 years, 3 months ago (2009-09-17 19:20:49 UTC) #9
laforge
For the record, I merged these changes on to the 211 branch that I'm kicking ...
11 years, 3 months ago (2009-09-17 20:32:26 UTC) #10
Timur Iskhodzhanov
On 2009/09/17 20:32:26, laforge wrote: > For the record, I merged these changes on to ...
11 years, 3 months ago (2009-09-17 20:47:08 UTC) #11
Aaron Boodman
http://codereview.chromium.org/215011/diff/7005/4006 File chrome/browser/extensions/user_script_master.h (right): http://codereview.chromium.org/215011/diff/7005/4006#newcode25 Line 25: class UserScriptMaster : public base::RefCountedThreadSafe<UserScriptMaster>, Does this class ...
11 years, 3 months ago (2009-09-17 22:59:34 UTC) #12
Timur Iskhodzhanov
On 2009/09/17 19:20:49, eroman wrote: > Sounds good, thanks! I've reproduced the report on LoadLog ...
11 years, 3 months ago (2009-09-18 18:40:06 UTC) #13
eroman
11 years, 3 months ago (2009-09-18 19:00:32 UTC) #14
Ah okay. I think I understand the problem now, thanks!

(If a request is cancelled, the HostResolverImpl::Job will be freed on the
worker thread, so in turn the HostResolverImpl::Request gets freed on the worker
thread, and calls LoadLog::Release()).

I would like to solve this by clearing the LoadLog within
HostResolverImpl::CancelRequest(), rather than making it RefCountedThreadSafe.
Can file this bug against me?

Powered by Google App Engine
This is Rietveld 408576698