Chromium Code Reviews
Help | Chromium Project | Sign in
(726)

Issue 338065: Add the ability for objects which derive from RefCountedThreadSafe to specify... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 5 months ago by jam
Modified:
2 years, 11 months ago
Reviewers:
darin
CC:
chromium-reviews_googlegroups.com, brettw+cc_chromium.org, ben+cc_chromium.org
Visibility:
Public.

Description

Add the ability for objects which derive from RefCountedThreadSafe to specify a destructor trait. This allows browser objects to specify which thread they're terminated on. The benefit is we avoid the need to do manual ref counting when an object posts tasks to itself on different threads, if an object must be destructed on a specific thread.

This patch adds initial support and only shows one example with ResourceMessageFilter. I will do the rest in a follow-up patch to keep things small.

BUG=25354
TEST=added unit tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30688

Patch Set 1 : '' #

Patch Set 2 : '' #

Patch Set 3 : Make IPC::ChannelProxy::MessageFilter not ref counted #

Patch Set 4 : Make MessageFilter be ref counted again #

Total comments: 5

Patch Set 5 : '' #

Patch Set 6 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+199 lines, -331 lines) Lint Patch
M base/ref_counted.h View 1 2 3 4 5 3 chunks +26 lines, -3 lines 0 comments 1 errors Download
M chrome/browser/chrome_thread.h View 1 2 3 4 1 chunk +29 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/chrome_thread_unittest.cc View 1 2 3 4 1 chunk +85 lines, -49 lines 0 comments 3 errors Download
D chrome/browser/renderer_host/file_system_accessor.h View 1 2 3 4 1 chunk +0 lines, -74 lines 0 comments 0 errors Download
D chrome/browser/renderer_host/file_system_accessor.cc View 1 2 3 4 1 chunk +0 lines, -44 lines 0 comments 0 errors Download
D chrome/browser/renderer_host/file_system_accessor_unittest.cc View 1 2 3 4 1 chunk +0 lines, -115 lines 0 comments 0 errors Download
M chrome/browser/renderer_host/resource_message_filter.h View 1 2 3 4 4 chunks +4 lines, -5 lines 0 comments 0 errors Download
M chrome/browser/renderer_host/resource_message_filter.cc View 1 2 3 4 5 chunks +34 lines, -37 lines 0 comments 0 errors Download
M chrome/chrome.gyp View 1 2 3 4 2 chunks +0 lines, -3 lines 0 comments 0 errors Download
M ipc/ipc_channel_proxy.h View 1 2 3 4 2 chunks +15 lines, -1 line 0 comments 0 errors Download
M ipc/ipc_channel_proxy.cc View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 3
jam
4 years, 5 months ago #1
darin
LGTM! http://codereview.chromium.org/338065/diff/9052/9069 File chrome/browser/chrome_thread.h (right): http://codereview.chromium.org/338065/diff/9052/9069#newcode142 Line 142: DeleteSoon(thread, FROM_HERE, x); unfortunately, the value of ...
4 years, 5 months ago #2
jam
4 years, 5 months ago #3
http://codereview.chromium.org/338065/diff/9052/9068
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h (right):

http://codereview.chromium.org/338065/diff/9052/9068#newcode50
Line 50: friend struct
On 2009/10/30 19:57:59, darin wrote:
> if we make DefaultRefCountedThreadSafeTraits call back to a static function on
> RefCountedThreadSafe that then does the delete call, then we can avoid
changing
> these friend declarations.

Done.

http://codereview.chromium.org/338065/diff/9052/9071
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/338065/diff/9052/9071#newcode410
Line 410: ChromeThread::DeleteOnIOThread::Destruct(this);
On 2009/10/30 19:57:59, darin wrote:
> nit: might be nice to use the more explicit:
> 
>   ChromeThread::DeleteSoon(ChromeThread::IO, FROM_HERE, this);

That would change the semantics a bit, since before the RMF would get deleted
right away if it's on the correct thread, but doing DeleteSoon would delay it
until the stack unwinds.  This could result in holding on to resources or memory
leaks in valgrind.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6