Keep the module in scope when executing scripts. This prevents a crash when the

script deletes the plugin object synchronously. This in turn deletes the
dispatcher which will make the code returning the out param and exception to
the plugin crash.

To prevent the crash, this patch adds a way for the proxy to manipulate the
refcount of the plugin object so that it's still alive when as long as the
scripting message is being processed.

A manual test is included. This is not automatically run now. I tried to fit it
into the current test infrastructure and found it very challenging, We need to
revisit this to allow custom tests to more easily be written.

TEST=manual with included plugin and html
BUG=none
Review URL: http://codereview.chromium.org/6881012

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

9 files changed, 166 insertions, 1 deletions

diff --git a/ppapi/c/private/ppb_proxy_private.h b/ppapi/c/private/ppb_proxy_private.h
index e1e133e..19d5a1e 100644
--- a/ppapi/c/private/ppb_proxy_private.h
+++ b/ppapi/c/private/ppb_proxy_private.h
@@ -38,6 +38,12 @@ struct PPB_Proxy_Private {
   // buffer. Returns 0 on failure or if the url loader doesn't have any data
   // now.
   int32_t (*GetURLLoaderBufferedBytes)(PP_Resource url_loader);
+
+  // Allows adding additional refcounts to the PluginModule that owns the
+  // proxy dispatcher (and all interface proxies). For every AddRef call
+  // there must be a corresponding release call.
+  void (*AddRefModule)(PP_Module module);
+  void (*ReleaseModule)(PP_Module module);
 };
 
 #endif  // PPAPI_C_PRIVATE_PROXY_PRIVATE_H_
diff --git a/ppapi/proxy/host_dispatcher.cc b/ppapi/proxy/host_dispatcher.cc
index 25e06fb..cba66de 100644
--- a/ppapi/proxy/host_dispatcher.cc
+++ b/ppapi/proxy/host_dispatcher.cc
@@ -240,6 +240,18 @@ InterfaceProxy* HostDispatcher::CreatePPBInterfaceProxy(
   return proxy;
 }
 
+// ScopedModuleReference -------------------------------------------------------
+
+ScopedModuleReference::ScopedModuleReference(Dispatcher* dispatcher) {
+  DCHECK(!dispatcher->IsPlugin());
+  dispatcher_ = static_cast<HostDispatcher*>(dispatcher);
+  dispatcher_->ppb_proxy()->AddRefModule(dispatcher_->pp_module());
+}
+
+ScopedModuleReference::~ScopedModuleReference() {
+  dispatcher_->ppb_proxy()->ReleaseModule(dispatcher_->pp_module());
+}
+
 }  // namespace proxy
 }  // namespace pp
 
diff --git a/ppapi/proxy/host_dispatcher.h b/ppapi/proxy/host_dispatcher.h
index ea676d1..2391308 100644
--- a/ppapi/proxy/host_dispatcher.h
+++ b/ppapi/proxy/host_dispatcher.h
@@ -128,6 +128,23 @@ class HostDispatcher : public Dispatcher {
   DISALLOW_COPY_AND_ASSIGN(HostDispatcher);
 };
 
+// Create this object on the stack to prevent the module (and hence the
+// dispatcher) from being deleted out from under you. This is necessary when
+// calling some scripting functions that may delete the plugin.
+//
+// This may only be called in the host. The parameter is a plain Dispatcher
+// since that's what most callers have.
+class ScopedModuleReference {
+ public:
+  ScopedModuleReference(Dispatcher* dispatcher);
+  ~ScopedModuleReference();
+
+ private:
+  HostDispatcher* dispatcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedModuleReference);
+};
+
 }  // namespace proxy
 }  // namespace pp
 
