Pepper: Fix a pile of bugs in the implementation of the file chooser API.

This fixes:
  - a crash, due to holding a plain old pointer to a ref-counted object;
  - a DCHECK() failure, due to using FilePath::AppendASCII();
  - incorrect/non-handling of PP_ERROR_INPROGRESS;
  - non-initialization of PPB_FileChooser_Impl::next_chosen_file_index_;
  - non-clearing of PPB_FileChooser_Impl::chosen_files_ upon Show() being called
    (e.g., a second time);
  - not aborting callbacks on resource deletion.
Probably something else too.

BUG=none
TEST=Trung is a bit happier

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

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

4 files changed, 78 insertions, 28 deletions

diff --git a/webkit/plugins/ppapi/ppb_file_chooser_impl.cc b/webkit/plugins/ppapi/ppb_file_chooser_impl.cc
index 0c1cc06..370fd44b 100644
--- a/webkit/plugins/ppapi/ppb_file_chooser_impl.cc
+++ b/webkit/plugins/ppapi/ppb_file_chooser_impl.cc
@@ -1,4 +1,4 @@
-// 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.
 
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/logging.h"
+#include "base/sys_string_conversions.h"
 #include "ppapi/c/pp_completion_callback.h"
 #include "ppapi/c/pp_errors.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebCString.h"
@@ -15,9 +16,11 @@
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFileChooserParams.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebString.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebVector.h"
+#include "webkit/plugins/ppapi/callbacks.h"
 #include "webkit/plugins/ppapi/common.h"
 #include "webkit/plugins/ppapi/ppb_file_ref_impl.h"
 #include "webkit/plugins/ppapi/plugin_delegate.h"
+#include "webkit/plugins/ppapi/plugin_module.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
 #include "webkit/plugins/ppapi/resource_tracker.h"
 #include "webkit/glue/webkit_glue.h"
@@ -92,13 +95,13 @@ class FileChooserCompletionImpl : public WebFileChooserCompletion {
   virtual void didChooseFile(const WebVector<WebString>& file_names) {
     std::vector<std::string> files;
     for (size_t i = 0; i < file_names.size(); i++)
-      files.push_back(file_names[i].utf8().data());
+      files.push_back(file_names[i].utf8());
 
     file_chooser_->StoreChosenFiles(files);
   }
 
  private:
-  PPB_FileChooser_Impl* file_chooser_;
+  scoped_refptr<PPB_FileChooser_Impl> file_chooser_;
 };
 
 }  // namespace
@@ -108,8 +111,9 @@ PPB_FileChooser_Impl::PPB_FileChooser_Impl(
     const PP_FileChooserOptions_Dev* options)
     : Resource(instance),
       mode_(options->mode),
