Extending "ChromeSitePerProcessPDFTest.EmbeddedPDFInsideCrossOriginFrame" to verify guest destructi…

chrome/browser/chrome_site_per_process_browsertest.cc

Index: chrome/browser/chrome_site_per_process_browsertest.cc
diff --git a/chrome/browser/chrome_site_per_process_browsertest.cc b/chrome/browser/chrome_site_per_process_browsertest.cc
index 788eb657d69eaae9da460526a4825f8bf4612d8e..b2176640bca8f9fd5c8fad0576f092b037a54dea 100644
--- a/chrome/browser/chrome_site_per_process_browsertest.cc
+++ b/chrome/browser/chrome_site_per_process_browsertest.cc
@@ -320,6 +320,34 @@ class ChromeSitePerProcessPDFTest : public ChromeSitePerProcessTest {
   DISALLOW_COPY_AND_ASSIGN(ChromeSitePerProcessPDFTest);
 };
 
+// Helper class to wait until a given WebContents is destroyed.
+class WebContentsDestructionObserver : content::WebContentsObserver {

[alexmos, 2016/11/23 00:23:51]

There's a WebContentsDestroyedWatcher in content/public/test/test_utils.h.  Is
it possible to use that one instead of a new class?

[EhsanK, 2016/11/23 00:51:39]

Yes indeed. Thanks for the suggestion.

+ public:
+  explicit WebContentsDestructionObserver(content::WebContents* web_contents)
+      : content::WebContentsObserver(web_contents),
+        web_contents_destroyed_(false) {}
+  ~WebContentsDestructionObserver() override {}
+
+  void Wait() {
+    if (web_contents_destroyed_)
+      return;
+    message_loop_runner_ = new content::MessageLoopRunner();
+    message_loop_runner_->Run();
+  }
+
+ private:
+  void WebContentsDestroyed() override {
+    web_contents_destroyed_ = true;
+    if (message_loop_runner_)
+      message_loop_runner_->Quit();
+  }
+
+  bool web_contents_destroyed_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(WebContentsDestructionObserver);
+};
+
 // TODO(ekaramad): This test is flaky on Windows 7. Enable it when the issue is
 // fixed ((https://crbug.com/666379).
 #if defined(OS_WIN)
@@ -330,7 +358,8 @@ class ChromeSitePerProcessPDFTest : public ChromeSitePerProcessTest {
   EmbeddedPDFInsideCrossOriginFrame
 #endif
 // This test verifies that when navigating an OOPIF to a page with <embed>-ed
-// PDF, the guest is properly created (https://crbug.com/649856).
+// PDF, the guest is properly created, and by removing the embedder frame, the
+// guest is properly destroyed (https://crbug.com/649856).
 IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessPDFTest,
                        MAYBE_EmbeddedPDFInsideCrossOriginFrame) {
   // Navigate to a page with an <iframe>.
@@ -351,4 +380,12 @@ IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessPDFTest,
 
   // Wait until the guest for PDF is created.
   test_guest_view_manager()->WaitForSingleGuestCreated();

[alexmos, 2016/11/23 00:23:51]

Can you save the result of this and use it below, instead of calling
GetLastGuestCreated again?

[EhsanK, 2016/11/23 00:51:39]

Acknowledged.

+
+  // Now detach the frame and observe that the guest is destroyed.
+  WebContentsDestructionObserver observer(
+      test_guest_view_manager()->GetLastGuestCreated());
+  EXPECT_TRUE(ExecuteScript(
+      active_web_contents,
+      "document.body.removeChild(document.querySelector('iframe'));"));
+  observer.Wait();

[alexmos, 2016/11/23 00:23:51]

Would it be useful to also check that GetNumGuestsActive() is 0 after this
point?

[EhsanK, 2016/11/23 00:51:39]

Acknowledged.

 }