From 16819b9d695c4bb204ff5d28db1e43787e8d9d35 Mon Sep 17 00:00:00 2001 From: imcheng Date: Fri, 3 Apr 2015 01:52:45 -0700 Subject: [PATCH] [Presentation API] Fix stability issues with PresentationService. - Temporarily comment out the ListenForDefaultSessionStart and ListenForSessionStateChange invoked by PresentationDispatcher right after connecting to PresentationServiceImpl. These calls contain callbacks which are dropped by PSImpl since they are not implemented, which leads to connection error. - Switch to use mojo::Binding in PSImpl. PSImpl is destroyed when a connection error occurs. Also since the frame can be destroyed before connection error, make sure we stop referencing the RFH after RenderFrameDeleted(). Review URL: https://codereview.chromium.org/1054713002 Cr-Commit-Position: refs/heads/master@{#323710} --- .../presentation/presentation_service_impl.cc | 33 ++++++++++++++-------- .../presentation/presentation_service_impl.h | 13 +++++++-- .../presentation_service_impl_unittest.cc | 24 +++++++++------- .../presentation/presentation_dispatcher.cc | 4 +++ 4 files changed, 50 insertions(+), 24 deletions(-) diff --git a/content/browser/presentation/presentation_service_impl.cc b/content/browser/presentation/presentation_service_impl.cc index fa4fa3d97983..9e2fd22e9c8c 100644 --- a/content/browser/presentation/presentation_service_impl.cc +++ b/content/browser/presentation/presentation_service_impl.cc @@ -50,19 +50,27 @@ void PresentationServiceImpl::CreateMojoService( WebContents::FromRenderFrameHost(render_frame_host); DCHECK(web_contents); - mojo::BindToRequest( - new PresentationServiceImpl( - render_frame_host, - web_contents, - GetContentClient()->browser()->GetPresentationServiceDelegate( - web_contents)), - &request); + // This object will be deleted when the RenderFrameHost is about to be + // deleted (RenderFrameDeleted) or if a connection error occurred + // (OnConnectionError). + PresentationServiceImpl* impl = new PresentationServiceImpl( + render_frame_host, + web_contents, + GetContentClient()->browser()->GetPresentationServiceDelegate( + web_contents)); + impl->Bind(request.Pass()); +} + +void PresentationServiceImpl::Bind( + mojo::InterfaceRequest request) { + binding_.reset(new mojo::Binding( + this, request.Pass())); + binding_->set_error_handler(this); } void PresentationServiceImpl::OnConnectionError() { - DVLOG(1) << "OnConnectionError: " - << render_frame_host_->GetProcess()->GetID() << ", " - << render_frame_host_->GetRoutingID(); + DVLOG(1) << "OnConnectionError"; + delete this; } PresentationServiceImpl::ScreenAvailabilityContext* @@ -336,8 +344,11 @@ void PresentationServiceImpl::RenderFrameDeleted( if (render_frame_host_ != render_frame_host) return; - // RenderFrameDeleted means this object is getting deleted soon. + // RenderFrameDeleted means |render_frame_host_| is going to be deleted soon. + // This object should also be deleted. Reset(); + render_frame_host_ = nullptr; + delete this; } void PresentationServiceImpl::Reset() { diff --git a/content/browser/presentation/presentation_service_impl.h b/content/browser/presentation/presentation_service_impl.h index 135320da4093..5b4972d62db9 100644 --- a/content/browser/presentation/presentation_service_impl.h +++ b/content/browser/presentation/presentation_service_impl.h @@ -39,8 +39,8 @@ class RenderFrameHost; // This class is instantiated on-demand via Mojo's ConnectToRemoteService // from the renderer when the first presentation API request is handled. class CONTENT_EXPORT PresentationServiceImpl - : public NON_EXPORTED_BASE( - mojo::InterfaceImpl), + : public NON_EXPORTED_BASE(presentation::PresentationService), + public mojo::ErrorHandler, public WebContentsObserver, public PresentationServiceDelegate::Observer { public: @@ -172,7 +172,10 @@ class CONTENT_EXPORT PresentationServiceImpl void ListenForSessionStateChange( const SessionStateCallback& callback) override; - // mojo::InterfaceImpl override. + // Creates a binding between this object and |request|. + void Bind(mojo::InterfaceRequest request); + + // mojo::ErrorHandler override. // Note that this is called when the RenderFrameHost is deleted. void OnConnectionError() override; @@ -259,6 +262,10 @@ class CONTENT_EXPORT PresentationServiceImpl int next_request_session_id_; base::hash_map> pending_session_cbs_; + // RAII binding of |this| to an Presentation interface request. + // The binding is removed when binding_ is cleared or goes out of scope. + scoped_ptr> binding_; + // NOTE: Weak pointers must be invalidated before all other member variables. base::WeakPtrFactory weak_factory_; diff --git a/content/browser/presentation/presentation_service_impl_unittest.cc b/content/browser/presentation/presentation_service_impl_unittest.cc index 93d02f5cf0fa..1f7f75840539 100644 --- a/content/browser/presentation/presentation_service_impl_unittest.cc +++ b/content/browser/presentation/presentation_service_impl_unittest.cc @@ -74,19 +74,20 @@ class PresentationServiceImplTest : public RenderViewHostImplTestHarness { void SetUp() override { RenderViewHostImplTestHarness::SetUp(); + auto request = mojo::GetProxy(&service_ptr_); EXPECT_CALL(mock_delegate_, AddObserver(_)).Times(1); - service_impl_.reset(mojo::WeakBindToProxy( - new PresentationServiceImpl( - contents()->GetMainFrame(), contents(), &mock_delegate_), - &service_ptr_)); + service_impl_.reset(new PresentationServiceImpl( + contents()->GetMainFrame(), contents(), &mock_delegate_)); + service_impl_->Bind(request.Pass()); } void TearDown() override { service_ptr_.reset(); - - EXPECT_CALL(mock_delegate_, RemoveObserver(Eq(service_impl_.get()))) - .Times(1); - service_impl_.reset(); + if (service_impl_.get()) { + EXPECT_CALL(mock_delegate_, RemoveObserver(Eq(service_impl_.get()))) + .Times(1); + service_impl_.reset(); + } RenderViewHostImplTestHarness::TearDown(); } @@ -290,8 +291,11 @@ TEST_F(PresentationServiceImplTest, ThisRenderFrameDeleted) { true); ExpectReset(); - service_impl_->RenderFrameDeleted(contents()->GetMainFrame()); - ExpectCleanState(); + + // Since the frame matched the service, |service_impl_| will be deleted. + PresentationServiceImpl* service = service_impl_.release(); + EXPECT_CALL(mock_delegate_, RemoveObserver(Eq(service))).Times(1); + service->RenderFrameDeleted(contents()->GetMainFrame()); } TEST_F(PresentationServiceImplTest, NotThisRenderFrameDeleted) { diff --git a/content/renderer/presentation/presentation_dispatcher.cc b/content/renderer/presentation/presentation_dispatcher.cc index c050a8bea698..39e5238b189b 100644 --- a/content/renderer/presentation/presentation_dispatcher.cc +++ b/content/renderer/presentation/presentation_dispatcher.cc @@ -216,12 +216,16 @@ void PresentationDispatcher::ConnectToPresentationServiceIfNeeded() { render_frame()->GetServiceRegistry()->ConnectToRemoteService( &presentation_service_); + // TODO(imcheng): Uncomment these once they are implemented on the browser + // side. (crbug.com/459006) + /* presentation_service_->ListenForDefaultSessionStart(base::Bind( &PresentationDispatcher::OnDefaultSessionStarted, base::Unretained(this))); presentation_service_->ListenForSessionStateChange(base::Bind( &PresentationDispatcher::OnSessionStateChange, base::Unretained(this))); + */ } } // namespace content -- 2.11.4.GIT