From c7959e210688e9d59fae6cea22d35498226dc90b Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Fri, 7 Jan 2011 01:06:10 +0000 Subject: [PATCH] Fix a crash when calling CefShutdown() before destroying all browser windows (issue #159). git-svn-id: https://chromiumembedded.googlecode.com/svn/trunk@157 5089003a-bbd8-11dd-ad1f-f1f9622dbc98 --- libcef/browser_impl.cc | 16 ++++++- libcef/cef_context.cc | 97 ++++++++++++++++++++++++++++++++++-------- libcef/cef_context.h | 22 +++++++++- libcef/scheme_impl.cc | 6 ++- libcef/v8_impl.cc | 12 ++++-- 5 files changed, 126 insertions(+), 27 deletions(-) diff --git a/libcef/browser_impl.cc b/libcef/browser_impl.cc index ababa6fe0..cccca35b0 100644 --- a/libcef/browser_impl.cc +++ b/libcef/browser_impl.cc @@ -483,8 +483,11 @@ bool CefBrowser::CreateBrowser(CefWindowInfo& windowInfo, bool popup, CefRefPtr handler, const CefString& url) { - if(!_Context.get()) + // Verify that the context is in a valid state. + if (!CONTEXT_STATE_VALID()) { + NOTREACHED(); return false; + } CefString newUrl = url; CefBrowserSettings settings(_Context->browser_defaults()); @@ -510,8 +513,17 @@ bool CefBrowser::CreateBrowser(CefWindowInfo& windowInfo, bool popup, CefRefPtr CefBrowser::CreateBrowserSync(CefWindowInfo& windowInfo, bool popup, CefRefPtr handler, const CefString& url) { - if(!_Context.get() || !CefThread::CurrentlyOn(CefThread::UI)) + // Verify that the context is in a valid state. + if (!CONTEXT_STATE_VALID()) { + NOTREACHED(); return NULL; + } + + // Verify that this method is being called on the UI thread. + if (!CefThread::CurrentlyOn(CefThread::UI)) { + NOTREACHED(); + return NULL; + } CefString newUrl = url; CefRefPtr alternateBrowser; diff --git a/libcef/cef_context.cc b/libcef/cef_context.cc index bf2ddf882..3a3ad7622 100644 --- a/libcef/cef_context.cc +++ b/libcef/cef_context.cc @@ -21,7 +21,7 @@ CefRefPtr _Context; bool CefInitialize(const CefSettings& settings, const CefBrowserSettings& browser_defaults) { - // Return true if the context is already initialized + // Return true if the global context already exists. if(_Context.get()) return true; @@ -31,33 +31,48 @@ bool CefInitialize(const CefSettings& settings, return false; } - // Create the new global context object + // Create the new global context object. _Context = new CefContext(); - // Initialize the global context + // Initialize the global context. return _Context->Initialize(settings, browser_defaults); } void CefShutdown() { - // Verify that the context is already initialized - if(!_Context.get()) + // Verify that the context is in a valid state. + if (!CONTEXT_STATE_VALID()) { + NOTREACHED(); return; + } - // Shut down the global context + // Must always be called on the same thread as Initialize. + if(!_Context->process()->CalledOnValidThread()) { + NOTREACHED(); + return; + } + + // Shut down the global context. This will block until shutdown is complete. _Context->Shutdown(); - // Delete the global context object + + // Delete the global context object. _Context = NULL; } void CefDoMessageLoopWork() { - // Verify that the context is already initialized. - if(!_Context.get() || !_Context->process()) + // Verify that the context is in a valid state. + if (!CONTEXT_STATE_VALID()) { + NOTREACHED(); return; + } - if(!_Context->process()->CalledOnValidThread()) + // Must always be called on the same thread as Initialize. + if(!_Context->process()->CalledOnValidThread()) { + NOTREACHED(); return; + } + _Context->process()->DoMessageLoopIteration(); } @@ -104,8 +119,11 @@ static void UIT_RegisterPlugin(struct CefPluginInfo* plugin_info) bool CefRegisterPlugin(const struct CefPluginInfo& plugin_info) { - if(!_Context.get()) + // Verify that the context is in a valid state. + if (!CONTEXT_STATE_VALID()) { + NOTREACHED(); return false; + } CefPluginInfo* pPluginInfo = new CefPluginInfo; *pPluginInfo = plugin_info; @@ -173,15 +191,15 @@ bool CefPostDelayedTask(CefThreadId threadId, CefRefPtr task, // CefContext -CefContext::CefContext() : process_(NULL) +CefContext::CefContext() : initialized_(false), shutting_down_(false) { } CefContext::~CefContext() { - // Just in case CefShutdown() isn't called - Shutdown(); + if(!shutting_down_) + Shutdown(); } bool CefContext::Initialize(const CefSettings& settings, @@ -200,19 +218,38 @@ bool CefContext::Initialize(const CefSettings& settings, process_ = new CefProcess(settings_.multi_threaded_message_loop); process_->CreateChildThreads(); + initialized_ = true; + return true; } void CefContext::Shutdown() { - // Remove all existing browsers. - //RemoveAllBrowsers(); + // Must always be called on the same thread as Initialize. + DCHECK(process_->CalledOnValidThread()); + + shutting_down_ = true; - // Deleting the process will destroy the child threads. + if(settings_.multi_threaded_message_loop) { + // Event that will be used to signal when shutdown is complete. Start in + // non-signaled mode so that the event will block. + base::WaitableEvent event(false, false); + + // Finish shutdown on the UI thread. + CefThread::PostTask(CefThread::UI, FROM_HERE, + NewRunnableMethod(this, &CefContext::UIT_FinishShutdown, &event)); + + // Block until shutdown is complete. + event.Wait(); + } else { + // Finish shutdown on the current thread, which should be the UI thread. + UIT_FinishShutdown(NULL); + } + + // Delete the process to destroy the child threads. process_ = NULL; } - bool CefContext::AddBrowser(CefRefPtr browser) { bool found = false; @@ -286,3 +323,27 @@ CefRefPtr CefContext::GetBrowserByID(int id) return browser; } + +void CefContext::UIT_FinishShutdown(base::WaitableEvent* event) +{ + DCHECK(CefThread::CurrentlyOn(CefThread::UI)); + + BrowserList list; + + Lock(); + if (!browserlist_.empty()) { + list = browserlist_; + browserlist_.clear(); + } + Unlock(); + + // Destroy any remaining browser windows. + if (!list.empty()) { + BrowserList::iterator it = list.begin(); + for(; it != list.end(); ++it) + (*it)->UIT_DestroyBrowser(); + } + + if(event) + event->Signal(); +} diff --git a/libcef/cef_context.h b/libcef/cef_context.h index b7287408f..978ebd4a9 100644 --- a/libcef/cef_context.h +++ b/libcef/cef_context.h @@ -33,6 +33,12 @@ public: const CefBrowserSettings& browser_defaults); void Shutdown(); + // Returns true if the context is initialized. + bool initialized() { return initialized_; } + + // Returns true if the context is shutting down. + bool shutting_down() { return shutting_down_; } + scoped_refptr process() { return process_; } bool AddBrowser(CefRefPtr browser); @@ -59,7 +65,17 @@ public: { storage_context_.reset(storage_context); } DOMStorageContext* storage_context() { return storage_context_.get(); } + static bool ImplementsThreadSafeReferenceCounting() { return true; } + private: + // Performs shutdown actions that need to occur on the UI thread before any + // threads are destroyed. + void UIT_FinishShutdown(base::WaitableEvent* event); + + // Track context state. + bool initialized_; + bool shutting_down_; + // Manages the various process threads. scoped_refptr process_; @@ -80,7 +96,11 @@ private: int next_browser_id_; }; -// Global context object pointer +// Global context object pointer. extern CefRefPtr _Context; +// Helper macro that returns true if the global context is in a valid state. +#define CONTEXT_STATE_VALID() \ + (_Context.get() && _Context->initialized() && !_Context->shutting_down()) + #endif // _CEF_CONTEXT_H diff --git a/libcef/scheme_impl.cc b/libcef/scheme_impl.cc index 9cb27085f..d683e4524 100644 --- a/libcef/scheme_impl.cc +++ b/libcef/scheme_impl.cc @@ -393,9 +393,11 @@ bool CefRegisterScheme(const CefString& scheme_name, const CefString& host_name, CefRefPtr factory) { - // Verify that the context is already initialized - if(!_Context.get()) + // Verify that the context is in a valid state. + if (!CONTEXT_STATE_VALID()) { + NOTREACHED(); return false; + } // Use a smart pointer for the wrapper object because // RunnableMethodTraits::RetainCallee() (originating from NewRunnableMethod) diff --git a/libcef/v8_impl.cc b/libcef/v8_impl.cc index 02e9a27e7..9a687acf8 100644 --- a/libcef/v8_impl.cc +++ b/libcef/v8_impl.cc @@ -143,12 +143,16 @@ bool CefRegisterExtension(const CefString& extension_name, const CefString& javascript_code, CefRefPtr handler) { - // Verify that the context is already initialized - if(!_Context.get()) + // Verify that the context is in a valid state. + if (!CONTEXT_STATE_VALID()) { + NOTREACHED(); return false; - - if(!handler.get()) + } + + if(!handler.get()) { + NOTREACHED(); return false; + } TrackString* name = new TrackString(extension_name); TrackAdd(name);