Ensure that the plugin HWND doesn't disappear before the plugin gets NPP_SetWindow with a null HWND.

BUG=23694
TEST=tried really hard to write an automated test but couldn't since it's a race condition that repros this (i.e. IO thread is busy and windows message gets dispatched on the UI thread first)
Review URL: http://codereview.chromium.org/268012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@28418 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 25 insertions, 70 deletions

diff --git a/chrome/browser/renderer_host/render_widget_host_view_win.cc b/chrome/browser/renderer_host/render_widget_host_view_win.cc
index d180419..65c4a0f 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_win.cc
+++ b/chrome/browser/renderer_host/render_widget_host_view_win.cc
@@ -187,6 +187,16 @@ class NotifyPluginProcessHostTask : public Task {
   HWND parent_;  // Parent HWND, created and destroyed on the browser UI thread.
 };
 
+// Windows callback for OnDestroy to detach the plugin windows.
+BOOL CALLBACK DetachPluginWindowsCallback(HWND window, LPARAM param) {
+  if (WebPluginDelegateImpl::IsPluginDelegateWindow(window) &&
+      !IsHungAppWindow(window)) {
+    ::ShowWindow(window, SW_HIDE);
+    SetParent(window, NULL);
+  }
+  return TRUE;
+}
+
 }  // namespace
 
 // RenderWidgetHostView --------------------------------------------------------
@@ -689,6 +699,21 @@ void RenderWidgetHostViewWin::OnActivate(UINT action, BOOL minimized,
 }
 
 void RenderWidgetHostViewWin::OnDestroy() {
+  // When a tab is closed all its child plugin windows are destroyed
+  // automatically. This happens before plugins get any notification that its
+  // instances are tearing down.
+  //
+  // Plugins like Quicktime assume that their windows will remain valid as long
+  // as they have plugin instances active. Quicktime crashes in this case
+  // because its windowing code cleans up an internal data structure that the
+  // handler for NPP_DestroyStream relies on.
+  //
+  // The fix is to detach plugin windows from web contents when it is going
+  // away. This will prevent the plugin windows from getting destroyed
+  // automatically. The detached plugin windows will get cleaned up in proper
+  // sequence as part of the usual cleanup when the plugin instance goes away.
+  EnumChildWindows(m_hWnd, DetachPluginWindowsCallback, NULL);
+
   ResetTooltip();
   TrackMouseLeave(false);
 }
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index 325e6bc..4279332 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -326,9 +326,6 @@ TabContents::~TabContents() {
       prefs->RemovePrefObserver(kPrefsToObserve[i], this);
   }
 
-  // Clean up subwindows like plugins and the find in page bar.
-  view_->OnContentsDestroy();
-
   NotifyDisconnected();
   HungRendererDialog::HideForTabContents(this);
 
diff --git a/chrome/browser/tab_contents/tab_contents_view.h b/chrome/browser/tab_contents/tab_contents_view.h
index 978b086..bc76ae8 100644
--- a/chrome/browser/tab_contents/tab_contents_view.h
+++ b/chrome/browser/tab_contents/tab_contents_view.h
@@ -75,14 +75,6 @@ class TabContentsView : public RenderViewHostDelegate::View {
     return gfx::Size(rc.width(), rc.height());
   }
 
-  // Called when the TabContents is being destroyed. This should clean up child
-  // windows that are part of the view.
-  //
-  // TODO(brettw) It seems like this might be able to be done internally as the
-  // window is being torn down without input from the TabContents. Try to
-  // implement functions that way rather than adding stuff here.
-  virtual void OnContentsDestroy() = 0;
-
   // Sets the page title for the native widgets corresponding to the view. This
   // is not strictly necessary and isn't expected to be displayed anywhere, but
   // can aid certain debugging tools such as Spy++ on Windows where you are
diff --git a/chrome/browser/tab_contents/tab_contents_view_gtk.cc b/chrome/browser/tab_contents/tab_contents_view_gtk.cc
index f5adeec..7a70a28 100644
--- a/chrome/browser/tab_contents/tab_contents_view_gtk.cc
+++ b/chrome/browser/tab_contents/tab_contents_view_gtk.cc
@@ -480,11 +480,6 @@ void TabContentsViewGtk::GetContainerBounds(gfx::Rect* out) const {
                requested_size_.width(), requested_size_.height());
 }
 
