Revert "Synthetic input waits on compositor display"

This reverts commit 258ed8d5.

Reason for revert: Suspect this introduced memory leaks and broke https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/WebKit%20Linux%20Trusty%20Leak/30388

Original change's description:
> 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}

TBR=danakj@chromium.org,bokan@chromium.org,dtapuska@chromium.org,nzolghadr@chromium.org,samans@chromium.org

Change-Id: I7a479a4af8fbec4b4452ff6d9d7b5b9df246f4a4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 902446
Reviewed-on: https://chromium-review.googlesource.com/c/1479370Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#633782}

cc/trees/layer_tree_host.cc

@@ -663,8 +663,7 @@ void LayerTreeHost::Composite(base::TimeTicks frame_begin_time, bool raster) {
   DCHECK(!settings_.single_thread_proxy_scheduler);
   SingleThreadProxy* proxy = static_cast<SingleThreadProxy*>(proxy_.get());
 
-  proxy->CompositeImmediately(frame_begin_time,
-                              raster || next_commit_forces_redraw_);
+  proxy->CompositeImmediately(frame_begin_time, raster);
 }
 
 bool LayerTreeHost::UpdateLayers() {

content/browser/renderer_host/render_widget_host_impl.cc

@@ -1322,14 +1322,6 @@ void RenderWidgetHostImpl::WaitForInputProcessed(
     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

content/renderer/input/widget_input_handler_impl.cc

@@ -47,7 +47,9 @@ WidgetInputHandlerImpl::WidgetInputHandlerImpl(
     : main_thread_task_runner_(main_thread_task_runner),
       input_handler_manager_(manager),
       input_event_queue_(input_event_queue),
-      render_widget_(render_widget) {}
+      render_widget_(render_widget),
+      binding_(this),
+      associated_binding_(this) {}
 
 WidgetInputHandlerImpl::~WidgetInputHandlerImpl() {}
 
@@ -160,22 +162,8 @@ void WidgetInputHandlerImpl::DispatchNonBlockingEvent(
 
 void WidgetInputHandlerImpl::WaitForInputProcessed(
     WaitForInputProcessedCallback callback) {
-  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();
+  // TODO(bokan): Implement this to actually ensure input is processed.
+  std::move(callback).Run();
 }
 
 void WidgetInputHandlerImpl::AttachSynchronousCompositor(
@@ -196,15 +184,6 @@ void WidgetInputHandlerImpl::RunOnMainThread(base::OnceClosure closure) {
 }
 
 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.

content/renderer/input/widget_input_handler_impl.h

@@ -5,7 +5,6 @@
 #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"
@@ -61,7 +60,6 @@ class WidgetInputHandlerImpl : public mojom::WidgetInputHandler {
       mojom::SynchronousCompositorHostAssociatedPtrInfo host,
       mojom::SynchronousCompositorAssociatedRequest compositor_request)
       override;
-  void InputWasProcessed();
 
  private:
   bool ShouldProxyToMainThread() const;
@@ -73,15 +71,8 @@ class WidgetInputHandlerImpl : public mojom::WidgetInputHandler {
   scoped_refptr<MainThreadEventQueue> input_event_queue_;
   base::WeakPtr<RenderWidget> render_widget_;
 
-  // 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};
+  mojo::Binding<mojom::WidgetInputHandler> binding_;
+  mojo::AssociatedBinding<mojom::WidgetInputHandler> associated_binding_;
 
   DISALLOW_COPY_AND_ASSIGN(WidgetInputHandlerImpl);
 };

content/renderer/input/widget_input_handler_manager.cc

@@ -369,75 +369,6 @@ void WidgetInputHandlerManager::DispatchEvent(
   }
 }
 
-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,

content/renderer/input/widget_input_handler_manager.h

@@ -21,10 +21,6 @@ class WebThreadScheduler;
 }  // namespace scheduler
 }  // namespace blink
 
-namespace gfx {
-struct PresentationFeedback;
-}  // namespace gfx
-
 namespace content {
 class MainThreadEventQueue;
 class SynchronousCompositorRegistry;
@@ -33,10 +29,9 @@ class SynchronousCompositorProxyRegistry;
 // 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 final
+class CONTENT_EXPORT WidgetInputHandlerManager
     : public base::RefCountedThreadSafe<WidgetInputHandlerManager>,
-      public ui::InputHandlerProxyClient,
-      public base::SupportsWeakPtr<WidgetInputHandlerManager> {
+      public ui::InputHandlerProxyClient {
  public:
   static scoped_refptr<WidgetInputHandlerManager> Create(
       base::WeakPtr<RenderWidget> render_widget,
@@ -90,10 +85,6 @@ class CONTENT_EXPORT WidgetInputHandlerManager final
   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;
@@ -157,11 +148,6 @@ class CONTENT_EXPORT WidgetInputHandlerManager final
 
   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_;

content/renderer/render_widget.cc

@@ -971,19 +971,10 @@ void RenderWidget::OnRequestSetBoundsAck() {
 }
 
 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(
-      std::move(callback));
+      base::BindOnce(&RenderWidget::DidPresentForceDrawFrame,
+                     weak_ptr_factory_.GetWeakPtr(), snapshot_id));
   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(
@@ -2502,11 +2493,6 @@ void RenderWidget::SetHidden(bool hidden) {
   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();
 }
 

content/renderer/render_widget.h

@@ -93,7 +93,6 @@ class SwapPromise;
 
 namespace gfx {
 class ColorSpace;
-struct PresentationFeedback;
 class Range;
 }
 
@@ -598,12 +597,6 @@ class CONTENT_EXPORT RenderWidget
   // 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);
 

third_party/blink/web_tests/media/controls/modern/overlay-play-button-tap-to-hide.html

@@ -18,34 +18,28 @@ async_test((t) => {
   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'));
 
-  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.
+  // Wait until we have loaded the video and tap on the button.
   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>

third_party/blink/web_tests/media/media-controls.js

@@ -553,6 +553,7 @@ function doubleTapAtCoordinates(x, y, timeout, callback) {
 }
 
 function singleTapAtCoordinates(xPos, yPos, callback) {
+  let delayCallback = function() { setTimeout(callback); };
   chrome.gpuBenchmarking.pointerActionSequence([
     {
       source: 'mouse',
@@ -561,7 +562,7 @@ function singleTapAtCoordinates(xPos, yPos, callback) {
         { name: 'pointerUp' }
       ]
     }
-  ], callback);
+  ], delayCallback);
 }
 
 function singleTapOutsideControl(control, callback) {