When clearing plugin data at shutdown, wait for it to finish.

Also, Add some browser tests for clearing plugin data and enable npapi_test_plugin on 64-bit Linux; apparently it's been fixed.

BUG=58235, 18337, 26625
TEST=PluginDataRemoverTest.*

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

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

chrome/browser/browser_process.h

@@ -187,10 +187,21 @@ class BrowserProcess {
   virtual void SetIPCLoggingEnabled(bool enable) = 0;
 #endif
 
+  const std::string& plugin_data_remover_mime_type() const {
+    return plugin_data_remover_mime_type_;
+  }
+
+  void set_plugin_data_remover_mime_type(const std::string& mime_type) {
+    plugin_data_remover_mime_type_ = mime_type;
+  }
+
  private:
   // User-data-dir based profiles.
   std::vector<std::wstring> user_data_dir_profiles_;
 
+  // Used for testing plugin data removal at shutdown.
+  std::string plugin_data_remover_mime_type_;
+
   DISALLOW_COPY_AND_ASSIGN(BrowserProcess);
 };
 

chrome/browser/browser_process_impl.cc

@@ -544,7 +544,9 @@ void BrowserProcessImpl::Observe(NotificationType type,
       if (prefs->GetBoolean(prefs::kClearSiteDataOnExit) &&
           local_state()->GetBoolean(prefs::kClearPluginLSODataEnabled)) {
         plugin_data_remover_ = new PluginDataRemover();
-        plugin_data_remover_->StartRemoving(base::Time(), NULL);
+        if (!plugin_data_remover_mime_type().empty())
+          plugin_data_remover_->set_mime_type(plugin_data_remover_mime_type());
+        plugin_data_remover_->StartRemoving(base::Time());
       }
     }
   } else {
@@ -553,13 +555,8 @@ void BrowserProcessImpl::Observe(NotificationType type,
 }
 
 void BrowserProcessImpl::WaitForPluginDataRemoverToFinish() {
-  if (!plugin_data_remover_.get() || !plugin_data_remover_->is_removing())
-    return;
-  plugin_data_remover_->set_done_task(new MessageLoop::QuitTask());
-  base::Time start_time(base::Time::Now());
-  MessageLoop::current()->Run();
-  UMA_HISTOGRAM_TIMES("ClearPluginData.wait_at_shutdown",
-                      base::Time::Now() - start_time);
+  if (plugin_data_remover_.get())
+    plugin_data_remover_->Wait();
 }
 
 #if (defined(OS_WIN) || defined(OS_LINUX)) && !defined(OS_CHROMEOS)

chrome/browser/browsing_data_remover.cc

@@ -260,9 +260,9 @@ void BrowsingDataRemover::Remove(int remove_mask) {
     waiting_for_clear_lso_data_ = true;
     if (!plugin_data_remover_.get())
       plugin_data_remover_ = new PluginDataRemover();
-    plugin_data_remover_->StartRemoving(
-        delete_begin_,
-        NewRunnableMethod(this, &BrowsingDataRemover::OnClearedPluginData));
+    base::WaitableEvent* event =
+        plugin_data_remover_->StartRemoving(delete_begin_);
+    watcher_.StartWatching(event, this);
   }
 
   NotifyAndDeleteIfDone();
@@ -509,7 +509,8 @@ ChromeAppCacheService* BrowsingDataRemover::GetAppCacheService() {
                          : NULL;
 }
 
-void BrowsingDataRemover::OnClearedPluginData() {
+void BrowsingDataRemover::OnWaitableEventSignaled(
+    base::WaitableEvent* waitable_event) {
   waiting_for_clear_lso_data_ = false;
   NotifyAndDeleteIfDone();
 }

chrome/browser/browsing_data_remover.h

@@ -10,6 +10,7 @@
 
 #include "base/observer_list.h"
 #include "base/ref_counted.h"
+#include "base/synchronization/waitable_event_watcher.h"
 #include "base/time.h"
 #include "chrome/browser/appcache/chrome_appcache_service.h"
 #include "chrome/browser/cancelable_request.h"
@@ -30,7 +31,8 @@ class DatabaseTracker;
 // BrowsingDataRemover is responsible for removing data related to browsing:
 // visits in url database, downloads, cookies ...
 
-class BrowsingDataRemover : public NotificationObserver {
+class BrowsingDataRemover : public NotificationObserver,
+                            public base::WaitableEventWatcher::Delegate {
  public:
   // Time period ranges available when doing browsing data removals.
   enum TimePeriod {
@@ -106,6 +108,10 @@ class BrowsingDataRemover : public NotificationObserver {
                        const NotificationSource& source,
                        const NotificationDetails& details);
 
+  // WaitableEventWatcher implementation.
+  // Called when plug-in data has been cleared. Invokes NotifyAndDeleteIfDone.
+  virtual void OnWaitableEventSignaled(base::WaitableEvent* waitable_event);
+
   // If we're not waiting on anything, notifies observers and deletes this
   // object.
   void NotifyAndDeleteIfDone();
@@ -142,9 +148,6 @@ class BrowsingDataRemover : public NotificationObserver {
   void OnAppCacheDeleted(int rv);
   ChromeAppCacheService* GetAppCacheService();
 
-  // Callback when plug-in data has been cleared. Invokes NotifyAndDeleteIfDone.
-  void OnClearedPluginData();
-
   // Calculate the begin time for the deletion range specified by |time_period|.
   base::Time CalculateBeginDeleteTime(TimePeriod time_period);
 
@@ -191,6 +194,7 @@ class BrowsingDataRemover : public NotificationObserver {
 
   // Used to delete plugin data.
   scoped_refptr<PluginDataRemover> plugin_data_remover_;
+  base::WaitableEventWatcher watcher_;
 
   // True if we're waiting for various data to be deleted.
   bool waiting_for_clear_databases_;

chrome/browser/plugin_data_remover.cc

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -29,7 +29,9 @@ const uint64 kClearAllData = 0;
 }  // namespace
 
 PluginDataRemover::PluginDataRemover()
-    : is_removing_(false),
+    : mime_type_(kFlashMimeType),
+      is_removing_(false),
+      event_(new base::WaitableEvent(true, false)),
       channel_(NULL) { }
 
 PluginDataRemover::~PluginDataRemover() {
@@ -38,25 +40,37 @@ PluginDataRemover::~PluginDataRemover() {
     BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE, channel_);
 }
 
-void PluginDataRemover::StartRemoving(base::Time begin_time, Task* done_task) {
-  DCHECK(!done_task_.get());
+base::WaitableEvent* PluginDataRemover::StartRemoving(
+    const base::Time& begin_time) {
   DCHECK(!is_removing_);
   remove_start_time_ = base::Time::Now();
   begin_time_ = begin_time;
 
-  message_loop_ = base::MessageLoopProxy::CreateForCurrentThread();
-  done_task_.reset(done_task);
   is_removing_ = true;
 
   AddRef();
   PluginService::GetInstance()->OpenChannelToPlugin(
-      GURL(), kFlashMimeType, this);
+      GURL(), mime_type_, this);
 
   BrowserThread::PostDelayedTask(
       BrowserThread::IO,
       FROM_HERE,
       NewRunnableMethod(this, &PluginDataRemover::OnTimeout),
       kRemovalTimeoutMs);
+
+  return event_.get();
+}
+
+void PluginDataRemover::Wait() {
+  base::Time start_time(base::Time::Now());
+  bool result = true;
+  if (is_removing_)
+    result = event_->Wait();
+  UMA_HISTOGRAM_TIMES("ClearPluginData.wait_at_shutdown",
+                      base::Time::Now() - start_time);
+  UMA_HISTOGRAM_TIMES("ClearPluginData.time_at_shutdown",
+                      base::Time::Now() - remove_start_time_);
+  DCHECK(result) << "Error waiting for plugin process";
 }
 
 int PluginDataRemover::ID() {
@@ -87,7 +101,7 @@ void PluginDataRemover::ConnectToChannel(const IPC::ChannelHandle& handle) {
   DCHECK(!channel_);
   channel_ = new IPC::Channel(handle, IPC::Channel::MODE_CLIENT, this);
   if (!channel_->Connect()) {
-    LOG(DFATAL) << "Couldn't connect to plugin";
+    NOTREACHED() << "Couldn't connect to plugin";
     SignalDone();
     return;
   }
@@ -96,27 +110,27 @@ void PluginDataRemover::ConnectToChannel(const IPC::ChannelHandle& handle) {
           new PluginMsg_ClearSiteData(std::string(),
                                       kClearAllData,
                                       begin_time_))) {
-    LOG(DFATAL) << "Couldn't send ClearSiteData message";
+    NOTREACHED() << "Couldn't send ClearSiteData message";
     SignalDone();
+    return;
   }
 }
 
 void PluginDataRemover::OnError() {
-  NOTREACHED() << "Couldn't open plugin channel";
+  LOG(DFATAL) << "Couldn't open plugin channel";
   SignalDone();
   Release();
 }
 
 void PluginDataRemover::OnClearSiteDataResult(bool success) {
-  if (!success)
-    LOG(DFATAL) << "ClearSiteData returned error";
+  LOG_IF(DFATAL, !success) << "ClearSiteData returned error";
   UMA_HISTOGRAM_TIMES("ClearPluginData.time",
                       base::Time::Now() - remove_start_time_);
   SignalDone();
 }
 
 void PluginDataRemover::OnTimeout() {
-  NOTREACHED() << "Timed out";
+  LOG_IF(DFATAL, is_removing_) << "Timed out";
   SignalDone();
 }
 
@@ -131,8 +145,10 @@ bool PluginDataRemover::OnMessageReceived(const IPC::Message& msg) {
 }
 
 void PluginDataRemover::OnChannelError() {
-  LOG(DFATAL) << "Channel error";
-  SignalDone();
+  if (is_removing_) {
+    NOTREACHED() << "Channel error";
+    SignalDone();
+  }
 }
 
 void PluginDataRemover::SignalDone() {
@@ -140,10 +156,7 @@ void PluginDataRemover::SignalDone() {
   if (!is_removing_)
     return;
   is_removing_ = false;
-  if (done_task_.get()) {
-    message_loop_->PostTask(FROM_HERE, done_task_.release());
-    message_loop_ = NULL;
-  }
+  event_->Signal();
 }
 
 // static

chrome/browser/plugin_data_remover.h

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -15,6 +15,7 @@ class Task;
 
 namespace base {
 class MessageLoopProxy;
+class WaitableEvent;
 }
 
 class PluginDataRemover : public base::RefCountedThreadSafe<PluginDataRemover>,
@@ -23,9 +24,11 @@ class PluginDataRemover : public base::RefCountedThreadSafe<PluginDataRemover>,
  public:
   PluginDataRemover();
 
-  // Starts removing plug-in data stored since |begin_time|. If |done_task| is
-  // not NULL, it is run on the current thread when removing has finished.
-  void StartRemoving(base::Time begin_time, Task* done_task);
+  // Used in tests to call a different plugin.
+  void set_mime_type(const std::string& mime_type) { mime_type_ = mime_type; }
+
+  // Starts removing plug-in data stored since |begin_time|.
+  base::WaitableEvent* StartRemoving(const base::Time& begin_time);
 
   // Returns whether there is a plug-in installed that supports removing
   // LSO data. Because this method possibly has to load the plug-in list, it
@@ -34,9 +37,10 @@ class PluginDataRemover : public base::RefCountedThreadSafe<PluginDataRemover>,
 
   bool is_removing() const { return is_removing_; }
 
-  // Sets the task to run when removing has finished. Takes ownership of
-  // the passed task.
-  void set_done_task(Task* task) { done_task_.reset(task); }
+  // Wait until removing has finished. When the browser is still running (i.e.
+  // not during shutdown), you should use a WaitableEventWatcher in combination
+  // with the WaitableEvent returned by StartRemoving.
+  void Wait();
 
   // PluginProcessHost::Client methods
   virtual int ID();
@@ -51,6 +55,7 @@ class PluginDataRemover : public base::RefCountedThreadSafe<PluginDataRemover>,
 
  private:
   friend class base::RefCountedThreadSafe<PluginDataRemover>;
+  friend class PluginDataRemoverTest;
   ~PluginDataRemover();
 
   void SignalDone();
@@ -58,13 +63,13 @@ class PluginDataRemover : public base::RefCountedThreadSafe<PluginDataRemover>,
   void OnClearSiteDataResult(bool success);
   void OnTimeout();
 
-  scoped_refptr<base::MessageLoopProxy> message_loop_;
+  std::string mime_type_;
   bool is_removing_;
-  scoped_ptr<Task> done_task_;
   // The point in time when we start removing data.
   base::Time remove_start_time_;
   // The point in time from which on we remove data.
   base::Time begin_time_;
+  scoped_ptr<base::WaitableEvent> event_;
   // We own the channel, but it's used on the IO thread, so it needs to be
   // deleted there as well.
   IPC::Channel* channel_;

chrome/browser/plugin_data_remover_browsertest.cc

+// 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 "chrome/browser/plugin_data_remover.h"
+
+#include "base/command_line.h"
+#include "base/path_service.h"
+#include "base/synchronization/waitable_event_watcher.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/in_process_browser_test.h"
+#include "chrome/test/ui_test_utils.h"
+
+namespace {
+const char* kNPAPITestPluginMimeType = "application/vnd.npapi-test";
+}
+
+class PluginDataRemoverTest : public InProcessBrowserTest,
+                              public base::WaitableEventWatcher::Delegate {
+ public:
+  PluginDataRemoverTest() : InProcessBrowserTest() { }
+
+  virtual void SetUpOnMainThread() {
+    old_plugin_data_remover_mime_type_ =
+        g_browser_process->plugin_data_remover_mime_type();
+    g_browser_process->set_plugin_data_remover_mime_type(
+        kNPAPITestPluginMimeType);
+  }
+
+  virtual void TearDownOnMainThread() {
+    g_browser_process->set_plugin_data_remover_mime_type(
+        old_plugin_data_remover_mime_type_);
+  }
+
+  virtual void OnWaitableEventSignaled(base::WaitableEvent* waitable_event) {
+    MessageLoop::current()->Quit();
+  }
+
+  virtual void SetUpCommandLine(CommandLine* command_line) {
+#ifdef OS_MACOSX
+    FilePath browser_directory;
+    PathService::Get(chrome::DIR_APP, &browser_directory);
+    command_line->AppendSwitchPath(switches::kExtraPluginDir,
+                                   browser_directory.AppendASCII("plugins"));
+#endif
+
+//    command_line->AppendSwitch(switches::kPluginStartupDialog);
+  }
+
+ private:
+  std::string old_plugin_data_remover_mime_type_;
+};
+
+IN_PROC_BROWSER_TEST_F(PluginDataRemoverTest, RemoveData) {
+  scoped_refptr<PluginDataRemover> plugin_data_remover(new PluginDataRemover());
+  plugin_data_remover->set_mime_type(kNPAPITestPluginMimeType);
+  base::WaitableEventWatcher watcher;
+  base::WaitableEvent* event =
+      plugin_data_remover->StartRemoving(base::Time());
+  watcher.StartWatching(event, this);
+  ui_test_utils::RunMessageLoop();
+}
+
+IN_PROC_BROWSER_TEST_F(PluginDataRemoverTest, AtShutdown) {
+  browser()->profile()->GetPrefs()->SetBoolean(
+      prefs::kClearSiteDataOnExit, true);
+  g_browser_process->local_state()->SetBoolean(
+      prefs::kClearPluginLSODataEnabled, true);
+}

chrome/chrome_tests.gypi

@@ -373,7 +373,7 @@
             'test/interactive_ui/npapi_interactive_test.cc',
           ],
         }],
-        ['target_arch!="x64" and target_arch!="arm"', {
+        ['target_arch!="arm"', {
           'dependencies': [
             # run time dependency
             '../webkit/webkit.gyp:npapi_test_plugin',
@@ -554,8 +554,7 @@
         'worker/worker_uitest.cc',
       ],
       'conditions': [
-        # http://code.google.com/p/chromium/issues/detail?id=18337
-        ['target_arch!="x64" and target_arch!="arm"', {
+        ['target_arch!="arm"', {
           'dependencies': [
             '../webkit/webkit.gyp:copy_npapi_test_plugin',
           ],
@@ -2131,6 +2130,7 @@
         'browser/in_process_webkit/indexed_db_browsertest.cc',
         'browser/net/cookie_policy_browsertest.cc',
         'browser/net/ftp_browsertest.cc',
+        'browser/plugin_data_remover_browsertest.cc',
         'browser/plugin_service_browsertest.cc',
         'browser/policy/device_management_backend_mock.cc',
         'browser/policy/device_management_backend_mock.h',
@@ -2327,6 +2327,12 @@
             'browser/ui/views/html_dialog_view_browsertest.cc',
           ],
         }],
+        ['target_arch!="arm"', {
+          'dependencies': [
+            # run time dependency
+            '../webkit/webkit.gyp:copy_npapi_test_plugin',
+          ],
+        }],  
       ],  # conditions
     },  # target browser_tests
     {

webkit/plugins/npapi/test/plugin_client.cc

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -35,6 +35,7 @@ NPError PluginClient::GetEntryPoints(NPPluginFuncs* pFuncs) {
   pFuncs->setvalue      = NPP_SetValue;
   pFuncs->javaClass     = NULL;
   pFuncs->urlredirectnotify = NPP_URLRedirectNotify;
+  pFuncs->clearsitedata = NPP_ClearSiteData;
 
   return NPERR_NO_ERROR;
 }
@@ -237,4 +238,11 @@ void NPP_URLRedirectNotify(NPP instance, const char* url, int32_t status,
     plugin->URLRedirectNotify(url, status, notify_data);
   }
 }
+
+NPError NPP_ClearSiteData(const char* site,
+                          uint64 flags,
+                          uint64 max_age) {
+  LOG(INFO) << "NPP_ClearSiteData called";
+  return NPERR_NO_ERROR;
+}
 } // extern "C"

webkit/tools/test_shell/test_shell.gypi

@@ -122,8 +122,7 @@
         '<(DEPTH)/webkit/support/webkit_support.gyp:glue',
       ],
       'conditions': [
-        # http://code.google.com/p/chromium/issues/detail?id=18337
-        ['target_arch!="x64" and target_arch!="arm"', {
+        ['target_arch!="arm"', {
           'dependencies': [
             'copy_npapi_test_plugin',
           ],
@@ -504,7 +503,7 @@
     },
   ],
   'conditions': [
-    ['target_arch!="x64" and target_arch!="arm"', {
+    ['target_arch!="arm"', {
       'targets': [
         {
           'target_name': 'npapi_test_common',