Fix scripting during NPP_Destroy. Note that if the plugin is making a call to the renderer so this instance is in the callstack, destruction will have to be asynchronous and so scripting still won't work. This change also fixes use of PluginChannel after it's deleted (if this was the last instance).

BUG=23713, 23706
TEST=added ui test
Review URL: http://codereview.chromium.org/258026

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

10 files changed, 164 insertions, 98 deletions

diff --git a/chrome/plugin/npobject_stub.cc b/chrome/plugin/npobject_stub.cc
index cc59bec..9f07211 100644
--- a/chrome/plugin/npobject_stub.cc
+++ b/chrome/plugin/npobject_stub.cc
@@ -9,7 +9,6 @@
 #include "chrome/plugin/npobject_util.h"
 #include "chrome/plugin/plugin_channel_base.h"
 #include "chrome/plugin/plugin_thread.h"
-#include "chrome/renderer/webplugin_delegate_proxy.h"
 #include "third_party/npapi/bindings/npapi.h"
 #include "third_party/npapi/bindings/npruntime.h"
 #include "webkit/api/public/WebBindings.h"
@@ -26,8 +25,6 @@ NPObjectStub::NPObjectStub(
     : npobject_(npobject),
       channel_(channel),
       route_id_(route_id),
-      valid_(true),
-      web_plugin_delegate_proxy_(NULL),
       containing_window_(containing_window),
       page_url_(page_url) {
   channel_->AddRoute(route_id, this, true);
@@ -37,11 +34,8 @@ NPObjectStub::NPObjectStub(
 }
 
 NPObjectStub::~NPObjectStub() {
-  if (web_plugin_delegate_proxy_)
-    web_plugin_delegate_proxy_->DropWindowScriptObject();
-
   channel_->RemoveRoute(route_id_);
-  if (npobject_ && valid_)
+  if (npobject_)
     WebBindings::releaseObject(npobject_);
 }
 
@@ -49,10 +43,21 @@ bool NPObjectStub::Send(IPC::Message* msg) {
   return channel_->Send(msg);
 }
 
+void NPObjectStub::OnPluginDestroyed() {
+  // We null out the underlying NPObject pointer since it's not valid anymore (
+  // ScriptController manually deleted the object).  As a result,
+  // OnMessageReceived won't dispatch any more messages.  Since this includes
+  // OnRelease, this object won't get deleted until OnChannelError which might
+  // not happen for a long time if this renderer process has a long lived
+  // plugin instance to the same process.  So we delete this object manually.
+  npobject_ = NULL;
+  MessageLoop::current()->DeleteSoon(FROM_HERE, this);
+}
+
 void NPObjectStub::OnMessageReceived(const IPC::Message& msg) {
   child_process_logging::ScopedActiveURLSetter url_setter(page_url_);
 
-  if (!valid_) {
+  if (!npobject_) {
     if (msg.is_sync()) {
       // The object could be garbage because the frame has gone away, so
       // just send an error reply to the caller.
@@ -82,10 +87,6 @@ void NPObjectStub::OnMessageReceived(const IPC::Message& msg) {
 }
 
 void NPObjectStub::OnChannelError() {
-  // When the plugin process is shutting down, all the NPObjectStubs
-  // destructors are called.  However the plugin dll might have already
-  // been released, in which case the NPN_ReleaseObject will cause a crash.
-  npobject_ = NULL;
   delete this;
 }
 
diff --git a/chrome/plugin/npobject_stub.h b/chrome/plugin/npobject_stub.h
index 6017f73..cc5eb3a 100644
--- a/chrome/plugin/npobject_stub.h
+++ b/chrome/plugin/npobject_stub.h
@@ -12,11 +12,11 @@
 
 #include "base/gfx/native_widget_types.h"
 #include "base/ref_counted.h"
+#include "base/weak_ptr.h"
 #include "googleurl/src/gurl.h"
 #include "ipc/ipc_channel.h"
 
 class PluginChannelBase;
-class WebPluginDelegateProxy;
 struct NPIdentifier_Param;
 struct NPObject;
 struct NPVariant_Param;
@@ -25,7 +25,8 @@ struct NPVariant_Param;
 // to the object.  The results are marshalled back.  See npobject_proxy.h for
 // more information.
 class NPObjectStub : public IPC::Channel::Listener,
-                     public IPC::Message::Sender {
+                     public IPC::Message::Sender,
+                     public base::SupportsWeakPtr<NPObjectStub> {
  public:
   NPObjectStub(NPObject* npobject,
                PluginChannelBase* channel,
@@ -40,10 +41,7 @@ class NPObjectStub : public IPC::Channel::Listener,
   // Called when the plugin widget that this NPObject came from is destroyed.
   // This is needed because the renderer calls NPN_DeallocateObject on the
   // window script object on destruction to avoid leaks.
-  void set_invalid() { valid_ = false; }
-  void set_proxy(WebPluginDelegateProxy* proxy) {
-    web_plugin_delegate_proxy_ = proxy;
-  }
+  void OnPluginDestroyed();
 
  private:
   // IPC::Channel::Listener implementation:
@@ -81,13 +79,6 @@ class NPObjectStub : public IPC::Channel::Listener,
   NPObject* npobject_;
   scoped_refptr<PluginChannelBase> channel_;
   int route_id_;
-
-  // These variables are used to ensure that the window script object is not
-  // called after the plugin widget has gone away, as the frame manually
-  // deallocates it and ignores the refcount to avoid leaks.
-  bool valid_;
-  WebPluginDelegateProxy* web_plugin_delegate_proxy_;
-
   gfx::NativeViewId containing_window_;
 
   // The url of the main frame hosting the plugin.
diff --git a/chrome/plugin/plugin_channel.cc b/chrome/plugin/plugin_channel.cc
index 54ed6f3..be8e5f9 100644
--- a/chrome/plugin/plugin_channel.cc
+++ b/chrome/plugin/plugin_channel.cc
@@ -222,11 +222,18 @@ void PluginChannel::OnDestroyInstance(int instance_id,
                                       IPC::Message* reply_msg) {
   for (size_t i = 0; i < plugin_stubs_.size(); ++i) {
     if (plugin_stubs_[i]->instance_id() == instance_id) {
-      filter_->ReleaseModalDialogEvent(
-          plugin_stubs_[i]->webplugin()->containing_window());
+      scoped_refptr<MessageFilter> filter(filter_);
+      gfx::NativeViewId window =
+          plugin_stubs_[i]->webplugin()->containing_window();
       plugin_stubs_.erase(plugin_stubs_.begin() + i);
-      RemoveRoute(instance_id);
       Send(reply_msg);
+      RemoveRoute(instance_id);
+      // NOTE: *this* might be deleted as a result of calling RemoveRoute.
+      // Don't release the modal dialog event right away, but do it after the
+      // stack unwinds since the plugin can be destroyed later if it's in use
+      // right now.
+      MessageLoop::current()->PostNonNestableTask(FROM_HERE, NewRunnableMethod(
+          filter.get(), &MessageFilter::ReleaseModalDialogEvent, window));
       return;
     }
   }
diff --git a/chrome/plugin/webplugin_delegate_stub.cc b/chrome/plugin/webplugin_delegate_stub.cc
index ca247f7..d2a583d 100644
--- a/chrome/plugin/webplugin_delegate_stub.cc
+++ b/chrome/plugin/webplugin_delegate_stub.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 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.
 
@@ -52,11 +52,13 @@ WebPluginDelegateStub::WebPluginDelegateStub(
     instance_id_(instance_id),
     channel_(channel),
     delegate_(NULL),
-    webplugin_(NULL) {
+    webplugin_(NULL),
+    in_destructor_(false) {
   DCHECK(channel);
 }
 
 WebPluginDelegateStub::~WebPluginDelegateStub() {
+  in_destructor_ = true;
   child_process_logging::ScopedActiveURLSetter url_setter(page_url_);
 
   if (channel_->in_send()) {
@@ -79,7 +81,9 @@ void WebPluginDelegateStub::OnMessageReceived(const IPC::Message& msg) {
   // A plugin can execute a script to delete itself in any of its NPP methods.
   // Hold an extra reference to ourself so that if this does occur and we're
   // handling a sync message, we don't crash when attempting to send a reply.
-  AddRef();
+  // The exception to this is when we're already in the destructor.
+  if (!in_destructor_)
+    AddRef();
 
   IPC_BEGIN_MESSAGE_MAP(WebPluginDelegateStub, msg)
     IPC_MESSAGE_HANDLER(PluginMsg_Init, OnInit)
@@ -113,7 +117,8 @@ void WebPluginDelegateStub::OnMessageReceived(const IPC::Message& msg) {
     IPC_MESSAGE_UNHANDLED_ERROR()
   IPC_END_MESSAGE_MAP()
 
-  Release();
+  if (!in_destructor_)
+    Release();
 }
 
 bool WebPluginDelegateStub::Send(IPC::Message* msg) {
diff --git a/chrome/plugin/webplugin_delegate_stub.h b/chrome/plugin/webplugin_delegate_stub.h
index 849066f..e85ca0a 100644
--- a/chrome/plugin/webplugin_delegate_stub.h
+++ b/chrome/plugin/webplugin_delegate_stub.h
@@ -101,6 +101,7 @@ class WebPluginDelegateStub : public IPC::Channel::Listener,
 
   WebPluginDelegateImpl* delegate_;
   WebPluginProxy* webplugin_;
+  bool in_destructor_;
 
   // The url of the main frame hosting the plugin.
   GURL page_url_;
diff --git a/chrome/renderer/webplugin_delegate_proxy.cc b/chrome/renderer/webplugin_delegate_proxy.cc
index af4760b..ab787b0 100644
--- a/chrome/renderer/webplugin_delegate_proxy.cc
+++ b/chrome/renderer/webplugin_delegate_proxy.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 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.
 
@@ -165,7 +165,6 @@ WebPluginDelegateProxy::WebPluginDelegateProxy(
       mime_type_(mime_type),
       instance_id_(MSG_ROUTING_NONE),
       npobject_(NULL),
-      window_script_object_(NULL),
       sad_plugin_(NULL),
       invalidate_pending_(false),
       transparent_(false),
@@ -176,30 +175,17 @@ WebPluginDelegateProxy::~WebPluginDelegateProxy() {
 }
 
 void WebPluginDelegateProxy::PluginDestroyed() {
-  if (window_) {
+  if (window_)
     WillDestroyWindow();
-  }
-  plugin_ = NULL;
-
-  if (npobject_) {
-    // When we destroy the plugin instance, the NPObjectStub NULLs out its
-    // pointer to the npobject (see NPObjectStub::OnChannelError).  Therefore,
-    // we release the object before destroying the instance to avoid leaking.
-    WebBindings::releaseObject(npobject_);
-    npobject_ = NULL;
-  }
-
-  if (window_script_object_) {
-    // The ScriptController deallocates this object independent of its ref count
-    // to avoid leaks if the plugin forgets to release it.  So mark the object
-    // invalid to avoid accessing it past this point.
-    window_script_object_->set_proxy(NULL);
-    window_script_object_->set_invalid();
-  }
 
   if (channel_host_) {
-    channel_host_->RemoveRoute(instance_id_);
     Send(new PluginMsg_DestroyInstance(instance_id_));
+
+    // Must remove the route after sending the destroy message, since
+    // RemoveRoute can lead to all the outstanding NPObjects being told the
+    // channel went away if this was the last instance.
+    channel_host_->RemoveRoute(instance_id_);
+
     // Release the channel host now. If we are is the last reference to the
     // channel, this avoids a race where this renderer asks a new connection to
     // the same plugin between now and the time 'this' is actually deleted.
@@ -210,6 +196,17 @@ void WebPluginDelegateProxy::PluginDestroyed() {
     channel_host_ = NULL;
   }
 
+  if (window_script_object_) {
+    // The ScriptController deallocates this object independent of its ref count
+    // to avoid leaks if the plugin forgets to release it.  So mark the object
+    // invalid to avoid accessing it past this point.  Note: only do this after
+    // the DestroyInstance message in case the window object is scripted by the
+    // plugin in NPP_Destroy.
+    window_script_object_->OnPluginDestroyed();
+  }
+
+  plugin_ = NULL;
+
   MessageLoop::current()->DeleteSoon(FROM_HERE, this);
 }
 
@@ -828,10 +825,8 @@ void WebPluginDelegateProxy::OnGetWindowScriptNPObject(
 
   // The stub will delete itself when the proxy tells it that it's released, or
   // otherwise when the channel is closed.
-  NPObjectStub* stub = new NPObjectStub(
-      npobject, channel_host_.get(), route_id, 0, page_url_);
-  window_script_object_ = stub;
-  window_script_object_->set_proxy(this);
+  window_script_object_ = (new NPObjectStub(
+      npobject, channel_host_.get(), route_id, 0, page_url_))->AsWeakPtr();
   *success = true;
   *npobject_ptr = reinterpret_cast<intptr_t>(npobject);
 }
diff --git a/chrome/renderer/webplugin_delegate_proxy.h b/chrome/renderer/webplugin_delegate_proxy.h
index 043bdb8..8651aabf 100644
--- a/chrome/renderer/webplugin_delegate_proxy.h
+++ b/chrome/renderer/webplugin_delegate_proxy.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 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.
 
@@ -45,9 +45,6 @@ class WebPluginDelegateProxy :
   WebPluginDelegateProxy(const std::string& mime_type,
                          const base::WeakPtr<RenderView>& render_view);
 
-  // Called to drop our pointer to the window script object.
-  void DropWindowScriptObject() { window_script_object_ = NULL; }
-
   // WebPluginDelegate implementation:
   virtual void PluginDestroyed();
   virtual bool Initialize(const GURL& url,
@@ -171,7 +168,7 @@ class WebPluginDelegateProxy :
   gfx::Rect plugin_rect_;
 
   NPObject* npobject_;
-  NPObjectStub* window_script_object_;
+  base::WeakPtr<NPObjectStub> window_script_object_;
 
   // Event passed in by the plugin process and is used to decide if
   // messages need to be pumped in the NPP_HandleEvent sync call.
diff --git a/chrome/test/data/npapi/ensure_scripting_works_in_destroy.html b/chrome/test/data/npapi/ensure_scripting_works_in_destroy.html
new file mode 100644
index 0000000..80b9f65
--- /dev/null
+++ b/chrome/test/data/npapi/ensure_scripting_works_in_destroy.html
@@ -0,0 +1,21 @@
+<html>
+<head>
+<script src="npapi.js"></script>
+<script>
+function onloadfunction() {
+  document.getElementById("theiframe").src = "about:blank";
+}
+</script>
+</head>
+
+<body onload="setTimeout(onloadfunction(), 0)">
+<div id="statusPanel" style="border: 1px solid red; width: 100%">
+Test running....
+</div>
+
+Tests that scripting works during NPP_Destroy.
+
+<iframe src="ensure_scripting_works_in_destroy_iframe.html" id="theiframe">
+</iframe>
+</body>
+</html>
diff --git a/chrome/test/data/npapi/ensure_scripting_works_in_destroy_iframe.html b/chrome/test/data/npapi/ensure_scripting_works_in_destroy_iframe.html
new file mode 100644
index 0000000..10cef59
--- /dev/null
+++ b/chrome/test/data/npapi/ensure_scripting_works_in_destroy_iframe.html
@@ -0,0 +1,33 @@
+<html>
+<head>
+<script src="npapi.js"></script>
+<script>
+function GetMagicNumber() {
+  return 42;
+}
+
+function onSuccess(name, id) {
+  parent.onSuccess(name, id);
+}
+
+function onFailure(name, id, status) {
+  parent.onFailure(name, id);
+}
+</script>
+</head>
+
+<body>
+<div id=PluginDiv>
+<embed type="application/vnd.npapi-test"
+  src="foo"
+  name="ensure_scripting_works_in_destroy"
+  id="1"
+  mode="np_embed"
+>
+</DIV>
+<script>
+  var height = document.body.offsetHeight;
+</script>
+
+</body>
+</html>
diff --git a/chrome/test/ui/npapi_uitest.cc b/chrome/test/ui/npapi_uitest.cc
index ae93190..d379426 100644
--- a/chrome/test/ui/npapi_uitest.cc
+++ b/chrome/test/ui/npapi_uitest.cc
@@ -159,27 +159,29 @@ TEST_F(NPAPITester, DISABLED_SelfDeletePluginInvokeAlert) {
 // Tests if a plugin executing a self deleting script in the context of
 // a synchronous paint event works correctly
 TEST_F(NPAPIVisiblePluginTester, SelfDeletePluginInvokeInSynchronousPaint) {
-  if (!UITest::in_process_renderer()) {
-    show_window_ = true;
-    std::wstring test_case = L"execute_script_delete_in_paint.html";
-    GURL url = GetTestUrl(L"npapi", test_case);
-    NavigateToURL(url);
-    WaitForFinish("execute_script_delete_in_paint", "1", url,
-                  kTestCompleteCookie, kTestCompleteSuccess,
-                  kShortWaitTimeout);
-  }
+  if (UITest::in_process_renderer())
+    return;
+
+  show_window_ = true;
+  std::wstring test_case = L"execute_script_delete_in_paint.html";
+  GURL url = GetTestUrl(L"npapi", test_case);
+  NavigateToURL(url);
+  WaitForFinish("execute_script_delete_in_paint", "1", url,
+                kTestCompleteCookie, kTestCompleteSuccess,
+                kShortWaitTimeout);
 }
 
 TEST_F(NPAPIVisiblePluginTester, SelfDeletePluginInNewStream) {
-  if (!UITest::in_process_renderer()) {
-    show_window_ = true;
-    std::wstring test_case = L"self_delete_plugin_stream.html";
-    GURL url = GetTestUrl(L"npapi", test_case);
-    NavigateToURL(url);
-    WaitForFinish("self_delete_plugin_stream", "1", url,
-                  kTestCompleteCookie, kTestCompleteSuccess,
-                  kShortWaitTimeout);
-  }
+  if (UITest::in_process_renderer())
+    return;
+
+  show_window_ = true;
+  std::wstring test_case = L"self_delete_plugin_stream.html";
+  GURL url = GetTestUrl(L"npapi", test_case);
+  NavigateToURL(url);
+  WaitForFinish("self_delete_plugin_stream", "1", url,
+                kTestCompleteCookie, kTestCompleteSuccess,
+                kShortWaitTimeout);
 }
 
 // Tests if a plugin has a non zero window rect.
@@ -235,15 +237,16 @@ TEST_F(NPAPIVisiblePluginTester, AlertInWindowMessage) {
 #endif
 
 TEST_F(NPAPIVisiblePluginTester, VerifyNPObjectLifetimeTest) {
-  if (!UITest::in_process_renderer()) {
-    show_window_ = true;
-    std::wstring test_case = L"npobject_lifetime_test.html";
-    GURL url = GetTestUrl(L"npapi", test_case);
-    NavigateToURL(url);
-    WaitForFinish("npobject_lifetime_test", "1", url,
-                  kTestCompleteCookie, kTestCompleteSuccess,
-                  kShortWaitTimeout);
-  }
+  if (UITest::in_process_renderer())
+    return;
+
+  show_window_ = true;
+  std::wstring test_case = L"npobject_lifetime_test.html";
+  GURL url = GetTestUrl(L"npapi", test_case);
+  NavigateToURL(url);
+  WaitForFinish("npobject_lifetime_test", "1", url,
+                kTestCompleteCookie, kTestCompleteSuccess,
+                kShortWaitTimeout);
 }
 
 // Tests that we don't crash or assert if NPP_New fails
@@ -255,14 +258,15 @@ TEST_F(NPAPIVisiblePluginTester, NewFails) {
 }
 
 TEST_F(NPAPIVisiblePluginTester, SelfDeletePluginInNPNEvaluate) {
-  if (!UITest::in_process_renderer()) {
-    GURL url = GetTestUrl(L"npapi",
-                          L"execute_script_delete_in_npn_evaluate.html");
-    NavigateToURL(url);
-    WaitForFinish("npobject_delete_plugin_in_evaluate", "1", url,
-                  kTestCompleteCookie, kTestCompleteSuccess,
-                  kShortWaitTimeout);
-  }
+  if (UITest::in_process_renderer())
+    return;
+
+  GURL url = GetTestUrl(L"npapi",
+                        L"execute_script_delete_in_npn_evaluate.html");
+  NavigateToURL(url);
+  WaitForFinish("npobject_delete_plugin_in_evaluate", "1", url,
+                kTestCompleteCookie, kTestCompleteSuccess,
+                kShortWaitTimeout);
 }
 
 // Flaky. See http://crbug.com/17645
@@ -321,3 +325,14 @@ TEST_F(NPAPIVisiblePluginTester, GetURLRequestFailWrite) {
   WaitForFinish("geturl_fail_write", "1", url, kTestCompleteCookie,
                 kTestCompleteSuccess, kShortWaitTimeout);
 }
+
+TEST_F(NPAPITester, EnsureScriptingWorksInDestroy) {
+  if (UITest::in_process_renderer())
+    return;
+
+  GURL url = GetTestUrl(L"npapi", L"ensure_scripting_works_in_destroy.html");
+  NavigateToURL(url);
+  WaitForFinish("ensure_scripting_works_in_destroy", "1", url,
+                kTestCompleteCookie, kTestCompleteSuccess,
+                kShortWaitTimeout);
+}