This fixes http://code.google.com/p/chromium/issues/detail?id=205, which was an issue with a windowed flash instance not rendering content at times.The bug occurs as a result of the following:-1. The flash plugin executes a script via GetURLNotify. This script calls window.open with the target as self, which shows up as a new tab in the browser. This causes a new RenderView object to be instantiated (See RenderView::CreateWebView).2. RenderView::CreateWebView sends over the ViewHostMsg_CreateWindow IPC message to the browser. The handler in the browser sends over an ack for this message with the window handle. This is used as the parent window for any plugins instantiated in the page.3. At times, the newly created view starts receiving data which is processed before the ViewMsg_CreatingNew_ACK message is received and processed by the view. This causes the plugin to be instantiated without a parent window thus ending up as a top level window.The fix is to queue up resource messages and process them after we receive the ack for the ViewHostMsg_CreateWindow IPC.

Tests :- Covered by UI tests.

R=jam


Review URL: http://codereview.chromium.org/7514

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

7 files changed, 121 insertions, 10 deletions

diff --git a/chrome/common/resource_dispatcher.cc b/chrome/common/resource_dispatcher.cc
index e3099af..81ed5a2 100644
--- a/chrome/common/resource_dispatcher.cc
+++ b/chrome/common/resource_dispatcher.cc
@@ -244,15 +244,8 @@ ResourceDispatcher::~ResourceDispatcher() {
 // ResourceDispatcher implementation ------------------------------------------
 
 bool ResourceDispatcher::OnMessageReceived(const IPC::Message& message) {
-  switch (message.type()) {
-    case ViewMsg_Resource_UploadProgress::ID:
-    case ViewMsg_Resource_ReceivedResponse::ID:
-    case ViewMsg_Resource_ReceivedRedirect::ID:
-    case ViewMsg_Resource_DataReceived::ID:
-    case ViewMsg_Resource_RequestComplete::ID:
-      break;
-    default:
-      return false;
+  if (!IsResourceMessage(message)) {
+    return false;
   }
 
   int request_id;
@@ -508,3 +501,19 @@ webkit_glue::ResourceLoaderBridge* ResourceDispatcher::CreateBridge(
                                                   request_context);
 }
 
+
+bool ResourceDispatcher::IsResourceMessage(const IPC::Message& message) const {
+  switch (message.type()) {
+    case ViewMsg_Resource_UploadProgress::ID:
+    case ViewMsg_Resource_ReceivedResponse::ID:
+    case ViewMsg_Resource_ReceivedRedirect::ID:
+    case ViewMsg_Resource_DataReceived::ID:
+    case ViewMsg_Resource_RequestComplete::ID:
+      return true;
+
+    default:
+      break;
+  }
+
+  return false;
+}
\ No newline at end of file
diff --git a/chrome/common/resource_dispatcher.h b/chrome/common/resource_dispatcher.h
index 984aed2..b4c4037 100644
--- a/chrome/common/resource_dispatcher.h
+++ b/chrome/common/resource_dispatcher.h
@@ -68,6 +68,10 @@ class ResourceDispatcher : public base::RefCounted<ResourceDispatcher> {
     message_sender_ = NULL;
   }
 
+  // Returns true if the message passed in is a resource related
+  // message.
+  bool IsResourceMessage(const IPC::Message& message) const;
+
  private:
   friend class ResourceDispatcherTest;
 
diff --git a/chrome/renderer/render_view.cc b/chrome/renderer/render_view.cc
index 253a869..f6c19d7 100644
--- a/chrome/renderer/render_view.cc
+++ b/chrome/renderer/render_view.cc
@@ -156,7 +156,8 @@ RenderView::RenderView()
     disable_popup_blocking_(false),
     has_unload_listener_(false),
     decrement_shared_popup_at_destruction_(false),
-    greasemonkey_enabled_(false) {
+    greasemonkey_enabled_(false),
+    waiting_for_create_window_ack_(false) {
   resource_dispatcher_ = new ResourceDispatcher(this);
 #ifdef CHROME_PERSONALIZATION
   personalization_ = Personalization::CreateRendererPersonalization();
@@ -291,9 +292,22 @@ void RenderView::Init(HWND parent_hwnd,
 }
 
 void RenderView::OnMessageReceived(const IPC::Message& message) {
+  // If the current RenderView instance represents a popup, then we
+  // need to wait for ViewMsg_CreatingNew_ACK to be sent by the browser.
+  // As part of this ack we also receive the browser window handle, which
+  // parents any plugins instantiated in this RenderView instance.
+  // Plugins can be instantiated only when we receive the parent window
+  // handle as they are child windows.
+  if (waiting_for_create_window_ack_ && 
+      resource_dispatcher_->IsResourceMessage(message)) {
+    queued_resource_messages_.push(new IPC::Message(message));
+    return;
+  }
+
   // Let the resource dispatcher intercept resource messages first.
   if (resource_dispatcher_->OnMessageReceived(message))
     return;
+
   IPC_BEGIN_MESSAGE_MAP(RenderView, message)
     IPC_MESSAGE_HANDLER(ViewMsg_CreatingNew_ACK, OnCreatingNewAck)
     IPC_MESSAGE_HANDLER(ViewMsg_CaptureThumbnail, SendThumbnail)
@@ -375,6 +389,15 @@ void RenderView::OnMessageReceived(const IPC::Message& message) {
 // view.
 void RenderView::OnCreatingNewAck(HWND parent) {
   CompleteInit(parent);
+
+  waiting_for_create_window_ack_ = false;
+
+  while (!queued_resource_messages_.empty()) {
+    IPC::Message* queued_msg = queued_resource_messages_.front();
+    queued_resource_messages_.pop();
+    resource_dispatcher_->OnMessageReceived(*queued_msg);
+    delete queued_msg;
+  }
 }
 
 void RenderView::SendThumbnail() {
@@ -1715,6 +1738,7 @@ WebView* RenderView::CreateWebView(WebView* webview, bool user_gesture) {
                                         prefs, shared_popup_counter_,
                                         routing_id);
   view->set_opened_by_user_gesture(user_gesture);
+  view->set_waiting_for_create_window_ack(true);
 
   // Copy over the alternate error page URL so we can have alt error pages in
   // the new render view (we don't need the browser to send the URL back down).
diff --git a/chrome/renderer/render_view.h b/chrome/renderer/render_view.h
index cba9df9..9c50dfe 100644
--- a/chrome/renderer/render_view.h
+++ b/chrome/renderer/render_view.h
@@ -512,6 +512,10 @@ class RenderView : public RenderWidget, public WebViewDelegate,
   // A helper method used by WasOpenedByUserGesture.
   bool WasOpenedByUserGestureHelper() const;
 
+  void set_waiting_for_create_window_ack(bool wait) {
+    waiting_for_create_window_ack_ = wait;
+  }
+
   // Handles resource loads for this view.
   scoped_refptr<ResourceDispatcher> resource_dispatcher_;
 
@@ -658,6 +662,13 @@ class RenderView : public RenderWidget, public WebViewDelegate,
   // True if Greasemonkey is enabled in this process.
   bool greasemonkey_enabled_;
 
+  // Resource message queue. Used to queue up resource IPCs if we need
+  // to wait for an ACK from the browser before proceeding.
+  std::queue<IPC::Message*> queued_resource_messages_;
+
+  // Set if we are waiting for an ack for ViewHostMsg_CreateWindow
+  bool waiting_for_create_window_ack_;
+
   DISALLOW_COPY_AND_ASSIGN(RenderView);
 };
 
diff --git a/chrome/test/data/npapi/get_javascript_open_popup_with_plugin.html b/chrome/test/data/npapi/get_javascript_open_popup_with_plugin.html
new file mode 100644
index 0000000..8f2f01a
--- /dev/null
+++ b/chrome/test/data/npapi/get_javascript_open_popup_with_plugin.html
@@ -0,0 +1,27 @@
+<html>
+
+<head>
+<script src="npapi.js"></script>
+</head>
+
+
+<body>
+<div id="statusPanel" style="border: 1px solid red; width: 100%">
+Test running....
+</div>
+
+
+Open Popup window with plugin Test<p>
+This test instantiates a plugin which executes the window.open call to open a popup <br />
+window with a windowed plugin instance. The test verifies that the plugin instance in <br />
+the popup window always has a valid parent window. <br />
+
+<embed type="application/vnd.npapi-test" 
+	src="foo"
+	name="plugin_javascript_open_popup_with_plugin"
+	id="1"
+	mode="np_embed"
+>
+
+</body>
+</html>
diff --git a/chrome/test/data/npapi/popup_window_with_target_plugin.html b/chrome/test/data/npapi/popup_window_with_target_plugin.html
new file mode 100644
index 0000000..5f203bd
--- /dev/null
+++ b/chrome/test/data/npapi/popup_window_with_target_plugin.html
@@ -0,0 +1,27 @@
+<html>
+
+<head>
+<script src="npapi.js"></script>
+</head>
+
+
+<body>
+<div id="statusPanel" style="border: 1px solid red; width: 100%">
+Test running....
+</div>
+
+
+Open Popup window with plugin Test<p>
+This test instantiates a plugin which executes the window.open call to open a popup <br />
+window with a windowed plugin instance. The test verifies that the plugin instance in <br />
+the popup window always has a valid parent window.<br />
+
+<embed type="application/vnd.npapi-test" 
+	src="foo"
+	name="plugin_popup_with_plugin_target"
+	id="1"
+	mode="np_embed"
+>
+
+</body>
+</html>
diff --git a/chrome/test/ui/npapi_uitest.cpp b/chrome/test/ui/npapi_uitest.cpp
index 525e48e..bce1497 100644
--- a/chrome/test/ui/npapi_uitest.cpp
+++ b/chrome/test/ui/npapi_uitest.cpp
@@ -270,4 +270,13 @@ TEST_F(NPAPIVisiblePluginTester, SelfDeletePluginInNPNEvaluate) {
                   kTestCompleteCookie, kTestCompleteSuccess,
                   kShortWaitTimeout);
  }
+}
+
+TEST_F(NPAPIVisiblePluginTester, OpenPopupWindowWithPlugin) {
+  GURL url = GetTestUrl(L"npapi",
+                        L"get_javascript_open_popup_with_plugin.html");
+  NavigateToURL(url);
+  WaitForFinish("plugin_popup_with_plugin_target", "1", url,
+                kTestCompleteCookie, kTestCompleteSuccess,
+                kShortWaitTimeout);
 }
\ No newline at end of file