PPAPI: Really force-free NPObjects on Instance destruction.

(There still seems to be a memory leak with this patch; I may have to check our NPObject reference counting next.)

BUG=114023
TEST=


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

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

7 files changed, 229 insertions, 39 deletions

diff --git a/ppapi/tests/test_case.cc b/ppapi/tests/test_case.cc
index 90e7d72..0100ded 100644
--- a/ppapi/tests/test_case.cc
+++ b/ppapi/tests/test_case.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -47,8 +47,13 @@ std::string TestCase::MakeFailureMessage(const char* file,
 pp::VarPrivate TestCase::GetTestObject() {
   if (test_object_.is_undefined()) {
     pp::deprecated::ScriptableObject* so = CreateTestObject();
-    if (so)
+    if (so) {
       test_object_ = pp::VarPrivate(instance_, so);  // Takes ownership.
+      // CheckResourcesAndVars runs and looks for leaks before we've actually
+      // completely shut down. Ignore the instance object, since it's not a real
+      // leak.
+      IgnoreLeakedVar(test_object_.pp_var().value.as_id);
+    }
   }
   return test_object_;
 }
@@ -78,6 +83,10 @@ bool TestCase::HandleInputEvent(const pp::InputEvent& event) {
   return false;
 }
 
+void TestCase::IgnoreLeakedVar(int64_t id) {
+  ignored_leaked_vars_.insert(id);
+}
+
 #if !(defined __native_client__)
 pp::deprecated::ScriptableObject* TestCase::CreateTestObject() {
   return NULL;
@@ -111,31 +120,27 @@ std::string TestCase::CheckResourcesAndVars() {
       errors += output.str();
     }
     */
-    const uint32_t kVarsToPrint = 10;
+    const int kVarsToPrint = 100;
     PP_Var vars[kVarsToPrint];
-    uint32_t leaked_vars = testing_interface_->GetLiveVars(vars, kVarsToPrint);
-    uint32_t tracked_vars = leaked_vars;
-#if !(defined __native_client__)
-    // Don't count test_object_ as a leak.
-    if (test_object_.pp_var().type > PP_VARTYPE_DOUBLE)
-      --leaked_vars;
-#endif
-    if (leaked_vars) {
+    int found_vars = testing_interface_->GetLiveVars(vars, kVarsToPrint);
+    // This will undercount if we are told to ignore a Var which is then *not*
+    // leaked. Worst case, we just won't print the little "Test leaked" message,
+    // but we'll still print any non-ignored leaked vars we found.
+    int leaked_vars =
+        found_vars - static_cast<int>(ignored_leaked_vars_.size());
+    if (leaked_vars > 0) {
       std::ostringstream output;
       output << "Test leaked " << leaked_vars << " vars (printing at most "
              << kVarsToPrint <<"):<p>";
       errors += output.str();
-      for (uint32_t i = 0; i < std::min(tracked_vars, kVarsToPrint); ++i) {
-        pp::Var leaked_var(pp::PASS_REF, vars[i]);
-#if (defined __native_client__)
+    }
+    for (int i = 0; i < std::min(found_vars, kVarsToPrint); ++i) {
+      pp::Var leaked_var(pp::PASS_REF, vars[i]);
+      if (ignored_leaked_vars_.count(leaked_var.pp_var().value.as_id) == 0)
         errors += leaked_var.DebugString() + "<p>";
-#else
-        if (!(leaked_var == test_object_))
-          errors += leaked_var.DebugString() + "<p>";
-#endif
-      }
     }
   }
   return errors;
 }
 
+
diff --git a/ppapi/tests/test_case.h b/ppapi/tests/test_case.h
index c04ed83..ee22f17 100644
--- a/ppapi/tests/test_case.h
+++ b/ppapi/tests/test_case.h
@@ -7,6 +7,7 @@
 
 #include <cmath>
 #include <limits>
+#include <set>
 #include <string>
 
 #include "ppapi/c/pp_resource.h"
@@ -70,10 +71,16 @@ class TestCase {
   // that want to handle view changes should override this method.
   virtual bool HandleInputEvent(const pp::InputEvent& event);
 
+  void IgnoreLeakedVar(int64_t id);
+
+  TestingInstance* instance() { return instance_; }
+
+  const PPB_Testing_Dev* testing_interface() { return testing_interface_; }
+
  protected:
 #if !(defined __native_client__)
   // Overridden by each test to supply a ScriptableObject corresponding to the
-  // test. There can only be one object created for all test in a given class
+  // test. There can only be one object created for all tests in a given class,
   // so be sure your object is designed to be re-used.
   //
   // This object should be created on the heap. Ownership will be passed to the
@@ -108,6 +115,9 @@ class TestCase {
   bool force_async_;
 
  private:
+  // Var ids that should be ignored when checking for leaks on shutdown.
+  std::set<int64_t> ignored_leaked_vars_;
+
 #if !(defined __native_client__)
   // Holds the test object, if any was retrieved from CreateTestObject.
   pp::VarPrivate test_object_;
diff --git a/ppapi/tests/test_case.html b/ppapi/tests/test_case.html
index f5c046e..92aebdb 100644
--- a/ppapi/tests/test_case.html
+++ b/ppapi/tests/test_case.html
@@ -10,6 +10,11 @@ function AdjustHeight(frameWin) {
 }
 
 function DidExecuteTests() {
+  var plugin = document.getElementById("plugin");
+  plugin.parentNode.removeChild(plugin);
+  plugin = undefined;
+  CheckPostConditions();
+
   if (window == top)
     return;
 
@@ -79,9 +84,10 @@ function ExtractSearchParameter(name) {
 //  - LogHTML
 //  - SetCookie
 //  - EvalScript
+//  - AddPostCondition
 // The second item is the verbatim message_contents.
 function ParseTestingMessage(message_data) {
-  if (typeof(message_data)!='string')
+  if (typeof(message_data) != "string")
     return undefined;
   var testing_message_prefix = "TESTING_MESSAGE";
   var delim_str = ":";
@@ -115,6 +121,46 @@ function EvalScript(script) {
   }
 }
 
+var conditions = [];
+// Add a "PostCondition". These are bits of script that are run after the plugin
+// is destroyed. If they evaluate to false or throw an exception, it's
+// considered a failure.
+function AddPostCondition(script) {
+  conditions.push(script);
+}
+// Update the HTML to show the failure and update cookies so that ui_tests
+// doesn't count this as a pass.
+function ConditionFailed(error) {
+  error_string = "Post condition check failed: " + error;
+  var i = 0;
+  // If the plugin thinks the test passed but a post-condition failed, we want
+  // to clear the PASS cookie so that ui_tests doesn't count it is a passed
+  // test.
+  if (window.document.cookie.indexOf("PASS") != -1) {
+    while (window.document.cookie.indexOf("PPAPI_PROGRESS_" + i) != -1) {
+      window.document.cookie = "PPAPI_PROGRESS_" + i + "=; max-age=0";
+      ++i;
+    }
+    window.document.cookie = "PPAPI_PROGRESS_0=" + error_string
+  }
+  LogHTML("<p>" + error_string);
+}
+// Iterate through the post conditions defined in |conditions| and check that
+// they all pass.
+function CheckPostConditions() {
+  for (var i = 0; i < conditions.length; ++i) {
+    var script = conditions[i];
+    try {
+      if (!eval(script)) {
+        ConditionFailed("\"" + script + "\"");
+      }
+    } catch (ex) {
+      ConditionFailed("\"" + script + "\"" + " failed with exception: " +
+                      "\"" + ex.toString() + "\"");
+    }
+  }
+}
+
 function IsTestingMessage(message_data) {
   return (ParseTestingMessage(message_data) != undefined);
 }
@@ -134,6 +180,8 @@ function handleTestingMessage(message_event) {
       SetCookie(contents);
     else if (type === "EvalScript")
       EvalScript(contents);
+    else if (type == "AddPostCondition")
+      AddPostCondition(contents);
   }
 }
 
@@ -184,6 +232,9 @@ onload = function() {
 // particular, we replace document.createEvent, MessageEvent.initMessageEvent,
 // and the MessageEvent constructor. Previous versions of the PostMessage
 // implementation made use of these and would fail (http://crbug.com/82604).
+// TODO(dmichael): These should clear the PASS cookie so that ui_tests sees
+//                 these as a failure (see ConditionFailed for how to do this).
+//                 crbug.com/109775
 document.createEvent = function() {
   LogHTML("<p>Bad document.createEvent called!");
 }
diff --git a/ppapi/tests/test_instance_deprecated.cc b/ppapi/tests/test_instance_deprecated.cc
index e7ac48b..72e1fad 100644
--- a/ppapi/tests/test_instance_deprecated.cc
+++ b/ppapi/tests/test_instance_deprecated.cc
@@ -1,9 +1,12 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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/tests/test_instance_deprecated.h"
 
+#include <assert.h>
+
+#include "ppapi/c/ppb_var.h"
 #include "ppapi/cpp/module.h"
 #include "ppapi/cpp/dev/scriptable_object_deprecated.h"
 #include "ppapi/tests/testing_instance.h"
@@ -17,7 +20,8 @@ static const char kReturnValueFunction[] = "ReturnValue";
 // ScriptableObject used by instance.
 class InstanceSO : public pp::deprecated::ScriptableObject {
  public:
-  InstanceSO(TestInstance* i) : test_instance_(i) {}
+  InstanceSO(TestInstance* i);
+  virtual ~InstanceSO();
 
   // pp::deprecated::ScriptableObject overrides.
   bool HasMethod(const pp::Var& name, pp::Var* exception);
@@ -29,6 +33,27 @@ class InstanceSO : public pp::deprecated::ScriptableObject {
   TestInstance* test_instance_;
 };
 
+InstanceSO::InstanceSO(TestInstance* i) : test_instance_(i) {
+  // Set up a post-condition for the test so that we can ensure our destructor
+  // is called. This only works in-process right now. Rather than disable the
+  // whole test, we only do this check when running in-process.
+  // TODO(dmichael): Figure out if we want this to work out-of-process, and if
+  //                 so, fix it. Note that it might just be failing because the
+  //                 ReleaseObject and Deallocate messages are asynchronous.
+  if (i->testing_interface() &&
+      i->testing_interface()->IsOutOfProcess() == PP_FALSE) {
+    i->instance()->AddPostCondition(
+        "window.document.getElementById('container').instance_object_destroyed"
+        );
+  }
+}
+
+InstanceSO::~InstanceSO() {
+  pp::Var exception;
+  pp::Var ret = test_instance_->instance()->ExecuteScript(
+      "document.getElementById('container').instance_object_destroyed=true;");
+}
+
 bool InstanceSO::HasMethod(const pp::Var& name, pp::Var* exception) {
   if (!name.is_string())
     return false;
@@ -79,6 +104,15 @@ bool TestInstance::Init() {
 
 void TestInstance::RunTests(const std::string& filter) {
   RUN_TEST(ExecuteScript, filter);
+  RUN_TEST(RecursiveObjects, filter);
+  RUN_TEST(LeakedObjectDestructors, filter);
+}
+
+void TestInstance::LeakReferenceAndIgnore(const pp::Var& leaked) {
+  static const PPB_Var* var_interface = static_cast<const PPB_Var*>(
+        pp::Module::Get()->GetBrowserInterface(PPB_VAR_INTERFACE));
+  var_interface->AddRef(leaked.pp_var());
+  IgnoreLeakedVar(leaked.pp_var().value.as_id);
 }
 
 pp::deprecated::ScriptableObject* TestInstance::CreateTestObject() {
@@ -120,3 +154,67 @@ std::string TestInstance::TestExecuteScript() {
 
   PASS();
 }
+
+// A scriptable object that contains other scriptable objects recursively. This
+// is used to help verify that our scriptable object clean-up code works
+// properly.
+class ObjectWithChildren : public pp::deprecated::ScriptableObject {
+ public:
+  ObjectWithChildren(TestInstance* i, int num_descendents) {
+    if (num_descendents > 0) {
+      child_ = pp::VarPrivate(i->instance(),
+                              new ObjectWithChildren(i, num_descendents - 1));
+    }
+  }
+  struct IgnoreLeaks {};
+  ObjectWithChildren(TestInstance* i, int num_descendents, IgnoreLeaks) {
+    if (num_descendents > 0) {
+      child_ = pp::VarPrivate(i->instance(),
+                              new ObjectWithChildren(i, num_descendents - 1,
+                                                     IgnoreLeaks()));
+      i->IgnoreLeakedVar(child_.pp_var().value.as_id);
+    }
+  }
+ private:
+  pp::VarPrivate child_;
+};
+
+std::string TestInstance::TestRecursiveObjects() {
+  // These should be deleted when we exit scope, so should not leak.
+  pp::VarPrivate not_leaked(instance(), new ObjectWithChildren(this, 50));
+
+  // Leak some, but tell TestCase to ignore the leaks. This test is run and then
+  // reloaded (see ppapi_uitest.cc). If these aren't cleaned up when the first
+  // run is torn down, they will show up as leaks in the second run.
+  // NOTE: The ScriptableObjects are actually leaked, but they should be removed
+  //       from the tracker. See below for a test that verifies that the
+  //       destructor is not run.
+  pp::VarPrivate leaked(
+      instance(),
+      new ObjectWithChildren(this, 50, ObjectWithChildren::IgnoreLeaks()));
+  // Now leak a reference to the root object. This should force the root and
+  // all its descendents to stay in the tracker.
+  LeakReferenceAndIgnore(leaked);
+
+  PASS();
+}
+
+// A scriptable object that should cause a crash if its destructor is run. We
+// don't run the destructor for objects which the plugin leaks. This is to
+// prevent them doing dangerous things at cleanup time, such as executing script
+// or creating new objects.
+class BadDestructorObject : public pp::deprecated::ScriptableObject {
+ public:
+  BadDestructorObject() {}
+  ~BadDestructorObject() {
+    assert(false);
+  }
+};
+
+std::string TestInstance::TestLeakedObjectDestructors() {
+  pp::VarPrivate leaked(instance(), new BadDestructorObject());
+  // Leak a reference so it gets deleted on instance shutdown.
+  LeakReferenceAndIgnore(leaked);
+  PASS();
+}
+
diff --git a/ppapi/tests/test_instance_deprecated.h b/ppapi/tests/test_instance_deprecated.h
index 8b81501..6ec9b46 100644
--- a/ppapi/tests/test_instance_deprecated.h
+++ b/ppapi/tests/test_instance_deprecated.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -20,12 +20,19 @@ class TestInstance : public TestCase {
 
   void set_string(const std::string& s) { string_ = s; }
 
+  // Leak a reference to the given var, but ignore the leak in the leak checking
+  // that happens at shutdown. This allows us to test the "ForceFree" that
+  // happens on instance shutdown.
+  void LeakReferenceAndIgnore(const pp::Var& leaked);
+
  protected:
   // Test case protected overrides.
   virtual pp::deprecated::ScriptableObject* CreateTestObject();
 
  private:
   std::string TestExecuteScript();
+  std::string TestRecursiveObjects();
+  std::string TestLeakedObjectDestructors();
 
   // Value written by set_string which is called by the ScriptableObject. This
   // allows us to keep track of what was called.
diff --git a/ppapi/tests/testing_instance.cc b/ppapi/tests/testing_instance.cc
index 58e0890..1da6105 100644
--- a/ppapi/tests/testing_instance.cc
+++ b/ppapi/tests/testing_instance.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -102,18 +102,16 @@ bool TestingInstance::HandleInputEvent(const pp::InputEvent& event) {
 }
 
 void TestingInstance::EvalScript(const std::string& script) {
-  std::string message("TESTING_MESSAGE:EvalScript:");
-  message.append(script);
-  PostMessage(pp::Var(message));
+  SendTestCommand("EvalScript", script);
 }
 
 void TestingInstance::SetCookie(const std::string& name,
                                 const std::string& value) {
-  std::string message("TESTING_MESSAGE:SetCookie:");
-  message.append(name);
-  message.append("=");
-  message.append(value);
-  PostMessage(pp::Var(message));
+  SendTestCommand("SetCookie", name + "=" + value);
+}
+
+void TestingInstance::AddPostCondition(const std::string& script) {
+  SendTestCommand("AddPostCondition", script);
 }
 
 void TestingInstance::LogTest(const std::string& test_name,
@@ -150,7 +148,7 @@ void TestingInstance::ExecuteTests(int32_t unused) {
   ReportProgress(kProgressSignal);
 
   // Clear the console.
-  PostMessage(pp::Var("TESTING_MESSAGE:ClearConsole"));
+  SendTestCommand("ClearConsole");
 
   if (!errors_.empty()) {
     // Catch initialization errors and output the current error string to
@@ -169,7 +167,9 @@ void TestingInstance::ExecuteTests(int32_t unused) {
 
   // Declare we're done by setting a cookie to either "PASS" or the errors.
   ReportProgress(errors_.empty() ? "PASS" : errors_);
-  PostMessage(pp::Var("TESTING_MESSAGE:DidExecuteTests"));
+  SendTestCommand("DidExecuteTests");
+  // Note, DidExecuteTests unloads the plugin. We can't really do anthing after
+  // this point.
 }
 
 TestCase* TestingInstance::CaseForTestName(const std::string& name) {
@@ -190,6 +190,18 @@ std::string TestingInstance::FilterForTestName(const std::string& name) {
   return "";
 }
 
+void TestingInstance::SendTestCommand(const std::string& command) {
+  std::string msg("TESTING_MESSAGE:");
+  msg += command;
+  PostMessage(pp::Var(msg));
+}
+
+void TestingInstance::SendTestCommand(const std::string& command,
+                                      const std::string& params) {
+  SendTestCommand(command + ":" + params);
+}
+
+
 void TestingInstance::LogAvailableTests() {
   // Print out a listing of all tests.
   std::vector<std::string> test_cases;
@@ -226,9 +238,7 @@ void TestingInstance::LogError(const std::string& text) {
 }
 
 void TestingInstance::LogHTML(const std::string& html) {
-  std::string message("TESTING_MESSAGE:LogHTML:");
-  message.append(html);
-  PostMessage(pp::Var(message));
+  SendTestCommand("LogHTML", html);
 }
 
 void TestingInstance::ReportProgress(const std::string& progress_value) {
diff --git a/ppapi/tests/testing_instance.h b/ppapi/tests/testing_instance.h
index 7b05174..f480e9d 100644
--- a/ppapi/tests/testing_instance.h
+++ b/ppapi/tests/testing_instance.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -89,6 +89,11 @@ pp::InstancePrivate {
   // Sets the given cookie in the current document.
   void SetCookie(const std::string& name, const std::string& value);
 
+  // Add a post-condition to the JavaScript on the test_case.html page. This
+  // JavaScript code will be run after the instance is shut down and must
+  // evaluate to |true| or the test will fail.
+  void AddPostCondition(const std::string& script);
+
  private:
   void ExecuteTests(int32_t unused);
 
@@ -106,6 +111,10 @@ pp::InstancePrivate {
   // Runs 'PostMessage_SendingData.
   std::string FilterForTestName(const std::string& name);
 
+  // Sends a test command to the page using PostMessage.
+  void SendTestCommand(const std::string& command);
+  void SendTestCommand(const std::string& command, const std::string& params);
+
   // Appends a list of available tests to the console in the document.
   void LogAvailableTests();