Test that visual state callbacks do not deadlock.

content/renderer/visual_state_browsertest.cc

Index: content/renderer/visual_state_browsertest.cc
diff --git a/content/renderer/visual_state_browsertest.cc b/content/renderer/visual_state_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..52d6e36278ab2fe9e99c7fa867b834cdb95b4f3e
--- /dev/null
+++ b/content/renderer/visual_state_browsertest.cc
@@ -0,0 +1,146 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "base/run_loop.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/content_switches.h"
+#include "content/public/renderer/render_view.h"
+#include "content/public/renderer/render_view_observer.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/test_utils.h"
+#include "content/renderer/render_frame_impl.h"
+#include "content/shell/browser/shell.h"
+
+namespace {
+// The first RenderFrame is routing ID 1, and the first RenderView is 2.
+const int kRenderViewRoutingId = 2;

[Sami, 2015/02/18 16:12:22]

Is there a way to query this, anyone? This seems a little fragile.

[Ignacio Solla, 2015/02/18 16:42:34]

Hardcoding it seems to be an extended practice in the codebase:

https://code.google.com/p/chromium/codesearch#search/&q=kRenderViewRoutingId&...

+
+const std::string kBlankUrl = "about:blank";
+
+// Timeout to wait for a commit. This number has been obtained by trial and
+// error. Try to keep this number as low as possible as one of the tests
+// will wait for this long. If the tests flake or start failing increasing this
+// number will likely fix it.
+const base::TimeDelta kCommitTimeout = base::TimeDelta::FromMilliseconds(150);

[piman, 2015/02/18 16:37:18]

Please no timeout. This is a sure way to make this test flaky (think valgrind).

[Ignacio Solla, 2015/02/18 17:01:59]

Without the timeout this test is useless. If the test is flaky then we need to
increase the timeout until it passes.

+}
+
+namespace content {
+
+class CommitObserver : public RenderViewObserver {
+ public:
+  CommitObserver(RenderView* render_view, const base::Closure& quit_closure)
+      : RenderViewObserver(render_view),
+        quit_closure_(quit_closure),
+        commit_count_(0) {}
+
+  void DidCommitCompositorFrame() override {
+    commit_count_++;
+    quit_closure_.Run();
+  }
+
+  int GetCommitCount() { return commit_count_; }
+
+ private:
+  base::Closure quit_closure_;
+  int commit_count_;
+};
+
+class VisualStateTest : public ContentBrowserTest {
+ public:
+  VisualStateTest() : callback_count_(0) {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kSingleProcess);

[Sami, 2015/02/18 16:12:22]

Do we really need to enforce this?

[piman, 2015/02/18 16:37:18]

Indeed, I don't think we want to run tests in that way. Single-process has a
bunch of things that are different from prod, we should test what we ship.

[Ignacio Solla, 2015/02/18 16:42:34]

Yes, to be able to use PostTaskToInProcessRendererAndWait.

+  }
+
+  // Waits at most kCommitTimeout and verifies that a single
+  // commit has completed.
+  void WaitForOneCommit() { WaitForCommits(1); }
+
+  // Waits for kCommitTimeout and verifies that no commits have
+  // completed.
+  void WaitForZeroCommits() { WaitForCommits(0); }
+
+  void InvokeVisualStateCallback(bool result) {
+    EXPECT_TRUE(result);
+    callback_count_++;
+  }
+
+  int GetCallbackCount() { return callback_count_; }
+
+ private:
+  RenderView* GetRenderView() {
+    // We could have the test on the UI thread get the WebContent's routing ID,
+    // but we know this will be the first RV so skip that and just hardcode it.

[piman, 2015/02/18 16:37:18]

please no.
One day something will change in how the routing IDs are assigned and this will
break.

[Ignacio Solla, 2015/02/18 17:01:59]

Hardcoding it seems to be an extended practice:
https://code.google.com/p/chromium/codesearch#search/&q=kRenderViewRoutingId&...

Could I keep this and then file a bug for somebody to fix all of the
occurrences? (I'm leaving so I will not be able to fix it myself :()

+    return RenderView::FromRoutingID(kRenderViewRoutingId);
+  }
+
+  RenderFrameImpl* GetRenderFrameImpl() {
+    return static_cast<RenderFrameImpl*>(GetRenderView()->GetMainRenderFrame());
+  }
+
+  void WaitForCommits(int commit_count) {
+    scoped_refptr<MessageLoopRunner> runner = new MessageLoopRunner;
+    CommitObserver observer(GetRenderView(), runner->QuitClosure());
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE, runner->QuitClosure(), kCommitTimeout);
+    runner->Run();
+    EXPECT_EQ(commit_count, observer.GetCommitCount());
+  }
+
+  int callback_count_;
+};
+
+// These two tests (ie. CallbackDoesNotDeadlockPart[1|2]) verify that visual
+// state callbacks do not deadlock. In other words, the visual state callback
+// should be received even if there are no pending updates or commits. To
+// achieve that the request to insert the callback requests a new commit that
+// should complete successfully (ie. LayerTreeHost::CommitComplete()).
+// CallbackDoesNotDeadlockPart1: verifies that this commit goes through.
+// CallbackDoesNotDeadlockPart2: verifies that loading kBlankUrl only requires a
+// single commit, so that the second commit in CallbackDoesNotDeadlockPart1 is
+// indeed caused by inserting the visual state callback.
+// Please see crbug/458577 for more details.
+IN_PROC_BROWSER_TEST_F(VisualStateTest, CallbackDoesNotDeadlockPart1) {
+  // By part 2 we know that loading kBlankUrl requires a single commit.
+  GURL url(kBlankUrl);
+  NavigateToURL(shell(), url);
+
+  // Wait for the commit corresponding to the load.
+  PostTaskToInProcessRendererAndWait(
+      base::Bind(&VisualStateTest::WaitForOneCommit, base::Unretained(this)));
+
+  // Insert a visual state callback. At this point we know by part 2 that there
+  // are no pending commits.
+  shell()->web_contents()->GetMainFrame()->InsertVisualStateCallback(base::Bind(
+      &VisualStateTest::InvokeVisualStateCallback, base::Unretained(this)));
+
+  // Verify that the callback is invoked and a new commit completed.
+  PostTaskToInProcessRendererAndWait(
+      base::Bind(&VisualStateTest::WaitForOneCommit, base::Unretained(this)));
+  EXPECT_EQ(1, GetCallbackCount());
+}
+
+IN_PROC_BROWSER_TEST_F(VisualStateTest, CallbackDoesNotDeadlockPart2) {
+  // Load kBlankUrl and wait for its commit.
+  GURL url(kBlankUrl);
+  NavigateToURL(shell(), url);
+  PostTaskToInProcessRendererAndWait(
+      base::Bind(&VisualStateTest::WaitForOneCommit, base::Unretained(this)));
+
+  // Verify that a second commit does not happen. Please note that in part 1
+  // the second commit happens within kCommitTimeout and here we wait for that
+  // long so we get a high level of confidence that this second commit will not
+  // happen (and manual tests in which we waited for much longer confirmed that
+  // it never happens)
+  PostTaskToInProcessRendererAndWait(
+      base::Bind(&VisualStateTest::WaitForZeroCommits, base::Unretained(this)));
+  EXPECT_EQ(0, GetCallbackCount());
+}
+
+}  // namespace content