-void TabContentsViewGtk::OnContentsDestroy() {
-  // We don't want to try to handle drag events from this point on.
-  drag_source_.reset();
-}
-
 void TabContentsViewGtk::SetPageTitle(const std::wstring& title) {
   // Set the window name to include the page title so it's easier to spot
   // when debugging (e.g. via xwininfo -tree).
diff --git a/chrome/browser/tab_contents/tab_contents_view_gtk.h b/chrome/browser/tab_contents/tab_contents_view_gtk.h
index 3e777b8..8552543 100644
--- a/chrome/browser/tab_contents/tab_contents_view_gtk.h
+++ b/chrome/browser/tab_contents/tab_contents_view_gtk.h
@@ -55,7 +55,6 @@ class TabContentsViewGtk : public TabContentsView,
   virtual gfx::NativeWindow GetTopLevelNativeWindow() const;
   virtual void InitRendererPrefs(RendererPreferences* prefs);
   virtual void GetContainerBounds(gfx::Rect* out) const;
-  virtual void OnContentsDestroy();
   virtual void SetPageTitle(const std::wstring& title);
   virtual void OnTabCrashed();
   virtual void SizeContents(const gfx::Size& size);
diff --git a/chrome/browser/tab_contents/tab_contents_view_mac.h b/chrome/browser/tab_contents/tab_contents_view_mac.h
index a7ee015..79c7fe4 100644
--- a/chrome/browser/tab_contents/tab_contents_view_mac.h
+++ b/chrome/browser/tab_contents/tab_contents_view_mac.h
@@ -54,7 +54,6 @@ class TabContentsViewMac : public TabContentsView,
   virtual gfx::NativeView GetContentNativeView() const;
   virtual gfx::NativeWindow GetTopLevelNativeWindow() const;
   virtual void GetContainerBounds(gfx::Rect* out) const;
-  virtual void OnContentsDestroy();
   virtual void RenderViewCreated(RenderViewHost* host);
   virtual void SetPageTitle(const std::wstring& title);
   virtual void OnTabCrashed();
diff --git a/chrome/browser/tab_contents/tab_contents_view_mac.mm b/chrome/browser/tab_contents/tab_contents_view_mac.mm
index c560fb6..5a9638c 100644
--- a/chrome/browser/tab_contents/tab_contents_view_mac.mm
+++ b/chrome/browser/tab_contents/tab_contents_view_mac.mm
@@ -112,9 +112,6 @@ void TabContentsViewMac::StartDragging(const WebDropData& drop_data,
   MessageLoop::current()->SetNestableTasksAllowed(false);
 }
 
-void TabContentsViewMac::OnContentsDestroy() {
-}
-
 void TabContentsViewMac::RenderViewCreated(RenderViewHost* host) {
   // We want updates whenever the intrinsic width of the webpage
   // changes. Put the RenderView into that mode.
diff --git a/chrome/browser/views/tab_contents/tab_contents_view_gtk.cc b/chrome/browser/views/tab_contents/tab_contents_view_gtk.cc
index 7022d5f..665a63d 100644
--- a/chrome/browser/views/tab_contents/tab_contents_view_gtk.cc
+++ b/chrome/browser/views/tab_contents/tab_contents_view_gtk.cc
@@ -181,16 +181,6 @@ void TabContentsViewGtk::StartDragging(const WebDropData& drop_data,
   // TODO(snej): Make use of WebDragOperationsMask
 }
 
-void TabContentsViewGtk::OnContentsDestroy() {
-  // TODO(brettw) this seems like maybe it can be moved into OnDestroy and this
-  // function can be deleted? If you're adding more here, consider whether it
-  // can be moved into OnDestroy which is a Windows message handler as the
-  // window is being torn down.
-
-  // We don't want to try to handle drag events from this point on.
-  drag_source_.reset();
-}
-
 void TabContentsViewGtk::SetPageTitle(const std::wstring& title) {
   // Set the window name to include the page title so it's easier to spot
   // when debugging (e.g. via xwininfo -tree).
diff --git a/chrome/browser/views/tab_contents/tab_contents_view_gtk.h b/chrome/browser/views/tab_contents/tab_contents_view_gtk.h
index 661b240..324846e 100644
--- a/chrome/browser/views/tab_contents/tab_contents_view_gtk.h
+++ b/chrome/browser/views/tab_contents/tab_contents_view_gtk.h
@@ -38,7 +38,6 @@ class TabContentsViewGtk : public TabContentsView,
   virtual gfx::NativeView GetContentNativeView() const;
   virtual gfx::NativeWindow GetTopLevelNativeWindow() const;
   virtual void GetContainerBounds(gfx::Rect* out) const;
-  virtual void OnContentsDestroy();
   virtual void SetPageTitle(const std::wstring& title);
   virtual void OnTabCrashed();
   virtual void SizeContents(const gfx::Size& size);
diff --git a/chrome/browser/views/tab_contents/tab_contents_view_win.cc b/chrome/browser/views/tab_contents/tab_contents_view_win.cc
index 7352951..ba61d9b 100644
--- a/chrome/browser/views/tab_contents/tab_contents_view_win.cc
+++ b/chrome/browser/views/tab_contents/tab_contents_view_win.cc
@@ -31,7 +31,6 @@
 #include "net/base/net_util.h"
 #include "views/focus/view_storage.h"
 #include "views/widget/root_view.h"
-#include "webkit/glue/plugins/webplugin_delegate_impl.h"
 #include "webkit/glue/webdropdata.h"
 
 using WebKit::WebDragOperation;
@@ -39,20 +38,6 @@ using WebKit::WebDragOperationNone;
 using WebKit::WebDragOperationsMask;
 using WebKit::WebInputEvent;
 
-namespace {
-
-// Windows callback for OnDestroy to detach the plugin windows.
-BOOL CALLBACK DetachPluginWindowsCallback(HWND window, LPARAM param) {
-  if (WebPluginDelegateImpl::IsPluginDelegateWindow(window) &&
-      !IsHungAppWindow(window)) {
-    ::ShowWindow(window, SW_HIDE);
-    SetParent(window, NULL);
-  }
-  return TRUE;
-}
-
-}  // namespace
-
 // static
 TabContentsView* TabContentsView::Create(TabContents* tab_contents) {
   return new TabContentsViewWin(tab_contents);
@@ -208,28 +193,6 @@ void TabContentsViewWin::StartDragging(const WebDropData& drop_data,
     tab_contents()->render_view_host()->DragSourceSystemDragEnded();
 }
 
-void TabContentsViewWin::OnContentsDestroy() {
-  // TODO(brettw) this seems like maybe it can be moved into OnDestroy and this
-  // function can be deleted? If you're adding more here, consider whether it
-  // can be moved into OnDestroy which is a Windows message handler as the
-  // window is being torn down.
-
-  // When a tab is closed all its child plugin windows are destroyed
-  // automatically. This happens before plugins get any notification that its
-  // instances are tearing down.
-  //
-  // Plugins like Quicktime assume that their windows will remain valid as long
-  // as they have plugin instances active. Quicktime crashes in this case
-  // because its windowing code cleans up an internal data structure that the
-  // handler for NPP_DestroyStream relies on.
-  //
-  // The fix is to detach plugin windows from web contents when it is going
-  // away. This will prevent the plugin windows from getting destroyed
-  // automatically. The detached plugin windows will get cleaned up in proper
-  // sequence as part of the usual cleanup when the plugin instance goes away.
-  EnumChildWindows(GetNativeView(), DetachPluginWindowsCallback, NULL);
-}
-
 void TabContentsViewWin::OnDestroy() {
   if (drop_target_.get()) {
     RevokeDragDrop(GetNativeView());
diff --git a/chrome/browser/views/tab_contents/tab_contents_view_win.h b/chrome/browser/views/tab_contents/tab_contents_view_win.h
index cf52294..b7ed158 100644
--- a/chrome/browser/views/tab_contents/tab_contents_view_win.h
+++ b/chrome/browser/views/tab_contents/tab_contents_view_win.h
@@ -41,7 +41,6 @@ class TabContentsViewWin : public TabContentsView,
   virtual gfx::NativeView GetContentNativeView() const;
   virtual gfx::NativeWindow GetTopLevelNativeWindow() const;
   virtual void GetContainerBounds(gfx::Rect* out) const;
-  virtual void OnContentsDestroy();
   virtual void SetPageTitle(const std::wstring& title);
   virtual void OnTabCrashed();
   virtual void SizeContents(const gfx::Size& size);