Fix a crash which occurs while dragging tabs back to the window where they originated from.

Reason for the crash is the recent change to the WebView::AttachWebContents function to show the
WebContentsView when the WCV's root window is NULL. This can happen if the tab was detached and
is being reattched in which case the clipping window is removed, etc.

The actual crash occurs in the RWHVA::WasShown function where we dereference a NULL host pointer
due to a NULL root window.

Fix is to show the WCV after the attach has happened in the holder. This works correctly and is required for the Legacy window to show up correctly.
I verified that WC::Show and WC::Hide only get called once and the same is true with RWHVA::WasShown/WasHidden.

I updated the TestWebViewAttachDetachWebContents webview unittest to check for a NULL root window
when WC::WasShown is fired and we test for the same.

BUG=415509

Review URL: https://codereview.chromium.org/584743004

Cr-Commit-Position: refs/heads/master@{#296072}

ui/views/controls/webview/webview.cc

@@ -303,13 +303,14 @@ void WebView::AttachWebContents() {
   if (holder_->native_view() == view_to_attach)
     return;
 
+  // The WCV needs to be parented before making it visible.
+  holder_->Attach(view_to_attach);
+
   // Fullscreen widgets are not parented by a WebContentsView. Their visibility
   // is controlled by content i.e. (RenderWidgetHost)
   if (!is_embedding_fullscreen_widget_)
     view_to_attach->Show();
 
-  holder_->Attach(view_to_attach);
-
   // The view will not be focused automatically when it is attached, so we need
   // to pass on focus to it if the FocusManager thinks the view is focused. Note
   // that not every Widget has a focus manager.

ui/views/controls/webview/webview_unittest.cc

@@ -102,6 +102,8 @@ class WebViewTestWebContentsObserver : public content::WebContentsObserver {
   }
 
   virtual void WasShown() OVERRIDE {
+    valid_root_while_shown_ =
+        web_contents()->GetNativeView()->GetRootWindow() != NULL;
     was_shown_ = true;
     ++shown_count_;
   }
@@ -117,11 +119,15 @@ class WebViewTestWebContentsObserver : public content::WebContentsObserver {
 
   int hidden_count() const { return hidden_count_; }
 
+  bool valid_root_while_shown() const { return valid_root_while_shown_; }
+
  private:
   content::WebContentsImpl* web_contents_;
   bool was_shown_;
   int32 shown_count_;
   int32 hidden_count_;
+  // Set to true if the view containing the webcontents has a valid root window.
+  bool valid_root_while_shown_;
 
   DISALLOW_COPY_AND_ASSIGN(WebViewTestWebContentsObserver);
 };
@@ -149,6 +155,7 @@ TEST_F(WebViewUnitTest, TestWebViewAttachDetachWebContents) {
   EXPECT_TRUE(web_contents1->GetNativeView()->IsVisible());
   EXPECT_EQ(observer1.shown_count(), 1);
   EXPECT_EQ(observer1.hidden_count(), 0);
+  EXPECT_TRUE(observer1.valid_root_while_shown());
 
   // Case 2: Create another WebContents and replace the current WebContents
   // via SetWebContents(). This should hide the current WebContents and show
@@ -156,6 +163,7 @@ TEST_F(WebViewUnitTest, TestWebViewAttachDetachWebContents) {
   content::WebContents::CreateParams params2(browser_context());
   scoped_ptr<content::WebContents> web_contents2(
       content::WebContents::Create(params2));
+
   WebViewTestWebContentsObserver observer2(web_contents2.get());
   EXPECT_FALSE(observer2.was_shown());
 
@@ -163,6 +171,7 @@ TEST_F(WebViewUnitTest, TestWebViewAttachDetachWebContents) {
   webview->SetWebContents(web_contents2.get());
   EXPECT_FALSE(observer1.was_shown());
   EXPECT_TRUE(observer2.was_shown());
+  EXPECT_TRUE(observer2.valid_root_while_shown());
 
   // WebContents1 should not get stray show calls when WebContents2 is set.
   EXPECT_EQ(observer1.shown_count(), 1);