diff --git a/ppapi/proxy/ppb_instance_proxy.cc b/ppapi/proxy/ppb_instance_proxy.cc
index 4d6f5f3..b2d6291 100644
--- a/ppapi/proxy/ppb_instance_proxy.cc
+++ b/ppapi/proxy/ppb_instance_proxy.cc
@@ -120,6 +120,12 @@ const InterfaceProxy::Info* PPB_Instance_Proxy::GetInfo() {
 }
 
 bool PPB_Instance_Proxy::OnMessageReceived(const IPC::Message& msg) {
+  // Prevent the dispatcher from going away during a call to ExecuteScript.
+  // This must happen OUTSIDE of ExecuteScript since the SerializedVars use
+  // the dispatcher upon return of the function (converting the
+  // SerializedVarReturnValue/OutParam to a SerializedVar in the destructor).
+  ScopedModuleReference death_grip(dispatcher());
+
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(PPB_Instance_Proxy, msg)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBInstance_GetWindowObject,
diff --git a/ppapi/proxy/ppb_var_deprecated_proxy.cc b/ppapi/proxy/ppb_var_deprecated_proxy.cc
index 9571689..aca0a9e 100644
--- a/ppapi/proxy/ppb_var_deprecated_proxy.cc
+++ b/ppapi/proxy/ppb_var_deprecated_proxy.cc
@@ -315,6 +315,13 @@ const InterfaceProxy::Info* PPB_Var_Deprecated_Proxy::GetInfo() {
 }
 
 bool PPB_Var_Deprecated_Proxy::OnMessageReceived(const IPC::Message& msg) {
+  // Prevent the dispatcher from going away during a call to Call or other
+  // function that could mutate the DOM. This must happen OUTSIDE of
+  // the message handlers since the SerializedVars use the dispatcher upon
+  // return of the function (converting the SerializedVarReturnValue/OutParam
+  // to a SerializedVar in the destructor).
+  ScopedModuleReference death_grip(dispatcher());
+
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(PPB_Var_Deprecated_Proxy, msg)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBVar_HasProperty,
diff --git a/ppapi/tests/manual/delete_plugin.cc b/ppapi/tests/manual/delete_plugin.cc
new file mode 100644
index 0000000..788100b
--- /dev/null
+++ b/ppapi/tests/manual/delete_plugin.cc
@@ -0,0 +1,76 @@
+// Copyright (c) 2011 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 "ppapi/c/pp_errors.h"
+#include "ppapi/c/pp_input_event.h"
+#include "ppapi/c/ppb_var.h"
+#include "ppapi/c/trusted/ppb_instance_trusted.h"
+#include "ppapi/cpp/completion_callback.h"
+#include "ppapi/cpp/instance.h"
+#include "ppapi/cpp/module.h"
+#include "ppapi/cpp/private/var_private.h"
+
+class MyInstance : public pp::Instance {
+ public:
+  MyInstance(PP_Instance instance) : pp::Instance(instance) {
+    factory_.Initialize(this);
+  }
+
+  virtual ~MyInstance() {
+  }
+
+  virtual bool Init(uint32_t argc, const char* argn[], const char* argv[]) {
+    return true;
+  }
+
+  virtual bool HandleInputEvent(const PP_InputEvent& event) {
+    switch (event.type) {
+      case PP_INPUTEVENT_TYPE_MOUSEDOWN:
+        pp::Module::Get()->core()->CallOnMainThread(100,
+            factory_.NewCallback(&MyInstance::SayHello));
+        return true;
+      case PP_INPUTEVENT_TYPE_MOUSEMOVE:
+        return true;
+      case PP_INPUTEVENT_TYPE_KEYDOWN:
+        return true;
+      default:
+        return false;
+    }
+  }
+
+ private:
+  void SayHello(int32_t) {
+    pp::Var code("deletePlugin()");
+    /*
+    When scripting is removed from instance, this is the code that will do the
+    same thing:
+    const PPB_Instance_Trusted* inst =
+        (const PPB_Instance_Trusted*)pp::Module::Get()->GetBrowserInterface(
+            PPB_INSTANCE_TRUSTED_INTERFACE);
+    inst->ExecuteScript(pp_instance(), code.pp_var(), NULL);
+    */
+    ExecuteScript(code);
+  }
+
+  pp::CompletionCallbackFactory<MyInstance> factory_;
+};
+
+class MyModule : public pp::Module {
+ public:
+  MyModule() : pp::Module() {}
+  virtual ~MyModule() {}
+
+  virtual pp::Instance* CreateInstance(PP_Instance instance) {
+    return new MyInstance(instance);
+  }
+};
+
+namespace pp {
+
+// Factory function for your specialization of the Module object.
+Module* CreateModule() {
+  return new MyModule();
+}
+
+}  // namespace pp
diff --git a/ppapi/tests/manual/delete_plugin.html b/ppapi/tests/manual/delete_plugin.html
new file mode 100644
index 0000000..9437c3d
--- /dev/null
+++ b/ppapi/tests/manual/delete_plugin.html
@@ -0,0 +1,25 @@
+<html><head><title>
+<body>
+<script type="text/javascript">
+function deletePlugin() {
+  // Remove the plugin.
+  document.getElementById('foo').removeChild(document.getElementById('plugin'));
+
+  // Forces a style recalculation which actually will delete the plugin.
+  // Without this, the plugin would be deleted later from a timer. If the test
+  // fails, this line will cause the crash.
+  var foo = document.getElementById('foo').offsetWidth;
+
+  // If we get here, there's no crash so we succeeded.
+  document.cookie = "COMPLETION_COOKIE=PASS; path=/";
+}
+</script>
+  This test tests deleting a the &lt;object&gt; synchronously from within a
+  script call. Neither the proxy nor the PPAPI implementation should crash in
+  this case.
+  <div id="foo">
+    <object id="plugin" type="application/x-ppapi-tests" width="400" height="400" style="border:5px solid blue;">
+      <param name="customtest" value="delete_plugin">
+    </object>
+  </div>
+</body>
diff --git a/webkit/plugins/ppapi/ppapi_plugin_instance.cc b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
index cd2d59f..78df75b 100644
--- a/webkit/plugins/ppapi/ppapi_plugin_instance.cc
+++ b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
@@ -664,6 +664,8 @@ PP_Var PluginInstance::ExecuteScript(PP_Var script, PP_Var* exception) {
   NPVariant result;
   bool ok = WebBindings::evaluate(NULL, frame->windowObject(), &np_script,
                                   &result);
+  // DANGER! |this| could be deleted at this point if the script removed the
+  // plugin from the DOM.
   if (!ok) {
     // TODO(brettw) bug 54011: The TryCatch isn't working properly and
     // doesn't actually catch this exception.
diff --git a/webkit/plugins/ppapi/ppb_proxy_impl.cc b/webkit/plugins/ppapi/ppb_proxy_impl.cc
index 9c69891..fc861af 100644
--- a/webkit/plugins/ppapi/ppb_proxy_impl.cc
+++ b/webkit/plugins/ppapi/ppb_proxy_impl.cc
@@ -44,11 +44,25 @@ int32_t GetURLLoaderBufferedBytes(PP_Resource url_loader) {
   return loader->buffer_size();
 }
 
+void AddRefModule(PP_Module module) {
+  PluginModule* plugin_module = ResourceTracker::Get()->GetModule(module);
+  if (plugin_module)
+    plugin_module->AddRef();
+}
+
+void ReleaseModule(PP_Module module) {
+  PluginModule* plugin_module = ResourceTracker::Get()->GetModule(module);
+  if (plugin_module)
+    plugin_module->Release();
+}
+
 const PPB_Proxy_Private ppb_proxy = {
   &PluginCrashed,
   &GetInstanceForResource,
   &SetReserveInstanceIDCallback,
-  &GetURLLoaderBufferedBytes
+  &GetURLLoaderBufferedBytes,
+  &AddRefModule,
+  &ReleaseModule
 };
 
 }  // namespace