Fix SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame on CFI Bots.

chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc

 // Copyright 2016 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 <vector>
 
 #include "base/command_line.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ skipping 54 matching lines @@
 
   bool success() const { return success_; }
 
  protected:
   content::TextInputManagerTester* tester() { return tester_.get(); }
 
   void OnSuccess() {
     success_ = true;
     if (message_loop_runner_)
       message_loop_runner_->Quit();
+
+    // By deleting |tester_| we make sure that the internal observer used in
+    // content/ is removed from the observer list of TextInputManager.
+    tester_.reset(nullptr);

[EhsanK, 2016/06/25 00:10:35]

This is the public API's helper, which owns an internal observer. Since we add
callbacks for observing to make it general purpose.

Destroying it here makes sense because none of the observer helper classes here
are intended to be called twice on their Wait() method.

This was causing CFI errors because some of the observers in the test were still
observing the TextInputManager after their view was destroyed. Sepcifically, I
believe the major one is the one on line 310 (ViewTextInputTypeObserver) which
was causing an issue on line 175. 

After we killed frames[0] in the test, we were still observing the
TextInputManager using the observer instance in line 310. When the second view
is killed, we get an observer call and the using the |view_| inside the observer
which is now dangling.

[Charlie Reis, 2016/06/28 18:12:41]

Acknowledged.

   }
 
  private:
   std::unique_ptr<content::TextInputManagerTester> tester_;
   bool success_;
   scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(TextInputManagerObserverBase);
 };
 
@@ skipping 231 matching lines @@
   // Make sure that the first view has set its TextInputState.type to NONE.
   view_type_observer.Wait();
 
   // Verify that we are tracking the TextInputState from the first frame.
   content::RenderWidgetHostView* first_view = frames[0]->GetView();
   ui::TextInputType first_view_type;
   EXPECT_TRUE(content::GetTextInputTypeForView(active_contents(), first_view,
                                                &first_view_type));
   EXPECT_EQ(ui::TEXT_INPUT_TYPE_NONE, first_view_type);
 
+  size_t registered_views_count =
+      content::GetRegisteredViewsCountFromTextInputManager(active_contents());
+
+  DCHECK_LT(2U, registered_views_count);

[Charlie Reis, 2016/06/28 18:12:41]

This is probably easier to read as:

// There are at least 2 views at this point, possibly more.
DCHECK_GT(registered_views_count, 2U);

[EhsanK, 2016/06/28 18:56:53]

Acknowledged.

+
   // Now that the second frame's <input> is focused, we crash the first frame
   // and observe that text input state is updated for the view.
   std::unique_ptr<content::TestRenderWidgetHostViewDestructionObserver>
       destruction_observer(
           new content::TestRenderWidgetHostViewDestructionObserver(first_view));
   frames[0]->GetProcess()->Shutdown(0, false);
   destruction_observer->Wait();
 
   // Verifying that the TextInputManager is no longer tracking TextInputState

[Charlie Reis, 2016/06/28 18:12:41]

nit: Verify

[EhsanK, 2016/06/28 18:56:53]

Done.

-  // for |first_view|. Note that |first_view| is now a dangling pointer.
-  EXPECT_FALSE(content::GetTextInputTypeForView(active_contents(), first_view,
-                                                &first_view_type));
+  // for |first_view|.
+  EXPECT_EQ(
+      registered_views_count - 1U,
+      content::GetRegisteredViewsCountFromTextInputManager(active_contents()));
+  // Now crash the second <iframe> which has an active view.

[Charlie Reis, 2016/06/28 18:12:41]

nit: Add blank line before.

[EhsanK, 2016/06/28 18:56:53]

Done.

+  destruction_observer.reset(
+      new content::TestRenderWidgetHostViewDestructionObserver(
+          frames[1]->GetView()));
+  frames[1]->GetProcess()->Shutdown(0, false);
+  destruction_observer->Wait();
 
-  // Now crash the second <iframe> which has an active view.
-  content::RenderWidgetHostView* second_view = frames[1]->GetView();
-  TextInputManagerChangeObserver change_observer(active_contents());
-  frames[1]->GetProcess()->Shutdown(0, false);
-  change_observer.Wait();
-  EXPECT_EQ(ui::TEXT_INPUT_TYPE_NONE,
-            content::GetTextInputTypeFromWebContents(active_contents()));
-  EXPECT_FALSE(!!content::GetActiveViewFromWebContents(active_contents()));
-  ui::TextInputType second_view_type;
-  EXPECT_FALSE(content::GetTextInputTypeForView(active_contents(), second_view,
-                                                &second_view_type));
+  EXPECT_EQ(
+      registered_views_count - 2U,
+      content::GetRegisteredViewsCountFromTextInputManager(active_contents()));
 }
 
 // The following test loads a page with two child frames: one in process and one
 // out of process with main frame. The test inserts an <input> inside each frame
 // and focuses the first frame and observes the TextInputManager setting the
 // state to ui::TEXT_INPUT_TYPE_TEXT. Then, the frame is detached and the test
 // observes that the state type is reset to ui::TEXT_INPUT_TYPE_NONE. The same
 // sequence of actions is then performed on the out of process frame.
 IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
                        ResetStateAfterFrameDetached) {
@@ skipping 188 matching lines @@
   EXPECT_FALSE(send_and_check_show_ime());
 
   // Set |TextInputState.show_ime_if_needed|. Expect IME.
   sender.SetShowImeIfNeeded(true);
   EXPECT_TRUE(send_and_check_show_ime());
 
   // Set |TextInputState.type| to ui::TEXT_INPUT_TYPE_NONE. Expect no IME.
   sender.SetType(ui::TEXT_INPUT_TYPE_NONE);
   EXPECT_FALSE(send_and_check_show_ime());
 }