Fix crash when backgrounding MojoShell on Android. Fixes issue #186.
When backgrounding MojoShell while running spinning-square.sky (in sky viewer), we had a crash due to CommandBufferImpl binding on one thread, but destroying its binding on another thread (hence the underlying HandleWatcher didn't get removed correctly from the right run loop).
This CL fixes the issue by making sure binding and destruction happen on the same thread, control_task_runner.
R=qsr@chromium.org
Review URL: https://codereview.chromium.org/1161533005
diff --git a/services/gles2/command_buffer_impl.cc b/services/gles2/command_buffer_impl.cc
index 29a772c..0903cc9 100644
--- a/services/gles2/command_buffer_impl.cc
+++ b/services/gles2/command_buffer_impl.cc
@@ -36,9 +36,7 @@
}
void DidLoseContext() override {
- control_task_runner_->PostTask(
- FROM_HERE, base::Bind(&CommandBufferImpl::DidLoseContext,
- command_buffer_));
+ command_buffer_->DidLoseContext();
}
base::WeakPtr<CommandBufferImpl> command_buffer_;
@@ -53,6 +51,7 @@
gpu::SyncPointManager* sync_point_manager,
scoped_ptr<CommandBufferDriver> driver)
: sync_point_manager_(sync_point_manager),
+ control_task_runner_(control_task_runner),
driver_task_runner_(base::MessageLoop::current()->task_runner()),
driver_(driver.Pass()),
viewport_parameter_listener_(listener.Pass()),
@@ -62,15 +61,12 @@
driver_->set_client(make_scoped_ptr(new CommandBufferDriverClientImpl(
weak_factory_.GetWeakPtr(), control_task_runner)));
- control_task_runner->PostTask(
+ control_task_runner_->PostTask(
FROM_HERE, base::Bind(&CommandBufferImpl::BindToRequest,
base::Unretained(this), base::Passed(&request)));
}
CommandBufferImpl::~CommandBufferImpl() {
- if (observer_) {
- observer_->OnCommandBufferImplDestroyed();
- }
driver_task_runner_->PostTask(
FROM_HERE, base::Bind(&DestroyDriver, base::Passed(&driver_)));
}
@@ -147,10 +143,35 @@
void CommandBufferImpl::BindToRequest(
mojo::InterfaceRequest<mojo::CommandBuffer> request) {
binding_.Bind(request.Pass());
+ binding_.set_error_handler(this);
}
void CommandBufferImpl::DidLoseContext() {
- binding_.OnConnectionError();
+ NotifyAndDestroy();
+}
+
+void CommandBufferImpl::OnConnectionError() {
+ // Called from the control_task_runner thread as we bound to the message pipe
+ // handle on that thread. However, the observer have to be notified on the
+ // thread that created this object, so we post on driver_task_runner.
+ driver_task_runner_->PostTask(
+ FROM_HERE,
+ base::Bind(&CommandBufferImpl::NotifyAndDestroy, base::Unretained(this)));
+}
+
+void CommandBufferImpl::NotifyAndDestroy() {
+ if (observer_) {
+ observer_->OnCommandBufferImplDestroyed();
+ }
+ // We have notified the observer on the right thread. However, destruction of
+ // this object must happen on control_task_runner_
+ control_task_runner_->PostTask(
+ FROM_HERE,
+ base::Bind(&CommandBufferImpl::Destroy, base::Unretained(this)));
+}
+
+void CommandBufferImpl::Destroy() {
+ delete this;
}
void CommandBufferImpl::UpdateVSyncParameters(base::TimeTicks timebase,
diff --git a/services/gles2/command_buffer_impl.h b/services/gles2/command_buffer_impl.h
index 44bf85b..d2f79c4 100644
--- a/services/gles2/command_buffer_impl.h
+++ b/services/gles2/command_buffer_impl.h
@@ -8,7 +8,7 @@
#include "base/memory/scoped_ptr.h"
#include "base/memory/weak_ptr.h"
#include "base/single_thread_task_runner.h"
-#include "mojo/public/cpp/bindings/strong_binding.h"
+#include "mojo/public/cpp/bindings/binding.h"
#include "mojo/services/gpu/public/interfaces/command_buffer.mojom.h"
#include "mojo/services/gpu/public/interfaces/viewport_parameter_listener.mojom.h"
@@ -23,7 +23,7 @@
// so that we can insert sync points without blocking on the GL driver. It
// forwards most method calls to the CommandBufferDriver, which runs on the
// same thread as the native viewport.
-class CommandBufferImpl : public mojo::CommandBuffer {
+class CommandBufferImpl : public mojo::CommandBuffer, mojo::ErrorHandler {
public:
class Observer {
public:
@@ -57,17 +57,26 @@
void UpdateVSyncParameters(base::TimeTicks timebase,
base::TimeDelta interval);
+ // Sets an observer for CommandBufferImpl destruction. An observer registered
+ // here will be notified of the destruction of this object on the thread used
+ // to create it, before the destruction happens.
void set_observer(Observer* observer) { observer_ = observer; }
+ // mojo::ErrorHandler implementation
+ void OnConnectionError() override;
+
private:
void BindToRequest(mojo::InterfaceRequest<CommandBuffer> request);
+ void NotifyAndDestroy();
+ void Destroy();
scoped_refptr<gpu::SyncPointManager> sync_point_manager_;
+ scoped_refptr<base::SingleThreadTaskRunner> control_task_runner_;
scoped_refptr<base::SingleThreadTaskRunner> driver_task_runner_;
scoped_ptr<CommandBufferDriver> driver_;
mojo::CommandBufferSyncPointClientPtr sync_point_client_;
mojo::ViewportParameterListenerPtr viewport_parameter_listener_;
- mojo::StrongBinding<CommandBuffer> binding_;
+ mojo::Binding<CommandBuffer> binding_;
Observer* observer_;
base::WeakPtrFactory<CommandBufferImpl> weak_factory_;
diff --git a/services/native_viewport/onscreen_context_provider.cc b/services/native_viewport/onscreen_context_provider.cc
index a7ba14b..90a98dc 100644
--- a/services/native_viewport/onscreen_context_provider.cc
+++ b/services/native_viewport/onscreen_context_provider.cc
@@ -32,8 +32,9 @@
widget_ = widget;
if (widget_ == gfx::kNullAcceleratedWidget) {
- if (command_buffer_impl_)
+ if (command_buffer_impl_) {
command_buffer_impl_->DidLoseContext();
+ }
return;
}
