diff options
author | paulg@google.com <paulg@google.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-04-14 02:03:20 +0000 |
---|---|---|
committer | paulg@google.com <paulg@google.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-04-14 02:03:20 +0000 |
commit | 05349986a0c968101b8925be496794b1f2bc40f5 (patch) | |
tree | 7cb79c84d92e7e5910664e94979debb9980b3e3d /chrome | |
parent | 3401b00e41731a8816d9aec9592b09efb4bf5eac (diff) | |
download | chromium_src-05349986a0c968101b8925be496794b1f2bc40f5.zip chromium_src-05349986a0c968101b8925be496794b1f2bc40f5.tar.gz chromium_src-05349986a0c968101b8925be496794b1f2bc40f5.tar.bz2 |
Fix a crash where the ResourceMessageFilter is deleted before a
SafeBrowsing check has completed. The problem occurs since the
SafeBrowsingResourceHandler is not deleted when its associated
URLRequest is cleaned up *and* a SafeBrowsing check is in progress.
When the check completes, the next resource handler in the chain
(the AsyncResourceHandler which caches a pointer the now deleted
ResourceMessageFilter) will crash.
This CL adds a notification for objects to know when the
ResourceMessageFilter is destroyed.
BUG=8544 (http://crbug.com)
Review URL: http://codereview.chromium.org/63036
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@13644 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome')
9 files changed, 113 insertions, 35 deletions
diff --git a/chrome/browser/extensions/extension_message_service.cc b/chrome/browser/extensions/extension_message_service.cc index 8a4ef73..f200c5b 100755 --- a/chrome/browser/extensions/extension_message_service.cc +++ b/chrome/browser/extensions/extension_message_service.cc @@ -11,6 +11,7 @@ #include "chrome/browser/renderer_host/render_view_host.h" #include "chrome/browser/renderer_host/render_process_host.h" #include "chrome/browser/renderer_host/resource_message_filter.h" +#include "chrome/common/notification_service.h" #include "chrome/common/render_messages.h" #include "chrome/common/stl_util-inl.h" @@ -132,14 +133,23 @@ void ExtensionMessageService::RendererReady(ResourceMessageFilter* renderer) { AutoLock lock(renderers_lock_); DCHECK(renderers_.find(renderer->GetProcessId()) == renderers_.end()); renderers_[renderer->GetProcessId()] = renderer; + + NotificationService::current()->AddObserver( + this, + NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN, + Source<ResourceMessageFilter>(renderer)); } -void ExtensionMessageService::RendererShutdown( - ResourceMessageFilter* renderer) { +void ExtensionMessageService::Observe(NotificationType type, + const NotificationSource& source, + const NotificationDetails& details) { + DCHECK(type.value == NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN); + ResourceMessageFilter* filter = Source<ResourceMessageFilter>(source).ptr(); + { AutoLock lock(renderers_lock_); - DCHECK(renderers_.find(renderer->GetProcessId()) != renderers_.end()); - renderers_.erase(renderer->GetProcessId()); + DCHECK(renderers_.find(filter->GetProcessId()) != renderers_.end()); + renderers_.erase(filter->GetProcessId()); } // Close any channels that share this filter. @@ -147,7 +157,13 @@ void ExtensionMessageService::RendererShutdown( for (MessageChannelMap::iterator it = channels_.begin(); it != channels_.end(); ) { MessageChannelMap::iterator current = it++; - if (current->second.port1 == renderer || current->second.port2 == renderer) + if (current->second.port1 == filter || current->second.port2 == filter) channels_.erase(current); } + + NotificationService::current()->RemoveObserver( + this, + NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN, + Source<ResourceMessageFilter>(filter)); } + diff --git a/chrome/browser/extensions/extension_message_service.h b/chrome/browser/extensions/extension_message_service.h index a63837a..1fc3c51 100755 --- a/chrome/browser/extensions/extension_message_service.h +++ b/chrome/browser/extensions/extension_message_service.h @@ -9,6 +9,7 @@ #include <string> #include "base/lock.h" +#include "chrome/common/notification_observer.h" class ExtensionView; class ResourceMessageFilter; @@ -23,7 +24,7 @@ class URLRequestContext; // port: an IPC::Message::Sender interface through which we communicate to a // process. We use MessageFilters for this since that allows us to send our // messages on the IO thread. -class ExtensionMessageService { +class ExtensionMessageService : public NotificationObserver { public: // Returns the message service for the given context. Messages can only // be sent within a single context. @@ -54,8 +55,10 @@ class ExtensionMessageService { // Called to let us know that a renderer has been started. void RendererReady(ResourceMessageFilter* port); - // Called to let us know that a renderer is going away. - void RendererShutdown(ResourceMessageFilter* port); + // NotificationObserver interface. + void Observe(NotificationType type, + const NotificationSource& source, + const NotificationDetails& details); private: // A map of extension ID to the render_process_id that the extension lives in. diff --git a/chrome/browser/renderer_host/resource_dispatcher_host.cc b/chrome/browser/renderer_host/resource_dispatcher_host.cc index 3169786..f73d507 100644 --- a/chrome/browser/renderer_host/resource_dispatcher_host.cc +++ b/chrome/browser/renderer_host/resource_dispatcher_host.cc @@ -382,7 +382,8 @@ void ResourceDispatcherHost::BeginRequest( request_data.url, request_data.resource_type, safe_browsing_, - this); + this, + receiver_); } // Insert a buffered event handler before the actual one. @@ -513,7 +514,8 @@ void ResourceDispatcherHost::BeginDownload(const GURL& url, url, ResourceType::MAIN_FRAME, safe_browsing_, - this); + this, + receiver_); } bool known_proto = URLRequest::IsHandledURL(url); diff --git a/chrome/browser/renderer_host/resource_message_filter.cc b/chrome/browser/renderer_host/resource_message_filter.cc index 1230f56..06433a1 100644 --- a/chrome/browser/renderer_host/resource_message_filter.cc +++ b/chrome/browser/renderer_host/resource_message_filter.cc @@ -139,9 +139,11 @@ ResourceMessageFilter::ResourceMessageFilter( } ResourceMessageFilter::~ResourceMessageFilter() { - WorkerService::GetInstance()->RendererShutdown(this); - ExtensionMessageService::GetInstance(request_context_.get())-> - RendererShutdown(this); + // Let interested observers know we are being deleted. + NotificationService::current()->Notify( + NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN, + Source<ResourceMessageFilter>(this), + NotificationService::NoDetails()); if (handle()) base::CloseProcessHandle(handle()); diff --git a/chrome/browser/renderer_host/safe_browsing_resource_handler.cc b/chrome/browser/renderer_host/safe_browsing_resource_handler.cc index 22c6873..02b946b 100644 --- a/chrome/browser/renderer_host/safe_browsing_resource_handler.cc +++ b/chrome/browser/renderer_host/safe_browsing_resource_handler.cc @@ -5,6 +5,8 @@ #include "chrome/browser/renderer_host/safe_browsing_resource_handler.h" #include "chrome/browser/renderer_host/resource_dispatcher_host.h" +#include "chrome/browser/renderer_host/resource_message_filter.h" +#include "chrome/common/notification_service.h" // Maximum time to wait for a gethash response from the Safe Browsing servers. static const int kMaxGetHashMs = 1000; @@ -16,7 +18,8 @@ SafeBrowsingResourceHandler::SafeBrowsingResourceHandler( const GURL& url, ResourceType::Type resource_type, SafeBrowsingService* safe_browsing, - ResourceDispatcherHost* resource_dispatcher_host) + ResourceDispatcherHost* resource_dispatcher_host, + ResourceDispatcherHost::Receiver* receiver) : next_handler_(handler), render_process_host_id_(render_process_host_id), render_view_id_(render_view_id), @@ -26,7 +29,8 @@ SafeBrowsingResourceHandler::SafeBrowsingResourceHandler( safe_browsing_(safe_browsing), queued_error_request_id_(-1), rdh_(resource_dispatcher_host), - resource_type_(resource_type) { + resource_type_(resource_type), + receiver_(receiver) { if (safe_browsing_->CheckUrl(url, this)) { safe_browsing_result_ = SafeBrowsingService::URL_SAFE; safe_browsing_->LogPauseDelay(base::TimeDelta()); // No delay. @@ -35,13 +39,20 @@ SafeBrowsingResourceHandler::SafeBrowsingResourceHandler( in_safe_browsing_check_ = true; // Can't pause now because it's too early, so we'll do it in OnWillRead. } + + NotificationService::current()->AddObserver( + this, + NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN, + Source<ResourceMessageFilter>( + static_cast<ResourceMessageFilter*>(receiver_))); } SafeBrowsingResourceHandler::~SafeBrowsingResourceHandler() { - // If we're being deleted before the SafeBrowsing check has completed, cancel - // the check. - if (in_safe_browsing_check_) - safe_browsing_->CancelCheck(this); + NotificationService::current()->RemoveObserver( + this, + NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN, + Source<ResourceMessageFilter>( + static_cast<ResourceMessageFilter*>(receiver_))); } bool SafeBrowsingResourceHandler::OnUploadProgress(int request_id, @@ -190,3 +201,15 @@ void SafeBrowsingResourceHandler::OnBlockingPageComplete(bool proceed) { Release(); } + +void SafeBrowsingResourceHandler::Observe(NotificationType type, + const NotificationSource& source, + const NotificationDetails& details) { + DCHECK(type.value == NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN); + if (in_safe_browsing_check_) { + safe_browsing_->CancelCheck(this); + in_safe_browsing_check_ = false; + Release(); + } +} + diff --git a/chrome/browser/renderer_host/safe_browsing_resource_handler.h b/chrome/browser/renderer_host/safe_browsing_resource_handler.h index 1585e35..19339a7 100644 --- a/chrome/browser/renderer_host/safe_browsing_resource_handler.h +++ b/chrome/browser/renderer_host/safe_browsing_resource_handler.h @@ -8,14 +8,15 @@ #include <string> #include "base/time.h" +#include "chrome/browser/renderer_host/resource_dispatcher_host.h" #include "chrome/browser/renderer_host/resource_handler.h" #include "chrome/browser/safe_browsing/safe_browsing_service.h" - -class ResourceDispatcherHost; +#include "chrome/common/notification_observer.h" // Checks that a url is safe. class SafeBrowsingResourceHandler : public ResourceHandler, - public SafeBrowsingService::Client { + public SafeBrowsingService::Client, + public NotificationObserver { public: SafeBrowsingResourceHandler(ResourceHandler* handler, int render_process_host_id, @@ -23,7 +24,8 @@ class SafeBrowsingResourceHandler : public ResourceHandler, const GURL& url, ResourceType::Type resource_type, SafeBrowsingService* safe_browsing, - ResourceDispatcherHost* resource_dispatcher_host); + ResourceDispatcherHost* resource_dispatcher_host, + ResourceDispatcherHost::Receiver* receiver); ~SafeBrowsingResourceHandler(); // ResourceHandler implementation: @@ -47,6 +49,11 @@ class SafeBrowsingResourceHandler : public ResourceHandler, // the user has decided to proceed with the current request, or go back. void OnBlockingPageComplete(bool proceed); + // NotificationObserver interface. + void Observe(NotificationType type, + const NotificationSource& source, + const NotificationDetails& details); + private: scoped_refptr<ResourceHandler> next_handler_; int render_process_host_id_; @@ -62,6 +69,7 @@ class SafeBrowsingResourceHandler : public ResourceHandler, ResourceDispatcherHost* rdh_; base::Time pause_time_; ResourceType::Type resource_type_; + ResourceDispatcherHost::Receiver* receiver_; DISALLOW_COPY_AND_ASSIGN(SafeBrowsingResourceHandler); }; diff --git a/chrome/browser/worker_host/worker_service.cc b/chrome/browser/worker_host/worker_service.cc index 8151d9b..04d9d8d 100644 --- a/chrome/browser/worker_host/worker_service.cc +++ b/chrome/browser/worker_host/worker_service.cc @@ -14,6 +14,7 @@ #include "chrome/browser/renderer_host/render_process_host.h" #include "chrome/browser/renderer_host/resource_message_filter.h" #include "chrome/common/chrome_switches.h" +#include "chrome/common/notification_service.h" #include "net/base/registry_controlled_domain.h" namespace { @@ -57,6 +58,13 @@ bool WorkerService::CreateDedicatedWorker(const GURL &url, // it to. worker->CreateWorker(url, render_view_route_id, ++next_worker_route_id_, renderer_route_id, filter); + + // Receive a notification if the message filter is deleted. + NotificationService::current()->AddObserver( + this, + NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN, + Source<ResourceMessageFilter>(filter)); + return true; } @@ -71,14 +79,6 @@ void WorkerService::ForwardMessage(const IPC::Message& message) { // TODO(jabdelmalek): tell sender that callee is gone } -void WorkerService::RendererShutdown(ResourceMessageFilter* filter) { - for (ChildProcessHost::Iterator iter(ChildProcessInfo::WORKER_PROCESS); - !iter.Done(); ++iter) { - WorkerProcessHost* worker = static_cast<WorkerProcessHost*>(*iter); - worker->RendererShutdown(filter); - } -} - WorkerProcessHost* WorkerService::GetProcessForDomain(const GURL& url) { int num_processes = 0; std::string domain = @@ -126,3 +126,21 @@ WorkerProcessHost* WorkerService::GetLeastLoadedWorker() { return smallest; } + +void WorkerService::Observe(NotificationType type, + const NotificationSource& source, + const NotificationDetails& details) { + DCHECK(type.value == NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN); + ResourceMessageFilter* filter = Source<ResourceMessageFilter>(source).ptr(); + + for (ChildProcessHost::Iterator iter(ChildProcessInfo::WORKER_PROCESS); + !iter.Done(); ++iter) { + WorkerProcessHost* worker = static_cast<WorkerProcessHost*>(*iter); + worker->RendererShutdown(filter); + } + + NotificationService::current()->RemoveObserver( + this, + NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN, + Source<ResourceMessageFilter>(filter)); +} diff --git a/chrome/browser/worker_host/worker_service.h b/chrome/browser/worker_host/worker_service.h index 1aea2aa..cf18d58 100644 --- a/chrome/browser/worker_host/worker_service.h +++ b/chrome/browser/worker_host/worker_service.h @@ -9,6 +9,7 @@ #include "base/basictypes.h" #include "base/singleton.h" +#include "chrome/common/notification_observer.h" #include "googleurl/src/gurl.h" namespace IPC { @@ -19,7 +20,7 @@ class MessageLoop; class WorkerProcessHost; class ResourceMessageFilter; -class WorkerService { +class WorkerService : public NotificationObserver { public: // Returns the WorkerService singleton. static WorkerService* GetInstance(); @@ -34,9 +35,10 @@ class WorkerService { // should be forwarded to the worker process. void ForwardMessage(const IPC::Message& message); - // Called by ResourceMessageFilter when it's destructed so that all the - // WorkerProcessHost objects can remove their pointers to it. - void RendererShutdown(ResourceMessageFilter* filter); + // NotificationObserver interface. + void Observe(NotificationType type, + const NotificationSource& source, + const NotificationDetails& details); private: friend struct DefaultSingletonTraits<WorkerService>; diff --git a/chrome/common/notification_type.h b/chrome/common/notification_type.h index e2b9da2..cae614d 100644 --- a/chrome/common/notification_type.h +++ b/chrome/common/notification_type.h @@ -157,6 +157,10 @@ class NotificationType { // both normal completion or via a cancel operation. DOWNLOAD_START, DOWNLOAD_STOP, + // Lets resource handlers and other interested observers know when the + // message filter is being deleted and can no longer be used. + RESOURCE_MESSAGE_FILTER_SHUTDOWN, + // Views ------------------------------------------------------------------- // Notification that a view was removed from a view hierarchy. The source |