From e57acace0b234703b7161c4dca15585a49be3493 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Tue, 23 Jul 2019 15:10:45 -0400 Subject: [PATCH] Fix crash on shutdown with PDF viewer and multi-threaded message loop (fixes issue #2709, see issue #2622) Requests from the PDF viewer are not associated with a CefBrowser. Consequently, the InterceptedRequestHandler for those requests will register as an observer of CefContext destruction. When the browser is closed the InterceptedRequestHandler is destroyed and an async task is posted to remove/delete the observer on the UI thread. If CefShutdown is then called the task may execute after shutdown has started, in which case CONTEXT_STATE_VALID() will return false. We still need to remove the observer in this case to avoid a use-after-free in FinishShutdownOnUIThread. --- .../resource_request_handler_wrapper.cc | 87 ++++++++++++------- 1 file changed, 56 insertions(+), 31 deletions(-) diff --git a/libcef/browser/net_service/resource_request_handler_wrapper.cc b/libcef/browser/net_service/resource_request_handler_wrapper.cc index 9a5dc7ded..2f72b08a6 100644 --- a/libcef/browser/net_service/resource_request_handler_wrapper.cc +++ b/libcef/browser/net_service/resource_request_handler_wrapper.cc @@ -166,7 +166,32 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { class DestructionObserver : public CefBrowserHostImpl::Observer, public CefContext::Observer { public: - DestructionObserver() = default; + explicit DestructionObserver(CefBrowserHostImpl* browser) { + if (browser) { + browser_info_ = browser->browser_info(); + browser->AddObserver(this); + } else { + CefContext::Get()->AddObserver(this); + } + } + + virtual ~DestructionObserver() { + CEF_REQUIRE_UIT(); + if (!registered_) + return; + + // Verify that the browser or context still exists before attempting to + // remove the observer. + if (browser_info_) { + auto browser = browser_info_->browser(); + if (browser) + browser->RemoveObserver(this); + } else if (CefContext::Get()) { + // Network requests may be torn down during shutdown, so we can't check + // CONTEXT_STATE_VALID() here. + CefContext::Get()->RemoveObserver(this); + } + } void SetWrapper(base::WeakPtr wrapper) { CEF_REQUIRE_IOT(); @@ -176,24 +201,32 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { void OnBrowserDestroyed(CefBrowserHostImpl* browser) override { CEF_REQUIRE_UIT(); browser->RemoveObserver(this); + registered_ = false; + browser_info_ = nullptr; NotifyOnDestroyed(); } void OnContextDestroyed() override { CEF_REQUIRE_UIT(); CefContext::Get()->RemoveObserver(this); + registered_ = false; NotifyOnDestroyed(); } private: void NotifyOnDestroyed() { - // It's not safe to test the WeakPtr on the UI thread, so we'll just post - // a task which will be ignored if the WeakPtr is invalid. - CEF_POST_TASK(CEF_IOT, base::BindOnce( - &InterceptedRequestHandlerWrapper::OnDestroyed, - wrapper_)); + if (wrapper_.MaybeValid()) { + // This will be a no-op if the WeakPtr is invalid. + CEF_POST_TASK( + CEF_IOT, + base::BindOnce(&InterceptedRequestHandlerWrapper::OnDestroyed, + wrapper_)); + } } + scoped_refptr browser_info_; + bool registered_ = true; + base::WeakPtr wrapper_; DISALLOW_COPY_AND_ASSIGN(DestructionObserver); }; @@ -211,25 +244,7 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { // InterceptedRequestHandlerWrapper::SetInitialized(). destruction_observer_->SetWrapper(nullptr); } - CEF_POST_TASK( - CEF_UIT, - base::BindOnce(&InitState::RemoveDestructionObserverOnUIThread, - browser_ ? browser_->browser_info() : nullptr, - std::move(destruction_observer_))); - } - } - - static void RemoveDestructionObserverOnUIThread( - scoped_refptr browser_info, - std::unique_ptr observer) { - // Verify that the browser or context is still valid before attempting to - // remove the observer. - if (browser_info) { - auto browser = browser_info->browser(); - if (browser) - browser->RemoveObserver(observer.get()); - } else if (CONTEXT_STATE_VALID()) { - CefContext::Get()->RemoveObserver(observer.get()); + DeleteDestructionObserver(); } } @@ -253,15 +268,12 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { // that we can stop accepting new requests and cancel pending/in-progress // requests in a timely manner (e.g. before we start asserting about // leaked objects during CEF shutdown). - destruction_observer_.reset(new DestructionObserver()); + destruction_observer_.reset(new DestructionObserver(browser.get())); if (browser) { // These references will be released in OnDestroyed(). browser_ = browser; frame_ = frame; - browser->AddObserver(destruction_observer_.get()); - } else { - CefContext::Get()->AddObserver(destruction_observer_.get()); } render_process_id_ = render_process_id; @@ -279,6 +291,17 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { DCHECK(!user_agent_.empty()); } + void DeleteDestructionObserver() { + DCHECK(destruction_observer_); + CEF_POST_TASK( + CEF_UIT, + base::BindOnce(&InitState::DeleteDestructionObserverOnUIThread, + std::move(destruction_observer_))); + } + + static void DeleteDestructionObserverOnUIThread( + std::unique_ptr observer) {} + // Only accessed on the UI thread. content::BrowserContext* browser_context_ = nullptr; @@ -1071,8 +1094,7 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { CEF_REQUIRE_IOT(); DCHECK(init_state_); - DCHECK(init_state_->destruction_observer_); - init_state_->destruction_observer_.reset(); + init_state_->DeleteDestructionObserver(); // Stop accepting new requests. shutting_down_ = true; @@ -1207,6 +1229,7 @@ void InitOnUIThread( } #endif + // May return nullptr for requests originating from guest views. browserPtr = CefBrowserHostImpl::GetBrowserForHost(frame); if (browserPtr) { framePtr = browserPtr->GetFrameForHost(frame); @@ -1253,6 +1276,8 @@ std::unique_ptr CreateInterceptedRequestHandler( if (frame) { render_frame_id = frame->GetRoutingID(); frame_tree_node_id = frame->GetFrameTreeNodeId(); + + // May return nullptr for requests originating from guest views. browserPtr = CefBrowserHostImpl::GetBrowserForHost(frame); if (browserPtr) { framePtr = browserPtr->GetFrameForHost(frame);