Skip to content

Commit

Permalink
[Presentation API] Fix race condition where Mojo pipes aren't closed.
Browse files Browse the repository at this point in the history
Race condition introduced in:
https://chromium-review.googlesource.com/c/chromium/src/+/724724

The crash is caused by a race condition, where the the renderer attempted
to register another PresentationController to PresentationServiceImpl
while there is still a (soon-to-be invalid) one already. When we moved
the PresentationController implementation from PresentationDispatcher to
blink::PresentationController, we are now creating/destoying the
PresentationController across navigation (instead of having it
long-lived in the PresentationDispatcher / RenderFrameImpl).

The fix is to close all message pipes / Reset() in
PresentationServiceImpl when a Mojo connection error is detected.
This way, the PresentationServiceImpl will be in a clean state when
the renderer connects to it again.

This also fixes PresentationReceiver's behavior of obtaining a
connection to PresentationService and immediately dropping it after
calling SetReceiver(), which would let to Reset() getting called with
this patch.

To merge back to 66 (if possible) and 67.

Bug: 832176
Change-Id: Ic7cd2601a107024143936fa9e1ae197505e4cf64
Reviewed-on: https://chromium-review.googlesource.com/1011289
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Commit-Queue: Derek Cheng <imcheng@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#551057}(cherry picked from commit 1ad7724)
Reviewed-on: https://chromium-review.googlesource.com/1017319
Cr-Commit-Position: refs/branch-heads/3396@{#85}
Cr-Branched-From: 9ef2aa8-refs/heads/master@{#550428}
  • Loading branch information
Derek Cheng committed Apr 18, 2018
1 parent 3f2a541 commit 77e751d
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 3 deletions.
11 changes: 11 additions & 0 deletions content/browser/presentation/presentation_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ PresentationServiceImpl::PresentationServiceImpl(

if (auto* delegate = GetPresentationServiceDelegate())
delegate->AddObserver(render_process_id_, render_frame_id_, this);

bindings_.set_connection_error_handler(base::BindRepeating(
&PresentationServiceImpl::OnConnectionError, base::Unretained(this)));
}

PresentationServiceImpl::~PresentationServiceImpl() {
Expand Down Expand Up @@ -118,6 +121,8 @@ void PresentationServiceImpl::SetController(
return;
}
controller_ = std::move(controller);
controller_.set_connection_error_handler(base::BindOnce(
&PresentationServiceImpl::OnConnectionError, base::Unretained(this)));
}

void PresentationServiceImpl::SetReceiver(
Expand All @@ -138,6 +143,8 @@ void PresentationServiceImpl::SetReceiver(
}

receiver_ = std::move(receiver);
receiver_.set_connection_error_handler(base::BindOnce(
&PresentationServiceImpl::OnConnectionError, base::Unretained(this)));
receiver_delegate_->RegisterReceiverConnectionAvailableCallback(
base::Bind(&PresentationServiceImpl::OnReceiverConnectionAvailable,
weak_factory_.GetWeakPtr()));
Expand Down Expand Up @@ -387,6 +394,10 @@ bool PresentationServiceImpl::FrameMatches(
render_frame_host->GetRoutingID() == render_frame_id_;
}

void PresentationServiceImpl::OnConnectionError() {
Reset();
}

PresentationServiceDelegate*
PresentationServiceImpl::GetPresentationServiceDelegate() {
return receiver_delegate_
Expand Down
4 changes: 4 additions & 0 deletions content/browser/presentation/presentation_service_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,10 @@ class CONTENT_EXPORT PresentationServiceImpl
// Returns true if this object is associated with |render_frame_host|.
bool FrameMatches(content::RenderFrameHost* render_frame_host) const;

// Invoked on Mojo connection error. Closes all Mojo message pipes held by
// |this|.
void OnConnectionError();

// Returns |controller_delegate| if current frame is controller frame; Returns
// |receiver_delegate| if current frame is receiver frame.
PresentationServiceDelegate* GetPresentationServiceDelegate();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,12 @@ ScriptPromise PresentationReceiver::connectionList(ScriptState* script_state) {
void PresentationReceiver::Init() {
DCHECK(!receiver_binding_.is_bound());

mojom::blink::PresentationServicePtr presentation_service;
auto* interface_provider = GetFrame()->Client()->GetInterfaceProvider();
interface_provider->GetInterface(mojo::MakeRequest(&presentation_service));
interface_provider->GetInterface(mojo::MakeRequest(&presentation_service_));

mojom::blink::PresentationReceiverPtr receiver_ptr;
receiver_binding_.Bind(mojo::MakeRequest(&receiver_ptr));
presentation_service->SetReceiver(std::move(receiver_ptr));
presentation_service_->SetReceiver(std::move(receiver_ptr));
}

void PresentationReceiver::OnReceiverTerminated() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class MODULES_EXPORT PresentationReceiver final
Member<PresentationConnectionList> connection_list_;

mojo::Binding<mojom::blink::PresentationReceiver> receiver_binding_;
mojom::blink::PresentationServicePtr presentation_service_;
WebPresentationClient* client_;
};

Expand Down

0 comments on commit 77e751d

Please sign in to comment.