-      accept_mime_types_(options->accept_mime_types),
-      completion_callback_() {
+      accept_mime_types_(options->accept_mime_types ?
+                         options->accept_mime_types : ""),
+      next_chosen_file_index_(0) {
 }
 
 PPB_FileChooser_Impl::~PPB_FileChooser_Impl() {
@@ -126,35 +130,71 @@ PPB_FileChooser_Impl* PPB_FileChooser_Impl::AsPPB_FileChooser_Impl() {
 
 void PPB_FileChooser_Impl::StoreChosenFiles(
     const std::vector<std::string>& files) {
+  chosen_files_.clear();
   next_chosen_file_index_ = 0;
-  std::vector<std::string>::const_iterator end_it = files.end();
   for (std::vector<std::string>::const_iterator it = files.begin();
-       it != end_it; it++) {
+       it != files.end(); ++it) {
+#if defined(OS_WIN)
+    FilePath file_path(base::SysUTF8ToWide(*it));
+#else
+    FilePath file_path(*it);
+#endif
+
     chosen_files_.push_back(make_scoped_refptr(
-        new PPB_FileRef_Impl(instance(), FilePath().AppendASCII(*it))));
+        new PPB_FileRef_Impl(instance(), file_path)));
   }
 
-  if (!completion_callback_.func)
-    return;
+  RunCallback(PP_OK);
+}
+
+int32_t PPB_FileChooser_Impl::ValidateCallback(
+    const PP_CompletionCallback& callback) {
+  // We only support non-blocking calls.
+  if (!callback.func)
+    return PP_ERROR_BADARGUMENT;
+
+  if (callback_.get() && !callback_->completed())
+    return PP_ERROR_INPROGRESS;
+
+  return PP_OK;
+}
+
+void PPB_FileChooser_Impl::RegisterCallback(
+    const PP_CompletionCallback& callback) {
+  DCHECK(callback.func);
+  DCHECK(!callback_.get() || callback_->completed());
+
+  PP_Resource resource_id = GetReferenceNoAddRef();
+  CHECK(resource_id);
+  callback_ = new TrackedCompletionCallback(
+      instance()->module()->GetCallbackTracker(), resource_id, callback);
+}
 
-  PP_CompletionCallback callback = {0};
-  std::swap(callback, completion_callback_);
-  PP_RunCompletionCallback(&callback, 0);
+void PPB_FileChooser_Impl::RunCallback(int32_t result) {
+  scoped_refptr<TrackedCompletionCallback> callback;
+  callback.swap(callback_);
+  callback->Run(result);  // Will complete abortively if necessary.
 }
 
-int32_t PPB_FileChooser_Impl::Show(PP_CompletionCallback callback) {
+int32_t PPB_FileChooser_Impl::Show(const PP_CompletionCallback& callback) {
+  int32_t rv = ValidateCallback(callback);
+  if (rv != PP_OK)
+    return rv;
+
   DCHECK((mode_ == PP_FILECHOOSERMODE_OPEN) ||
          (mode_ == PP_FILECHOOSERMODE_OPENMULTIPLE));
-  DCHECK(!completion_callback_.func);
-  completion_callback_ = callback;
 
   WebFileChooserParams params;
   params.multiSelect = (mode_ == PP_FILECHOOSERMODE_OPENMULTIPLE);
   params.acceptTypes = WebString::fromUTF8(accept_mime_types_);
   params.directory = false;
 
-  return instance()->delegate()->RunFileChooser(
-      params, new FileChooserCompletionImpl(this));
+  if (!instance()->delegate()->RunFileChooser(params,
+          new FileChooserCompletionImpl(this)))
+    return PP_ERROR_FAILED;
+
+  RegisterCallback(callback);
+  return PP_ERROR_WOULDBLOCK;
 }
 
 scoped_refptr<PPB_FileRef_Impl> PPB_FileChooser_Impl::GetNextChosenFile() {
@@ -166,4 +206,3 @@ scoped_refptr<PPB_FileRef_Impl> PPB_FileChooser_Impl::GetNextChosenFile() {
 
 }  // namespace ppapi
 }  // namespace webkit
-
diff --git a/webkit/plugins/ppapi/ppb_file_chooser_impl.h b/webkit/plugins/ppapi/ppb_file_chooser_impl.h
index 1d4c7de..93004d0 100644
--- a/webkit/plugins/ppapi/ppb_file_chooser_impl.h
+++ b/webkit/plugins/ppapi/ppb_file_chooser_impl.h
@@ -1,4 +1,4 @@
-// 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.
 
@@ -8,17 +8,18 @@
 #include <string>
 #include <vector>
 
-#include "base/scoped_ptr.h"
+#include "base/ref_counted.h"
 #include "ppapi/c/dev/ppb_file_chooser_dev.h"
-#include "ppapi/c/pp_completion_callback.h"
 #include "webkit/plugins/ppapi/resource.h"
 
+struct PP_CompletionCallback;
+
 namespace webkit {
 namespace ppapi {
 
-class PluginDelegate;
 class PluginInstance;
 class PPB_FileRef_Impl;
+class TrackedCompletionCallback;
 
 class PPB_FileChooser_Impl : public Resource {
  public:
@@ -36,14 +37,25 @@ class PPB_FileChooser_Impl : public Resource {
   // Stores the list of selected files.
   void StoreChosenFiles(const std::vector<std::string>& files);
 
+  // Check that |callback| is valid (only non-blocking operation is supported)
+  // and that no callback is already pending. Returns |PP_OK| if okay, else
+  // |PP_ERROR_...| to be returned to the plugin.
+  int32_t ValidateCallback(const PP_CompletionCallback& callback);
+
+  // Sets up |callback| as the pending callback. This should only be called once
+  // it is certain that |PP_ERROR_WOULDBLOCK| will be returned.
+  void RegisterCallback(const PP_CompletionCallback& callback);
+
+  void RunCallback(int32_t result);
+
   // PPB_FileChooser implementation.
-  int32_t Show(PP_CompletionCallback callback);
+  int32_t Show(const PP_CompletionCallback& callback);
   scoped_refptr<PPB_FileRef_Impl> GetNextChosenFile();
 
  private:
   PP_FileChooserMode_Dev mode_;
   std::string accept_mime_types_;
-  PP_CompletionCallback completion_callback_;
+  scoped_refptr<TrackedCompletionCallback> callback_;
   std::vector< scoped_refptr<PPB_FileRef_Impl> > chosen_files_;
   size_t next_chosen_file_index_;
 };
diff --git a/webkit/plugins/ppapi/ppb_file_io_impl.cc b/webkit/plugins/ppapi/ppb_file_io_impl.cc
index fab8c7b..6597aee 100644
--- a/webkit/plugins/ppapi/ppb_file_io_impl.cc
+++ b/webkit/plugins/ppapi/ppb_file_io_impl.cc
@@ -437,7 +437,7 @@ void PPB_FileIO_Impl::RegisterCallback(PP_CompletionCallback callback) {
       instance()->module()->GetCallbackTracker(), resource_id, callback);
 }
 
-void PPB_FileIO_Impl::RunPendingCallback(int result) {
+void PPB_FileIO_Impl::RunPendingCallback(int32_t result) {
   scoped_refptr<TrackedCompletionCallback> callback;
   callback.swap(callback_);
   callback->Run(result);  // Will complete abortively if necessary.
diff --git a/webkit/plugins/ppapi/ppb_file_io_impl.h b/webkit/plugins/ppapi/ppb_file_io_impl.h
index 3bc2742..29ed237 100644
--- a/webkit/plugins/ppapi/ppb_file_io_impl.h
+++ b/webkit/plugins/ppapi/ppb_file_io_impl.h
@@ -12,7 +12,6 @@
 #include "base/scoped_callback_factory.h"
 #include "base/scoped_ptr.h"
 #include "ppapi/c/dev/pp_file_info_dev.h"
-#include "ppapi/c/pp_completion_callback.h"
 #include "ppapi/c/pp_time.h"
 #include "webkit/plugins/ppapi/callbacks.h"
 #include "webkit/plugins/ppapi/plugin_delegate.h"
@@ -88,7 +87,7 @@ class PPB_FileIO_Impl : public Resource {
   // it is certain that |PP_ERROR_WOULDBLOCK| will be returned.
   void RegisterCallback(PP_CompletionCallback callback);
 
-  void RunPendingCallback(int result);
+  void RunPendingCallback(int32_t result);
 
   void StatusCallback(base::PlatformFileError error_code);
   void AsyncOpenFileCallback(base::PlatformFileError error_code,