Fixed a crash and added a test to find it.

BUG=46526
TEST=Run test_shell_tests.exe
Review URL: http://codereview.chromium.org/2817005

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

3 files changed, 67 insertions, 14 deletions

diff --git a/webkit/glue/plugins/plugin_lib.cc b/webkit/glue/plugins/plugin_lib.cc
index c335b51..d3ae50e 100644
--- a/webkit/glue/plugins/plugin_lib.cc
+++ b/webkit/glue/plugins/plugin_lib.cc
@@ -43,11 +43,17 @@ PluginLib* PluginLib::CreatePluginLib(const FilePath& filename) {
 
 void PluginLib::UnloadAllPlugins() {
   if (g_loaded_libs) {
-    for (size_t i = 0; i < g_loaded_libs->size(); ++i)
-      (*g_loaded_libs)[i]->Unload();
-
-    delete g_loaded_libs;
-    g_loaded_libs = NULL;
+    // PluginLib::Unload() can remove items from the list and even delete
+    // the list when it removes the last item, so we must work with a copy
+    // of the list so that we don't get the carpet removed under our feet.
+    std::vector<scoped_refptr<PluginLib> > loaded_libs(*g_loaded_libs);
+    for (size_t i = 0; i < loaded_libs.size(); ++i)
+      loaded_libs[i]->Unload();
+
+    if (g_loaded_libs && g_loaded_libs->empty()) {
+      delete g_loaded_libs;
+      g_loaded_libs = NULL;
+    }
   }
 }
 
@@ -68,7 +74,7 @@ PluginLib::PluginLib(const WebPluginInfo& info,
       skip_unload_(false),
       always_loaded_(false) {
   StatsCounter(kPluginLibrariesLoadedCounter).Increment();
-  memset((void*)&plugin_funcs_, 0, sizeof(plugin_funcs_));
+  memset(static_cast<void*>(&plugin_funcs_), 0, sizeof(plugin_funcs_));
   g_loaded_libs->push_back(this);
 
   if (entry_points) {
@@ -142,7 +148,7 @@ PluginInstance* PluginLib::CreateInstance(const std::string& mime_type) {
   PluginInstance* new_instance = new PluginInstance(this, mime_type);
   instance_count_++;
   StatsCounter(kPluginInstancesActiveCounter).Increment();
-  DCHECK(new_instance != 0);
+  DCHECK_NE(static_cast<PluginInstance*>(NULL), new_instance);
   return new_instance;
 }
 
@@ -277,7 +283,7 @@ void PluginLib::Unload() {
         base::UnloadNativeLibrary(library_);
     }
 
-    library_ = 0;
+    library_ = NULL;
   }
 
   for (size_t i = 0; i < g_loaded_libs->size(); ++i) {
diff --git a/webkit/glue/plugins/plugin_lib.h b/webkit/glue/plugins/plugin_lib.h
index 53568f1..3888eec 100644
--- a/webkit/glue/plugins/plugin_lib.h
+++ b/webkit/glue/plugins/plugin_lib.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef WEBKIT_GLUE_PLUGIN_PLUGIN_LIB_H_
-#define WEBKIT_GLUE_PLUGIN_PLUGIN_LIB_H_
+#ifndef WEBKIT_GLUE_PLUGINS_PLUGIN_LIB_H_
+#define WEBKIT_GLUE_PLUGINS_PLUGIN_LIB_H_
 
 #include <string>
 #include <vector>
@@ -81,7 +81,8 @@ class PluginLib : public base::RefCounted<PluginLib> {
   // Loads the library now and ensures it's never unloaded.
   void EnsureAlwaysLoaded();
 
- private:
+  // protected for testability.
+ protected:
   friend class base::RefCounted<PluginLib>;
 
   // Creates a new PluginLib.
@@ -120,4 +121,4 @@ class PluginLib : public base::RefCounted<PluginLib> {
 
 }  // namespace NPAPI
 
-#endif  // WEBKIT_GLUE_PLUGIN_PLUGIN_LIB_H_
+#endif  // WEBKIT_GLUE_PLUGINS_PLUGIN_LIB_H_
diff --git a/webkit/glue/plugins/plugin_lib_unittest.cc b/webkit/glue/plugins/plugin_lib_unittest.cc
index 0ab9da3..7204629 100644
--- a/webkit/glue/plugins/plugin_lib_unittest.cc
+++ b/webkit/glue/plugins/plugin_lib_unittest.cc
@@ -7,6 +7,54 @@
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+// Test the unloading of plugin libs. Bug http://crbug.com/46526 showed that
+// if UnloadAllPlugins() simply iterates through the g_loaded_libs global
+// variable, we can get a crash if no plugin libs were marked as always loaded.
+class PluginLibTest : public NPAPI::PluginLib {
+ public:
+  PluginLibTest() : NPAPI::PluginLib(WebPluginInfo(), NULL) {
+  }
+  using NPAPI::PluginLib::Unload;
+};
+
+TEST(PluginLibLoading, UnloadAllPlugins) {
+  // For the creation of the g_loaded_libs global variable.
+  ASSERT_EQ(static_cast<PluginLibTest*>(NULL),
+      PluginLibTest::CreatePluginLib(FilePath()));
+
+  // Try with a single plugin lib.
+  scoped_refptr<PluginLibTest> plugin_lib1 = new PluginLibTest();
+  NPAPI::PluginLib::UnloadAllPlugins();
+
+  // Need to create it again, it should have been destroyed above.
+  ASSERT_EQ(static_cast<PluginLibTest*>(NULL),
+      PluginLibTest::CreatePluginLib(FilePath()));
+
+  // Try with two plugin libs.
+  plugin_lib1 = new PluginLibTest();
+  scoped_refptr<PluginLibTest> plugin_lib2 = new PluginLibTest();
+  NPAPI::PluginLib::UnloadAllPlugins();
+
+  // Need to create it again, it should have been destroyed above.
+  ASSERT_EQ(static_cast<PluginLibTest*>(NULL),
+      PluginLibTest::CreatePluginLib(FilePath()));
+
+  // Now try to manually Unload one and then UnloadAll.
+  plugin_lib1 = new PluginLibTest();
+  plugin_lib2 = new PluginLibTest();
+  plugin_lib1->Unload();
+  NPAPI::PluginLib::UnloadAllPlugins();
+
+  // Need to create it again, it should have been destroyed above.
+  ASSERT_EQ(static_cast<PluginLibTest*>(NULL),
+      PluginLibTest::CreatePluginLib(FilePath()));
+
+  // Now try to manually Unload the only one and then UnloadAll.
+  plugin_lib1 = new PluginLibTest();
+  plugin_lib1->Unload();
+  NPAPI::PluginLib::UnloadAllPlugins();
+}
+
 #if defined(OS_LINUX)
 
 // Test parsing a simple description: Real Audio.
@@ -100,5 +148,3 @@ TEST(MIMEDescriptionParse, ComplicatedJava) {
 }
 
 #endif  // defined(OS_LINUX)
-
-