diff options
author | jam@chromium.org <jam@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-11-02 06:10:30 +0000 |
---|---|---|
committer | jam@chromium.org <jam@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-11-02 06:10:30 +0000 |
commit | f671062f8c572d6729173c4ac7f058a1a89b5a1d (patch) | |
tree | 3eebe393f6f20310ba812662d092c3ca3c3c4d9e | |
parent | 6fad26338ed6119903826156f307e20fe6657c31 (diff) | |
download | chromium_src-f671062f8c572d6729173c4ac7f058a1a89b5a1d.zip chromium_src-f671062f8c572d6729173c4ac7f058a1a89b5a1d.tar.gz chromium_src-f671062f8c572d6729173c4ac7f058a1a89b5a1d.tar.bz2 |
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
Review URL: http://codereview.chromium.org/338065
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@30688 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | base/ref_counted.h | 29 | ||||
-rw-r--r-- | chrome/browser/chrome_thread.h | 29 | ||||
-rw-r--r-- | chrome/browser/chrome_thread_unittest.cc | 156 | ||||
-rw-r--r-- | chrome/browser/renderer_host/file_system_accessor.cc | 44 | ||||
-rw-r--r-- | chrome/browser/renderer_host/file_system_accessor.h | 74 | ||||
-rw-r--r-- | chrome/browser/renderer_host/file_system_accessor_unittest.cc | 115 | ||||
-rw-r--r-- | chrome/browser/renderer_host/resource_message_filter.cc | 70 | ||||
-rw-r--r-- | chrome/browser/renderer_host/resource_message_filter.h | 8 | ||||
-rwxr-xr-x | chrome/chrome.gyp | 3 | ||||
-rw-r--r-- | ipc/ipc_channel_proxy.cc | 6 | ||||
-rw-r--r-- | ipc/ipc_channel_proxy.h | 16 |
11 files changed, 209 insertions, 341 deletions
diff --git a/base/ref_counted.h b/base/ref_counted.h index e04bb7c..ea532a4 100644 --- a/base/ref_counted.h +++ b/base/ref_counted.h @@ -93,6 +93,21 @@ class RefCounted : public subtle::RefCountedBase { DISALLOW_COPY_AND_ASSIGN(RefCounted<T>); }; +// Forward declaration. +template <class T, typename Traits> class RefCountedThreadSafe; + +// Default traits for RefCountedThreadSafe<T>. Deletes the object when its ref +// count reaches 0. Overload to delete it on a different thread etc. +template<typename T> +struct DefaultRefCountedThreadSafeTraits { + static void Destruct(T* x) { + // Delete through RefCountedThreadSafe to make child classes only need to be + // friend with RefCountedThreadSafe instead of this struct, which is an + // implementation detail. + RefCountedThreadSafe<T, DefaultRefCountedThreadSafeTraits>::DeleteInternal(x); + } +}; + // // A thread-safe variant of RefCounted<T> // @@ -100,7 +115,12 @@ class RefCounted : public subtle::RefCountedBase { // ... // }; // -template <class T> +// If you're using the default trait, then you may choose to add compile time +// asserts that no one else is deleting your object. i.e. +// private: +// friend struct base::RefCountedThreadSafe<MyFoo>; +// ~MyFoo(); +template <class T, typename Traits = DefaultRefCountedThreadSafeTraits<T> > class RefCountedThreadSafe : public subtle::RefCountedThreadSafeBase { public: RefCountedThreadSafe() { } @@ -112,12 +132,15 @@ class RefCountedThreadSafe : public subtle::RefCountedThreadSafeBase { void Release() { if (subtle::RefCountedThreadSafeBase::Release()) { - delete static_cast<T*>(this); + Traits::Destruct(static_cast<T*>(this)); } } private: - DISALLOW_COPY_AND_ASSIGN(RefCountedThreadSafe<T>); + friend struct DefaultRefCountedThreadSafeTraits<T>; + static void DeleteInternal(T* x) { delete x; } + + DISALLOW_COPY_AND_ASSIGN(RefCountedThreadSafe); }; // diff --git a/chrome/browser/chrome_thread.h b/chrome/browser/chrome_thread.h index 4240127..8f64dde 100644 --- a/chrome/browser/chrome_thread.h +++ b/chrome/browser/chrome_thread.h @@ -125,6 +125,35 @@ class ChromeThread : public base::Thread { // sets identifier to its ID. Otherwise returns false. static bool GetCurrentThreadIdentifier(ID* identifier); + // Use these templates in conjuction with RefCountedThreadSafe when you want + // to ensure that an object is deleted on a specific thread. This is needed + // when an object can hop between threads (i.e. IO -> FILE -> IO), and thread + // switching delays can mean that the final IO tasks executes before the FILE + // task's stack unwinds. This would lead to the object destructing on the + // FILE thread, which often is not what you want (i.e. to unregister from + // NotificationService, to notify other objects on the creating thread etc). + template<ID thread> + struct DeleteOnThread { + template<typename T> + static void Destruct(T* x) { + if (CurrentlyOn(thread)) { + delete x; + } else { + DeleteSoon(thread, FROM_HERE, x); + } + } + }; + + // Sample usage: + // class Foo + // : public base::RefCountedThreadSafe< + // Foo, ChromeThread::DeleteOnIOThread> { + struct DeleteOnUIThread : public DeleteOnThread<UI> { }; + struct DeleteOnIOThread : public DeleteOnThread<IO> { }; + struct DeleteOnFileThread : public DeleteOnThread<FILE> { }; + struct DeleteOnDBThread : public DeleteOnThread<DB> { }; + struct DeleteOnWebKitThread : public DeleteOnThread<WEBKIT> { }; + private: // Common initialization code for the constructors. void Initialize(); diff --git a/chrome/browser/chrome_thread_unittest.cc b/chrome/browser/chrome_thread_unittest.cc index a6c9981..62f8097 100644 --- a/chrome/browser/chrome_thread_unittest.cc +++ b/chrome/browser/chrome_thread_unittest.cc @@ -1,72 +1,108 @@ -// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. +// Copyright (c) 2009 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include <vector> - #include "chrome/browser/chrome_thread.h" #include "testing/gtest/include/gtest/gtest.h" #include "testing/platform_test.h" -typedef PlatformTest ChromeThreadTest; - -TEST_F(ChromeThreadTest, Get) { - /* - // TODO(jabdelmalek): rewrite this test when the change to delete objects on - // a specific thread lands. - scoped_ptr<ChromeThread> io_thread; - scoped_ptr<ChromeThread> file_thread; - scoped_ptr<ChromeThread> db_thread; - - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::IO) == NULL); - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::FILE) == NULL); - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::DB) == NULL); - - // Phase 1: Create threads. - - io_thread.reset(new ChromeThread(ChromeThread::IO)); - file_thread.reset(new ChromeThread(ChromeThread::FILE)); - db_thread.reset(new ChromeThread(ChromeThread::DB)); - - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::IO) == NULL); - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::FILE) == NULL); - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::DB) == NULL); - - // Phase 2: Start the threads. - - io_thread->Start(); - file_thread->Start(); - db_thread->Start(); - - EXPECT_TRUE(io_thread->message_loop() != NULL); - EXPECT_TRUE(file_thread->message_loop() != NULL); - EXPECT_TRUE(db_thread->message_loop() != NULL); - - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::IO) == - io_thread->message_loop()); - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::FILE) == - file_thread->message_loop()); - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::DB) == - db_thread->message_loop()); - - // Phase 3: Stop the threads. - - io_thread->Stop(); - file_thread->Stop(); - db_thread->Stop(); +class ChromeThreadTest : public testing::Test { + public: + void Release() { + CHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE)); + loop_.PostTask(FROM_HERE, new MessageLoop::QuitTask); + } + + protected: + virtual void SetUp() { + file_thread_.reset(new ChromeThread(ChromeThread::FILE)); + io_thread_.reset(new ChromeThread(ChromeThread::IO)); + file_thread_->Start(); + io_thread_->Start(); + } + + virtual void TearDown() { + file_thread_->Stop(); + io_thread_->Stop(); + } + + static void BasicFunction(MessageLoop* message_loop) { + CHECK(ChromeThread::CurrentlyOn(ChromeThread::IO)); + message_loop->PostTask(FROM_HERE, new MessageLoop::QuitTask); + } + + class DummyTask : public Task { + public: + DummyTask(bool* deleted) : deleted_(deleted) { } + ~DummyTask() { + *deleted_ = true; + } + + void Run() { + CHECK(false); + } + + private: + bool* deleted_; + }; + + class DeletedOnIO + : public base::RefCountedThreadSafe< + DeletedOnIO, ChromeThread::DeleteOnIOThread> { + public: + DeletedOnIO(MessageLoop* message_loop) : message_loop_(message_loop) { } + + ~DeletedOnIO() { + CHECK(ChromeThread::CurrentlyOn(ChromeThread::IO)); + message_loop_->PostTask(FROM_HERE, new MessageLoop::QuitTask()); + } + + private: + MessageLoop* message_loop_; + }; + + class NeverDeleted + : public base::RefCountedThreadSafe< + NeverDeleted, ChromeThread::DeleteOnWebKitThread> { + public: + ~NeverDeleted() { + CHECK(false); + } + }; + + private: + scoped_ptr<ChromeThread> file_thread_; + scoped_ptr<ChromeThread> io_thread_; + MessageLoop loop_; +}; + +TEST_F(ChromeThreadTest, PostTask) { + ChromeThread::PostTask( + ChromeThread::IO, FROM_HERE, + NewRunnableFunction(&BasicFunction, MessageLoop::current())); + MessageLoop::current()->Run(); +} - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::IO) == NULL); - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::FILE) == NULL); - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::DB) == NULL); +TEST_F(ChromeThreadTest, Release) { + ChromeThread::ReleaseSoon(ChromeThread::FILE, FROM_HERE, this); + MessageLoop::current()->Run(); +} - // Phase 4: Destroy the threads. +TEST_F(ChromeThreadTest, TaskToNonExistentThreadIsDeleted) { + bool deleted = false; + ChromeThread::PostTask( + ChromeThread::WEBKIT, FROM_HERE, + new DummyTask(&deleted)); + EXPECT_TRUE(deleted); +} - io_thread.reset(); - file_thread.reset(); - db_thread.reset(); +TEST_F(ChromeThreadTest, ReleasedOnCorrectThread) { + { + scoped_refptr<DeletedOnIO> test(new DeletedOnIO(MessageLoop::current())); + } + MessageLoop::current()->Run(); +} - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::IO) == NULL); - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::FILE) == NULL); - EXPECT_TRUE(ChromeThread::GetMessageLoop(ChromeThread::DB) == NULL); - */ +TEST_F(ChromeThreadTest, NotReleasedIfTargetThreadNonExistent) { + scoped_refptr<NeverDeleted> test(new NeverDeleted()); } diff --git a/chrome/browser/renderer_host/file_system_accessor.cc b/chrome/browser/renderer_host/file_system_accessor.cc deleted file mode 100644 index de5fc7b..0000000 --- a/chrome/browser/renderer_host/file_system_accessor.cc +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "chrome/browser/renderer_host/file_system_accessor.h" - -#include "base/file_util.h" -#include "base/message_loop.h" -#include "chrome/browser/chrome_thread.h" - -FileSystemAccessor::FileSystemAccessor(void* param, FileSizeCallback* callback) - : param_(param), callback_(callback) { - caller_loop_ = MessageLoop::current(); -} - -FileSystemAccessor::~FileSystemAccessor() { -} - -void FileSystemAccessor::RequestFileSize(const FilePath& path, - void* param, - FileSizeCallback* callback) { - // Getting file size could take long time if it lives on a network share, - // so run it on FILE thread. - ChromeThread::PostTask( - ChromeThread::FILE, FROM_HERE, - NewRunnableMethod(new FileSystemAccessor(param, callback), - &FileSystemAccessor::GetFileSize, path)); -} - -void FileSystemAccessor::GetFileSize(const FilePath& path) { - int64 result; - // Set result to -1 if failed to get file size. - if (!file_util::GetFileSize(path, &result)) - result = -1; - - // Pass the result back to the caller thread. - caller_loop_->PostTask( - FROM_HERE, - NewRunnableMethod(this, &FileSystemAccessor::GetFileSizeCompleted, result)); -} - -void FileSystemAccessor::GetFileSizeCompleted(int64 result) { - callback_->Run(result, param_); -} diff --git a/chrome/browser/renderer_host/file_system_accessor.h b/chrome/browser/renderer_host/file_system_accessor.h deleted file mode 100644 index 8400898..0000000 --- a/chrome/browser/renderer_host/file_system_accessor.h +++ /dev/null @@ -1,74 +0,0 @@ -// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. -// -// FileSystemAccessor provides functions so consumers can do file access -// asynchronously. It would PostTask to FILE thread to access file info, then on -// completion PostTask back to the caller thread and pass the result back. -// Here is an example on how to use it to get file size: -// 1. Define a callback function so FileSystemAccessor could run it after the -// task has been completed: -// void Foo::GetFileSizeCallback(int64 result, void* param) { -// } -// 2. Call static function FileSystemAccessor::RequestFileSize, provide file -// path, param (any object you want to pass back to the callback function) -// and callback: -// FileSystemAccessor::RequestFileSize( -// path, -// param, -// NewCallback(this, &Foo::GetFileSizeCallback)); -// 3. FileSystemAceessor would PostTask to FILE thread to get file size, then -// on completion it would PostTask back to the current thread and run -// Foo::GetFileSizeCallback. -// - -#ifndef CHROME_BROWSER_RENDERER_HOST_FILE_SYSTEM_ACCESSOR_H_ -#define CHROME_BROWSER_RENDERER_HOST_FILE_SYSTEM_ACCESSOR_H_ - -#include "base/scoped_ptr.h" -#include "base/ref_counted.h" -#include "base/task.h" - -class FilePath; -class MessageLoop; - -class FileSystemAccessor - : public base::RefCountedThreadSafe<FileSystemAccessor> { - public: - typedef Callback2<int64, void*>::Type FileSizeCallback; - - virtual ~FileSystemAccessor(); - - // Request to get file size. - // - // param is an object that is owned by the caller and needs to pass back to - // the caller by FileSystemAccessor through the callback function. - // It can be set to NULL if no object needs to pass back. - // - // FileSizeCallback function is defined as: - // void f(int64 result, void* param); - // Variable result has the file size. If the file does not exist or there is - // error accessing the file, result is set to -1. If the given path is a - // directory, result is set to 0. - static void RequestFileSize(const FilePath& path, - void* param, - FileSizeCallback* callback); - - private: - FileSystemAccessor(void* param, FileSizeCallback* callback); - - // Get file size on the worker thread and pass result back to the caller - // thread. - void GetFileSize(const FilePath& path); - - // Getting file size completed, callback to reply message. - void GetFileSizeCompleted(int64 result); - - MessageLoop* caller_loop_; - void* param_; - scoped_ptr<FileSizeCallback> callback_; - - DISALLOW_COPY_AND_ASSIGN(FileSystemAccessor); -}; - -#endif // CHROME_BROWSER_RENDERER_HOST_FILE_SYSTEM_ACCESSOR_H_ diff --git a/chrome/browser/renderer_host/file_system_accessor_unittest.cc b/chrome/browser/renderer_host/file_system_accessor_unittest.cc deleted file mode 100644 index 62f1008..0000000 --- a/chrome/browser/renderer_host/file_system_accessor_unittest.cc +++ /dev/null @@ -1,115 +0,0 @@ -// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "base/file_util.h" -#include "base/message_loop.h" -#include "base/timer.h" -#include "chrome/browser/chrome_thread.h" -#include "chrome/browser/renderer_host/file_system_accessor.h" -#include "chrome/test/file_test_utils.h" -#include "testing/gtest/include/gtest/gtest.h" - -class FileSystemAccessorTest : public testing::Test { - protected: - virtual void SetUp() { - // Make sure the current thread has a message loop. - EXPECT_TRUE(MessageLoop::current() != NULL); - - // Create FILE thread which is used to do file access. - file_thread_.reset(new ChromeThread(ChromeThread::FILE)); - - // Start FILE thread and verify FILE message loop exists. - file_thread_->Start(); - EXPECT_TRUE(file_thread_->message_loop() != NULL); - } - - virtual void TearDown() { - file_thread_->Stop(); - } - - void TestGetFileSize(const FilePath& path, - void* param, - int64 expected_result, - void* expected_param) { - // Initialize the actual result not equal to the expected result. - result_ = expected_result + 1; - param_ = NULL; - - FileSystemAccessor::RequestFileSize( - path, - param, - NewCallback(this, &FileSystemAccessorTest::GetFileSizeCallback)); - - // Time out if getting file size takes more than 10 seconds. - const int kGetFileSizeTimeoutSeconds = 10; - base::OneShotTimer<MessageLoop> timer; - timer.Start(base::TimeDelta::FromSeconds(kGetFileSizeTimeoutSeconds), - MessageLoop::current(), &MessageLoop::Quit); - - MessageLoop::current()->Run(); - - EXPECT_EQ(expected_result, result_); - EXPECT_EQ(expected_param, param_); - } - - private: - void GetFileSizeCallback(int64 result, void* param) { - result_ = result; - param_ = param; - MessageLoop::current()->Quit(); - } - - scoped_ptr<ChromeThread> file_thread_; - FilePath temp_file_; - int64 result_; - void* param_; - MessageLoop loop_; -}; - -TEST_F(FileSystemAccessorTest, GetFileSize) { - const std::string data("This is test data."); - - FilePath path; - FILE* file = file_util::CreateAndOpenTemporaryFile(&path); - EXPECT_TRUE(file != NULL); - size_t bytes_written = fwrite(data.data(), 1, data.length(), file); - EXPECT_TRUE(file_util::CloseFile(file)); - - FileAutoDeleter deleter(path); - - TestGetFileSize(path, NULL, bytes_written, NULL); -} - -TEST_F(FileSystemAccessorTest, GetFileSizeWithParam) { - const std::string data("This is test data."); - - FilePath path; - FILE* file = file_util::CreateAndOpenTemporaryFile(&path); - EXPECT_TRUE(file != NULL); - size_t bytes_written = fwrite(data.data(), 1, data.length(), file); - EXPECT_TRUE(file_util::CloseFile(file)); - - FileAutoDeleter deleter(path); - - int param = 100; - TestGetFileSize(path, static_cast<void*>(¶m), - bytes_written, static_cast<void*>(¶m)); -} - -TEST_F(FileSystemAccessorTest, GetFileSizeEmptyFile) { - FilePath path; - EXPECT_TRUE(file_util::CreateTemporaryFile(&path)); - FileAutoDeleter deleter(path); - - TestGetFileSize(path, NULL, 0, NULL); -} - -TEST_F(FileSystemAccessorTest, GetFileSizeNotFound) { - FilePath path; - EXPECT_TRUE(file_util::CreateNewTempDirectory( - FILE_PATH_LITERAL("chrome_test_"), &path)); - FileAutoDeleter deleter(path); - - TestGetFileSize(path.Append(FILE_PATH_LITERAL("foo.txt")), NULL, -1, NULL); -} diff --git a/chrome/browser/renderer_host/resource_message_filter.cc b/chrome/browser/renderer_host/resource_message_filter.cc index c4ccda3..5b4aa81 100644 --- a/chrome/browser/renderer_host/resource_message_filter.cc +++ b/chrome/browser/renderer_host/resource_message_filter.cc @@ -7,6 +7,7 @@ #include "app/clipboard/clipboard.h" #include "app/gfx/native_widget_types.h" #include "base/command_line.h" +#include "base/file_util.h" #include "base/histogram.h" #include "base/process_util.h" #include "base/thread.h" @@ -28,7 +29,6 @@ #include "chrome/browser/renderer_host/audio_renderer_host.h" #include "chrome/browser/renderer_host/browser_render_process_host.h" #include "chrome/browser/renderer_host/database_dispatcher_host.h" -#include "chrome/browser/renderer_host/file_system_accessor.h" #include "chrome/browser/renderer_host/render_widget_helper.h" #include "chrome/browser/renderer_host/socket_stream_dispatcher_host.h" #include "chrome/browser/spellchecker.h" @@ -406,6 +406,10 @@ bool ResourceMessageFilter::OnMessageReceived(const IPC::Message& msg) { return handled; } +void ResourceMessageFilter::OnDestruct() { + ChromeThread::DeleteOnIOThread::Destruct(this); +} + void ResourceMessageFilter::OnReceiveContextMenuMsg(const IPC::Message& msg) { void* iter = NULL; ContextMenuParams params; @@ -594,35 +598,22 @@ void ResourceMessageFilter::OnLoadFont(LOGFONT font) { void ResourceMessageFilter::OnGetPlugins(bool refresh, IPC::Message* reply_msg) { - // Schedule plugin loading on the file thread. - // Note: it's possible that the only reference to this object is the task. If - // If the task executes on the file thread, and before it returns, the task it - // posts to the IO thread runs, then this object will get destructed on the - // file thread. We need this object to be destructed on the IO thread, so do - // the refcounting manually. - AddRef(); ChromeThread::PostTask( ChromeThread::FILE, FROM_HERE, - NewRunnableFunction(&ResourceMessageFilter::OnGetPluginsOnFileThread, - this, refresh, reply_msg)); + NewRunnableMethod( + this, &ResourceMessageFilter::OnGetPluginsOnFileThread, refresh, + reply_msg)); } void ResourceMessageFilter::OnGetPluginsOnFileThread( - ResourceMessageFilter* filter, - bool refresh, - IPC::Message* reply_msg) { + bool refresh, IPC::Message* reply_msg) { + DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE)); std::vector<WebPluginInfo> plugins; NPAPI::PluginList::Singleton()->GetPlugins(refresh, &plugins); ViewHostMsg_GetPlugins::WriteReplyParams(reply_msg, plugins); ChromeThread::PostTask( ChromeThread::IO, FROM_HERE, - NewRunnableMethod(filter, &ResourceMessageFilter::OnPluginsLoaded, - reply_msg)); -} - -void ResourceMessageFilter::OnPluginsLoaded(IPC::Message* reply_msg) { - Send(reply_msg); - Release(); + NewRunnableMethod(this, &ResourceMessageFilter::Send, reply_msg)); } void ResourceMessageFilter::OnGetPluginPath(const GURL& url, @@ -1130,29 +1121,36 @@ void ResourceMessageFilter::OnSetCacheMode(bool enabled) { void ResourceMessageFilter::OnGetFileSize(const FilePath& path, IPC::Message* reply_msg) { - // Increase the ref count to ensure ResourceMessageFilter won't be destroyed - // before GetFileSize callback is done. - AddRef(); - // Get file size only when the child process has been granted permission to // upload the file. - if (ChildProcessSecurityPolicy::GetInstance()->CanUploadFile(id(), path)) { - FileSystemAccessor::RequestFileSize( - path, - reply_msg, - NewCallback(this, &ResourceMessageFilter::ReplyGetFileSize)); - } else { - ReplyGetFileSize(-1, reply_msg); + if (!ChildProcessSecurityPolicy::GetInstance()->CanUploadFile(id(), path)) { + ViewHostMsg_GetFileSize::WriteReplyParams( + reply_msg, static_cast<int64>(-1)); + Send(reply_msg); + return; } + + // Getting file size could take long time if it lives on a network share, + // so run it on FILE thread. + ChromeThread::PostTask( + ChromeThread::FILE, FROM_HERE, + NewRunnableMethod( + this, &ResourceMessageFilter::OnGetFileSizeOnFileThread, path, + reply_msg)); } -void ResourceMessageFilter::ReplyGetFileSize(int64 result, void* param) { - IPC::Message* reply_msg = static_cast<IPC::Message*>(param); +void ResourceMessageFilter::OnGetFileSizeOnFileThread( + const FilePath& path, IPC::Message* reply_msg) { + DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE)); + + int64 result; + if (!file_util::GetFileSize(path, &result)) + result = -1; ViewHostMsg_GetFileSize::WriteReplyParams(reply_msg, result); - Send(reply_msg); - // Getting file size callback done, decrease the ref count. - Release(); + ChromeThread::PostTask( + ChromeThread::IO, FROM_HERE, + NewRunnableMethod(this, &ResourceMessageFilter::Send, reply_msg)); } void ResourceMessageFilter::OnKeygen(uint32 key_size_index, diff --git a/chrome/browser/renderer_host/resource_message_filter.h b/chrome/browser/renderer_host/resource_message_filter.h index 1c3d520..0184591 100644 --- a/chrome/browser/renderer_host/resource_message_filter.h +++ b/chrome/browser/renderer_host/resource_message_filter.h @@ -97,6 +97,7 @@ class ResourceMessageFilter : public IPC::ChannelProxy::MessageFilter, virtual void OnChannelError(); virtual void OnChannelClosing(); virtual bool OnMessageReceived(const IPC::Message& message); + virtual void OnDestruct(); // ResourceDispatcherHost::Receiver methods: virtual bool Send(IPC::Message* message); @@ -149,10 +150,7 @@ class ResourceMessageFilter : public IPC::ChannelProxy::MessageFilter, void OnGetScreenInfo(gfx::NativeViewId window, IPC::Message* reply); #endif void OnGetPlugins(bool refresh, IPC::Message* reply_msg); - static void OnGetPluginsOnFileThread(ResourceMessageFilter* filter, - bool refresh, - IPC::Message* reply_msg); - void OnPluginsLoaded(IPC::Message* reply_msg); + void OnGetPluginsOnFileThread(bool refresh, IPC::Message* reply_msg); void OnGetPluginPath(const GURL& url, const GURL& policy_url, const std::string& mime_type, @@ -283,7 +281,7 @@ class ResourceMessageFilter : public IPC::ChannelProxy::MessageFilter, void OnSetCacheMode(bool enabled); void OnGetFileSize(const FilePath& path, IPC::Message* reply_msg); - void ReplyGetFileSize(int64 result, void* param); + void OnGetFileSizeOnFileThread(const FilePath& path, IPC::Message* reply_msg); void OnKeygen(uint32 key_size_index, const std::string& challenge_string, const GURL& url, std::string* signed_public_key); #if defined(OS_LINUX) diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp index fe770c7..12eb600 100755 --- a/chrome/chrome.gyp +++ b/chrome/chrome.gyp @@ -1946,8 +1946,6 @@ 'browser/renderer_host/download_resource_handler.h', 'browser/renderer_host/download_throttling_resource_handler.cc', 'browser/renderer_host/download_throttling_resource_handler.h', - 'browser/renderer_host/file_system_accessor.cc', - 'browser/renderer_host/file_system_accessor.h', 'browser/renderer_host/gtk_im_context_wrapper.cc', 'browser/renderer_host/gtk_im_context_wrapper.h', 'browser/renderer_host/gtk_key_bindings_handler.cc', @@ -4611,7 +4609,6 @@ 'browser/privacy_blacklist/blacklist_io_unittest.cc', 'browser/profile_manager_unittest.cc', 'browser/renderer_host/audio_renderer_host_unittest.cc', - 'browser/renderer_host/file_system_accessor_unittest.cc', 'browser/renderer_host/render_widget_host_unittest.cc', 'browser/renderer_host/resource_dispatcher_host_unittest.cc', 'browser/renderer_host/test/render_view_host_unittest.cc', diff --git a/ipc/ipc_channel_proxy.cc b/ipc/ipc_channel_proxy.cc index cbe8cc2..4d13b2b8 100644 --- a/ipc/ipc_channel_proxy.cc +++ b/ipc/ipc_channel_proxy.cc @@ -10,6 +10,12 @@ namespace IPC { +// static +void ChannelProxy::MessageFilterTraits::Destruct( + ChannelProxy::MessageFilter* filter) { + filter->OnDestruct(); +} + //------------------------------------------------------------------------------ // This task ensures the message is deleted if the task is deleted without diff --git a/ipc/ipc_channel_proxy.h b/ipc/ipc_channel_proxy.h index a2d55be..3369f50 100644 --- a/ipc/ipc_channel_proxy.h +++ b/ipc/ipc_channel_proxy.h @@ -46,9 +46,16 @@ class SendTask; // class ChannelProxy : public Message::Sender { public: + + class MessageFilter; + struct MessageFilterTraits { + static void Destruct(MessageFilter* filter); + }; + // A class that receives messages on the thread where the IPC channel is // running. It can choose to prevent the default action for an IPC message. - class MessageFilter : public base::RefCountedThreadSafe<MessageFilter> { + class MessageFilter + : public base::RefCountedThreadSafe<MessageFilter, MessageFilterTraits> { public: virtual ~MessageFilter() {} @@ -79,6 +86,13 @@ class ChannelProxy : public Message::Sender { virtual bool OnMessageReceived(const Message& message) { return false; } + + // Called when the message filter is about to be deleted. This gives + // derived classes the option of controlling which thread they're deleted + // on etc. + virtual void OnDestruct() { + delete this; + } }; // Initializes a channel proxy. The channel_id and mode parameters are |