Synthetic input waits on compositor display
This CL makes synthetic input - the kind used in web tests and
telemetry (e.g. gpuBenchmarking.scrollBy) - wait until a CompositorFrame
has been submitted by the renderer and displayed by the display
compositor before resolving the completion callback. This means client
code that wants to wait until any observable side-effects of this input
is visible to further input need only wait on the gesture's completion
callback.
To give a motivating example: suppose we wish to write a test that
scrolls an out-of-process iframe into view and clicks on a button in the
frame. The code might look something like this:
gpuBenchmarking.smoothScroll(1000, () => {
gpuBenchmarking.tap(0, 0);
});
This code contains a race today. The callback for smoothScroll is
invoked as soon as the ScrollEnd is received in the renderer. However,
until a new compositor frame is submitted from the renderer, the tap
may occur against stale hit testing geometry. This is a major source of
flakiness in our tests.
This CL fixes the problem by forcing the renderer to perform a full
redraw at the end of each gesture. The redraw produces a compositor
frame and we invoke the callback once the compositor frame is displayed.
We do this by reusing the RequestPresentation mechanism in RenderWidget.
RequestPresentation required two modifications to work in web tests
which use a single thread proxy with no scheduler:
- LayerTreeHost::Composite needs to check the forced redraw flag to
determine whether we need to raster, otherwise it won't produce a
frame
- RequestPresentation must request a main frame since there's no
scheduler to perform the commit, which is what SetNeedsForcedRedraw
requests.
The timing change exposed an issue in the
overlay-play-button-tap-to-hide.html test so this CL also cleans that
test up to listen to the animation changes in media controls properly.
Finally, it's possible we may get input in a RenderWidget that's not
currently displayed. e.g. A click event sent via ChromeDriver causes a
TouchStart followed by a TouchEnd. The TouchStart causes a window.open
which opens and focuses a new tab. The TouchEnd then happens on the
background tab. In this case, we should resolve the callback rather than
waiting on a CompositorFrame that'll never come. See ChromeDriver test
testNetworkConnectionTypeIsAppliedToAllTabs for an example of this.
Bug: 902446
Change-Id: Ib2dddee08400dfa1137c674c47c0d7106961162f
Reviewed-on: https://chromium-review.googlesource.com/c/1390329
Reviewed-by: Saman Sami <samans@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#633753}diff --git a/cc/trees/layer_tree_host.cc b/cc/trees/layer_tree_host.cc
index cb3c29d..6c0996a 100644
--- a/cc/trees/layer_tree_host.cc
+++ b/cc/trees/layer_tree_host.cc
@@ -663,7 +663,8 @@
DCHECK(!settings_.single_thread_proxy_scheduler);
SingleThreadProxy* proxy = static_cast<SingleThreadProxy*>(proxy_.get());
- proxy->CompositeImmediately(frame_begin_time, raster);
+ proxy->CompositeImmediately(frame_begin_time,
+ raster || next_commit_forces_redraw_);
}
bool LayerTreeHost::UpdateLayers() {
diff --git a/content/browser/renderer_host/render_widget_host_impl.cc b/content/browser/renderer_host/render_widget_host_impl.cc
index 96d1d48..b5b6df1 100644
--- a/content/browser/renderer_host/render_widget_host_impl.cc
+++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -1322,6 +1322,14 @@
SyntheticGestureParams::GestureType type,
SyntheticGestureParams::GestureSourceType source,
base::OnceClosure callback) {
+ // TODO(bokan): The RequestPresentationCallback mechanism doesn't seem to
+ // work in OOPIFs. For now, just callback immediately. Remove when fixed.
+ // https://crbug.com/924646.
+ if (GetView()->IsRenderWidgetHostViewChildFrame()) {
+ std::move(callback).Run();
+ return;
+ }
+
// TODO(bokan): Input can be queued and delayed in InputRouterImpl based on
// the kind of events we're getting. To be truly robust, we should wait until
// those queues are flushed before issuing this message. This will be done in
diff --git a/content/renderer/input/widget_input_handler_impl.cc b/content/renderer/input/widget_input_handler_impl.cc
index fbc544cf..6e7a3d1a 100644
--- a/content/renderer/input/widget_input_handler_impl.cc
+++ b/content/renderer/input/widget_input_handler_impl.cc
@@ -47,9 +47,7 @@
: main_thread_task_runner_(main_thread_task_runner),
input_handler_manager_(manager),
input_event_queue_(input_event_queue),
- render_widget_(render_widget),
- binding_(this),
- associated_binding_(this) {}
+ render_widget_(render_widget) {}
WidgetInputHandlerImpl::~WidgetInputHandlerImpl() {}
@@ -162,8 +160,22 @@
void WidgetInputHandlerImpl::WaitForInputProcessed(
WaitForInputProcessedCallback callback) {
- // TODO(bokan): Implement this to actually ensure input is processed.
- std::move(callback).Run();
+ DCHECK(!input_processed_ack_);
+
+ // Store so that we can respond even if the renderer is destructed.
+ input_processed_ack_ = std::move(callback);
+
+ input_handler_manager_->WaitForInputProcessed(
+ base::BindOnce(&WidgetInputHandlerImpl::InputWasProcessed,
+ weak_ptr_factory_.GetWeakPtr()));
+}
+
+void WidgetInputHandlerImpl::InputWasProcessed() {
+ // The callback can be be invoked when the renderer is hidden and then again
+ // when it's shown. We can also be called after Release is called so always
+ // check that the callback exists.
+ if (input_processed_ack_)
+ std::move(input_processed_ack_).Run();
}
void WidgetInputHandlerImpl::AttachSynchronousCompositor(
@@ -184,6 +196,15 @@
}
void WidgetInputHandlerImpl::Release() {
+ // If the renderer is closed, make sure we ack the outstanding Mojo callback
+ // so that we don't DCHECK and/or leave the browser-side blocked for an ACK
+ // that will never come if the renderer is destroyed before this callback is
+ // invoked. Note, this method will always be called on the Mojo-bound thread
+ // first and then again on the main thread, the callback will always be
+ // called on the Mojo-bound thread though.
+ if (input_processed_ack_)
+ std::move(input_processed_ack_).Run();
+
if (!main_thread_task_runner_->BelongsToCurrentThread()) {
// Close the binding on the compositor thread first before telling the main
// thread to delete this object.
diff --git a/content/renderer/input/widget_input_handler_impl.h b/content/renderer/input/widget_input_handler_impl.h
index 4e9fa4e..f704aed 100644
--- a/content/renderer/input/widget_input_handler_impl.h
+++ b/content/renderer/input/widget_input_handler_impl.h
@@ -5,6 +5,7 @@
#ifndef CONTENT_RENDERER_INPUT_WIDGET_INPUT_HANDLER_IMPL_H_
#define CONTENT_RENDERER_INPUT_WIDGET_INPUT_HANDLER_IMPL_H_
+#include "base/memory/weak_ptr.h"
#include "base/single_thread_task_runner.h"
#include "content/common/input/input_handler.mojom.h"
#include "mojo/public/cpp/bindings/associated_binding.h"
@@ -60,6 +61,7 @@
mojom::SynchronousCompositorHostAssociatedPtrInfo host,
mojom::SynchronousCompositorAssociatedRequest compositor_request)
override;
+ void InputWasProcessed();
private:
bool ShouldProxyToMainThread() const;
@@ -71,8 +73,15 @@
scoped_refptr<MainThreadEventQueue> input_event_queue_;
base::WeakPtr<RenderWidget> render_widget_;
- mojo::Binding<mojom::WidgetInputHandler> binding_;
- mojo::AssociatedBinding<mojom::WidgetInputHandler> associated_binding_;
+ // This callback is used to respond to the WaitForInputProcessed Mojo
+ // message. We keep it around so that we can respond even if the renderer is
+ // killed before we actually fully process the input.
+ WaitForInputProcessedCallback input_processed_ack_;
+
+ mojo::Binding<mojom::WidgetInputHandler> binding_{this};
+ mojo::AssociatedBinding<mojom::WidgetInputHandler> associated_binding_{this};
+
+ base::WeakPtrFactory<WidgetInputHandlerImpl> weak_ptr_factory_{this};
DISALLOW_COPY_AND_ASSIGN(WidgetInputHandlerImpl);
};
diff --git a/content/renderer/input/widget_input_handler_manager.cc b/content/renderer/input/widget_input_handler_manager.cc
index 9b251a0..21033c1 100644
--- a/content/renderer/input/widget_input_handler_manager.cc
+++ b/content/renderer/input/widget_input_handler_manager.cc
@@ -369,6 +369,75 @@
}
}
+void WidgetInputHandlerManager::InvokeInputProcessedCallback() {
+ DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
+
+ // We can call this method even if we didn't request a callback (e.g. when
+ // the renderer becomes hidden).
+ if (!input_processed_callback_)
+ return;
+
+ // The handler's method needs to respond to the mojo message so it needs to
+ // run on the input handling thread. PostTask to the correct task runner.
+ // Even if we're already on the correct thread, we PostTask for symmetry.
+ base::SingleThreadTaskRunner* mojo_bound_task_runner =
+ compositor_task_runner_ ? compositor_task_runner_.get()
+ : main_thread_task_runner_.get();
+
+ mojo_bound_task_runner->PostTask(FROM_HERE,
+ std::move(input_processed_callback_));
+}
+
+void WidgetInputHandlerManager::InputWasProcessed(
+ const gfx::PresentationFeedback&) {
+ InvokeInputProcessedCallback();
+}
+
+static void WaitForInputProcessedFromMain(
+ base::WeakPtr<RenderWidget> render_widget) {
+ // If the widget is destroyed while we're posting to the main thread, the
+ // Mojo message will be acked in WidgetInputHandlerImpl's destructor.
+ if (!render_widget)
+ return;
+
+ WidgetInputHandlerManager* manager =
+ render_widget->widget_input_handler_manager();
+
+ // If the RenderWidget is hidden, we won't produce compositor frames for it
+ // so just ACK the input to prevent blocking the browser indefinitely.
+ if (render_widget->is_hidden()) {
+ manager->InvokeInputProcessedCallback();
+ return;
+ }
+
+ auto redraw_complete_callback = base::BindOnce(
+ &WidgetInputHandlerManager::InputWasProcessed, manager->AsWeakPtr());
+
+ // We consider all observable effects of an input gesture to be processed
+ // when the CompositorFrame caused by that input has been produced, send, and
+ // displayed. RequestPresentation will force a a commit and redraw and
+ // callback when the CompositorFrame has been displayed in the display
+ // service. Some examples of non-trivial effects that require waiting that
+ // long: committing NonFastScrollRegions to the compositor, sending
+ // touch-action rects to the browser, and sending updated surface information
+ // to the display compositor for up-to-date OOPIF hit-testing.
+ render_widget->RequestPresentation(std::move(redraw_complete_callback));
+}
+
+void WidgetInputHandlerManager::WaitForInputProcessed(
+ base::OnceClosure callback) {
+ // Note, this will be called from the mojo-bound thread which could be either
+ // main or compositor.
+ DCHECK(!input_processed_callback_);
+ input_processed_callback_ = std::move(callback);
+
+ // We musn't touch render_widget_ from the impl thread so post all the setup
+ // to the main thread.
+ main_thread_task_runner_->PostTask(
+ FROM_HERE,
+ base::BindOnce(&WaitForInputProcessedFromMain, render_widget_));
+}
+
void WidgetInputHandlerManager::InitOnCompositorThread(
const base::WeakPtr<cc::InputHandler>& input_handler,
bool smooth_scroll_enabled,
diff --git a/content/renderer/input/widget_input_handler_manager.h b/content/renderer/input/widget_input_handler_manager.h
index 3c2bdd4..2bb3a53 100644
--- a/content/renderer/input/widget_input_handler_manager.h
+++ b/content/renderer/input/widget_input_handler_manager.h
@@ -21,6 +21,10 @@
} // namespace scheduler
} // namespace blink
+namespace gfx {
+struct PresentationFeedback;
+} // namespace gfx
+
namespace content {
class MainThreadEventQueue;
class SynchronousCompositorRegistry;
@@ -29,9 +33,10 @@
// This class maintains the compositor InputHandlerProxy and is
// responsible for passing input events on the compositor and main threads.
// The lifecycle of this object matches that of the RenderWidget.
-class CONTENT_EXPORT WidgetInputHandlerManager
+class CONTENT_EXPORT WidgetInputHandlerManager final
: public base::RefCountedThreadSafe<WidgetInputHandlerManager>,
- public ui::InputHandlerProxyClient {
+ public ui::InputHandlerProxyClient,
+ public base::SupportsWeakPtr<WidgetInputHandlerManager> {
public:
static scoped_refptr<WidgetInputHandlerManager> Create(
base::WeakPtr<RenderWidget> render_widget,
@@ -85,6 +90,10 @@
content::SynchronousCompositorRegistry* GetSynchronousCompositorRegistry();
#endif
+ void InvokeInputProcessedCallback();
+ void InputWasProcessed(const gfx::PresentationFeedback& feedback);
+ void WaitForInputProcessed(base::OnceClosure callback);
+
protected:
friend class base::RefCountedThreadSafe<WidgetInputHandlerManager>;
~WidgetInputHandlerManager() override;
@@ -148,6 +157,11 @@
base::Optional<cc::TouchAction> white_listed_touch_action_;
+ // Callback used to respond to the WaitForInputProcessed Mojo message. This
+ // callback is set from and must be invoked from the Mojo-bound thread, it
+ // will call into the WidgetInputHandlerImpl.
+ base::OnceClosure input_processed_callback_;
+
#if defined(OS_ANDROID)
std::unique_ptr<SynchronousCompositorProxyRegistry>
synchronous_compositor_registry_;
diff --git a/content/renderer/render_widget.cc b/content/renderer/render_widget.cc
index ad56cd0..3cfcf4c8 100644
--- a/content/renderer/render_widget.cc
+++ b/content/renderer/render_widget.cc
@@ -972,10 +972,19 @@
}
void RenderWidget::OnForceRedraw(int snapshot_id) {
+ RequestPresentation(base::BindOnce(&RenderWidget::DidPresentForceDrawFrame,
+ weak_ptr_factory_.GetWeakPtr(),
+ snapshot_id));
+}
+
+void RenderWidget::RequestPresentation(PresentationTimeCallback callback) {
layer_tree_view_->layer_tree_host()->RequestPresentationTimeForNextFrame(
- base::BindOnce(&RenderWidget::DidPresentForceDrawFrame,
- weak_ptr_factory_.GetWeakPtr(), snapshot_id));
+ std::move(callback));
layer_tree_view_->SetNeedsForcedRedraw();
+
+ // Need this since single thread mode doesn't have a scheduler so the above
+ // call won't cause us to generate a new frame.
+ ScheduleAnimation();
}
void RenderWidget::DidPresentForceDrawFrame(
@@ -2494,6 +2503,11 @@
if (render_widget_scheduling_state_)
render_widget_scheduling_state_->SetHidden(hidden);
+ // If the renderer was hidden, resolve any pending synthetic gestures so they
+ // aren't blocked waiting for a compositor frame to be generated.
+ if (is_hidden_)
+ widget_input_handler_manager_->InvokeInputProcessedCallback();
+
StartStopCompositor();
}
diff --git a/content/renderer/render_widget.h b/content/renderer/render_widget.h
index 6205ae9..b04fa9e 100644
--- a/content/renderer/render_widget.h
+++ b/content/renderer/render_widget.h
@@ -93,6 +93,7 @@
namespace gfx {
class ColorSpace;
+struct PresentationFeedback;
class Range;
}
@@ -598,6 +599,12 @@
// TODO(ajwong): This should be moved into RenderView.
void UpdateWebViewWithDeviceScaleFactor();
+ // Forces a redraw and invokes the callback once the frame's been displayed
+ // to the user.
+ using PresentationTimeCallback =
+ base::OnceCallback<void(const gfx::PresentationFeedback&)>;
+ void RequestPresentation(PresentationTimeCallback callback);
+
// RenderWidget IPC message handler that can be overridden by subclasses.
virtual void OnSynchronizeVisualProperties(const VisualProperties& params);
diff --git a/third_party/blink/web_tests/media/controls/modern/overlay-play-button-tap-to-hide.html b/third_party/blink/web_tests/media/controls/modern/overlay-play-button-tap-to-hide.html
index 729fcc0..96e1e2e 100644
--- a/third_party/blink/web_tests/media/controls/modern/overlay-play-button-tap-to-hide.html
+++ b/third_party/blink/web_tests/media/controls/modern/overlay-play-button-tap-to-hide.html
@@ -18,28 +18,34 @@
const button = mediaControlsOverlayPlayButtonInternal(video);
const controls = mediaControls(video);
const overlay = mediaControlsOverlayPlayButton(video);
+ let call_count = 0;
// Make sure the overlay button is not hidden.
assert_false(overlay.classList.contains('hidden'));
- // Wait until we have loaded the video and tap on the button.
+ overlay.addEventListener('transitionend', t.step_func(() => {
+ call_count++;
+ if (call_count == 1) {
+ // Now tap anywhere on the controls and make sure the button
+ // unhides itself.
+ assert_true(overlay.classList.contains('hidden'));
+ singleTapOnControl(controls);
+ } else if(call_count == 2) {
+ // The second time a transition ends, it must be the controls becoming
+ // visible.
+ assert_false(overlay.classList.contains('hidden'));
+ t.done();
+ } else {
+ assert_unreached("Unexpected transitionend event");
+ }
+ }));
+
+ // Wait until we have loaded the video and tap on the button. This will
+ // transition the button to visible.
video.addEventListener('canplay', t.step_func(() => {
singleTapOnControl(button);
}), { once: true });
- // When we start playing make sure the overlay button is hidden.
- video.addEventListener('play', t.step_func(() => {
- assert_true(overlay.classList.contains('hidden'));
-
- // Now tap anywhere on the controls and make sure the button
- // unhides itself.
- singleTapOnControl(controls, t.step_func(() => {
- overlay.addEventListener('transitionend', t.step_func_done(() => {
- assert_false(overlay.classList.contains('hidden'));
- }));
- }));
- }), { once: true });
-
video.src = '../../content/test.webm';
});
</script>
diff --git a/third_party/blink/web_tests/media/media-controls.js b/third_party/blink/web_tests/media/media-controls.js
index 87ba2f5..bd173bf 100644
--- a/third_party/blink/web_tests/media/media-controls.js
+++ b/third_party/blink/web_tests/media/media-controls.js
@@ -553,7 +553,6 @@
}
function singleTapAtCoordinates(xPos, yPos, callback) {
- let delayCallback = function() { setTimeout(callback); };
chrome.gpuBenchmarking.pointerActionSequence([
{
source: 'mouse',
@@ -562,7 +561,7 @@
{ name: 'pointerUp' }
]
}
- ], delayCallback);
+ ], callback);
}
function singleTapOutsideControl(control, callback) {
