Bug 1677168 - nsFilePicker cleanup

2 files changed, 107 insertions, 275 deletions

diff --git a/system/interface/windows/nsFilePicker.cpp b/system/interface/windows/nsFilePicker.cpp
index 59ae152ec..3d733fcb7 100644
--- a/system/interface/windows/nsFilePicker.cpp
+++ b/system/interface/windows/nsFilePicker.cpp
@@ -35,12 +35,14 @@ using mozilla::mscom::EnsureMTA;
 using mozilla::UniquePtr;
 using namespace mozilla::widget;
 
-char16_t *nsFilePicker::mLastUsedUnicodeDirectory;
+UniquePtr<char16_t[], nsFilePicker::FreeDeleter>
+    nsFilePicker::sLastUsedUnicodeDirectory;
+
 char nsFilePicker::mLastUsedDirectory[MAX_PATH+1] = { 0 };
 
 static const wchar_t kDialogPtrProp[] = L"DialogPtrProperty";
 static const DWORD kDialogTimerID = 9999;
-static const unsigned long kDialogTimerTimeout = 300;
+
 
 #define MAX_EXTENSION_LENGTH 10
 #define FILE_BUFFER_SIZE     4096 
@@ -71,31 +73,6 @@ private:
   RefPtr<nsWindow> mWindow;
 };
 
-// Manages the current working path.
-class AutoRestoreWorkingPath
-{
-public:
-  AutoRestoreWorkingPath() {
-    DWORD bufferLength = GetCurrentDirectoryW(0, nullptr);
-    mWorkingPath = MakeUnique<wchar_t[]>(bufferLength);
-    if (GetCurrentDirectoryW(bufferLength, mWorkingPath.get()) == 0) {
-      mWorkingPath = nullptr;
-    }
-  }
-
-  ~AutoRestoreWorkingPath() {
-    if (HasWorkingPath()) {
-      ::SetCurrentDirectoryW(mWorkingPath.get());
-    }
-  }
-
-  inline bool HasWorkingPath() const {
-    return mWorkingPath != nullptr;
-  }
-private:
-  UniquePtr<wchar_t[]> mWorkingPath;
-};
-
 // Manages NS_NATIVE_TMP_WINDOW child windows. NS_NATIVE_TMP_WINDOWs are
 // temporary child windows of mParentWidget created to address RTL issues
 // in picker dialogs. We are responsible for destroying these.
@@ -140,57 +117,11 @@ private:
   RefPtr<nsWindow> mWindow;
 };
 
-// Manages a simple callback timer
-class AutoTimerCallbackCancel
-{
-public:
-  AutoTimerCallbackCancel(nsFilePicker* aTarget,
-                          nsTimerCallbackFunc aCallbackFunc) {
-    Init(aTarget, aCallbackFunc);
-  }
-
-  ~AutoTimerCallbackCancel() {
-    if (mPickerCallbackTimer) {
-      mPickerCallbackTimer->Cancel();
-    }
-  }
-
-private:
-  void Init(nsFilePicker* aTarget,
-            nsTimerCallbackFunc aCallbackFunc) {
-    mPickerCallbackTimer = do_CreateInstance("@mozilla.org/timer;1");
-    if (!mPickerCallbackTimer) {
-      NS_WARNING("do_CreateInstance for timer failed??");
-      return;
-    }
-    mPickerCallbackTimer->InitWithFuncCallback(aCallbackFunc,
-                                               aTarget,
-                                               kDialogTimerTimeout,
-                                               nsITimer::TYPE_REPEATING_SLACK);
-  }
-  nsCOMPtr<nsITimer> mPickerCallbackTimer;
-    
-};
-
 ///////////////////////////////////////////////////////////////////////////////
 // nsIFilePicker
 
-nsFilePicker::nsFilePicker() :
-  mSelectedType(1)
-  , mDlgWnd(nullptr)
-  , mFDECookie(0)
-{
-   CoInitialize(nullptr);
-}
-
-nsFilePicker::~nsFilePicker()
-{
-  if (mLastUsedUnicodeDirectory) {
-    free(mLastUsedUnicodeDirectory);
-    mLastUsedUnicodeDirectory = nullptr;
-  }
-  CoUninitialize();
-}
+nsFilePicker::nsFilePicker()
+    : mSelectedType(1) {}
 
 NS_IMPL_ISUPPORTS(nsFilePicker, nsIFilePicker)
 
@@ -205,130 +136,6 @@ NS_IMETHODIMP nsFilePicker::Init(mozIDOMWindowProxy *aParent, const nsAString& a
   return nsBaseFilePicker::Init(aParent, aTitle, aMode);
 }
 
-STDMETHODIMP nsFilePicker::QueryInterface(REFIID refiid, void** ppvResult)
-{
-  *ppvResult = nullptr;
-  if (IID_IUnknown == refiid ||
-      refiid == IID_IFileDialogEvents) {
-    *ppvResult = this;
-  }
-
-  if (nullptr != *ppvResult) {
-    ((LPUNKNOWN)*ppvResult)->AddRef();
-    return S_OK;
-  }
-
-  return E_NOINTERFACE;
-}
-
-
-/*
- * Callbacks
- */
-
-HRESULT
-nsFilePicker::OnFileOk(IFileDialog *pfd)
-{
-  return S_OK;
-}
-
-HRESULT
-nsFilePicker::OnFolderChanging(IFileDialog *pfd,
-                               IShellItem *psiFolder)
-{
-  return S_OK;
-}
-
-HRESULT
-nsFilePicker::OnFolderChange(IFileDialog *pfd)
-{
-  return S_OK;
-}
-
-HRESULT
-nsFilePicker::OnSelectionChange(IFileDialog *pfd)
-{
-  return S_OK;
-}
-
-HRESULT
-nsFilePicker::OnShareViolation(IFileDialog *pfd,
-                               IShellItem *psi,
-                               FDE_SHAREVIOLATION_RESPONSE *pResponse)
-{
-  return S_OK;
-}
-
-HRESULT
-nsFilePicker::OnTypeChange(IFileDialog *pfd)
-{
-  // Failures here result in errors due to security concerns.
-  RefPtr<IOleWindow> win;
-  pfd->QueryInterface(IID_IOleWindow, getter_AddRefs(win));
-  if (!win) {
-    NS_ERROR("Could not retrieve the IOleWindow interface for IFileDialog.");
-    return S_OK;
-  }
-  HWND hwnd = nullptr;
-  win->GetWindow(&hwnd);
-  if (!hwnd) {
-    NS_ERROR("Could not retrieve the HWND for IFileDialog.");
-    return S_OK;
-  }
-  
-  SetDialogHandle(hwnd);
-  return S_OK;
-}
-
-HRESULT
-nsFilePicker::OnOverwrite(IFileDialog *pfd,
-                          IShellItem *psi,
-                          FDE_OVERWRITE_RESPONSE *pResponse)
-{
-  return S_OK;
-}
-
-/*
- * Close on parent close logic
- */
-
-bool
-nsFilePicker::ClosePickerIfNeeded()
-{
-  if (!mParentWidget || !mDlgWnd)
-    return false;
-
-  nsWindow *win = static_cast<nsWindow *>(mParentWidget.get());
-  if (IsWindow(mDlgWnd) && IsWindowVisible(mDlgWnd) && win->DestroyCalled()) {
-    wchar_t className[64];
-    // Make sure we have the right window
-    if (GetClassNameW(mDlgWnd, className, mozilla::ArrayLength(className)) &&
-        !wcscmp(className, L"#32770") &&
-        DestroyWindow(mDlgWnd)) {
-      mDlgWnd = nullptr;
-      return true;
-    }
-  }
-  return false;
-}
-
-void
-nsFilePicker::PickerCallbackTimerFunc(nsITimer *aTimer, void *aCtx)
-{
-  nsFilePicker* picker = (nsFilePicker*)aCtx;
-  if (picker->ClosePickerIfNeeded()) {
-    aTimer->Cancel();
-  }
-}
-
-void
-nsFilePicker::SetDialogHandle(HWND aWnd)
-{
-  if (!aWnd || mDlgWnd)
-    return;
-  mDlgWnd = aWnd;
-}
-
 /*
  * Folder picker invocation
  */
@@ -353,15 +160,14 @@ nsFilePicker::ShowFolderPicker(const nsString& aInitialDir)
   }
 
   RefPtr<IFileOpenDialog> dialog;
-  if (FAILED(CoCreateInstance(CLSID_FileOpenDialog, nullptr, CLSCTX_INPROC,
+  if (FAILED(CoCreateInstance(CLSID_FileOpenDialog,
+                              nullptr,
+                              CLSCTX_INPROC_SERVER,
                               IID_IFileOpenDialog,
                               getter_AddRefs(dialog)))) {
     return false;
   }
 
-  // hook up event callbacks
-  dialog->Advise(this, &mFDECookie);
-
   // options
   FILEOPENDIALOGOPTIONS fos = FOS_PICKFOLDERS;
   // Require interaction if the folder picker is triggered by an element that
@@ -371,13 +177,22 @@ nsFilePicker::ShowFolderPicker(const nsString& aInitialDir)
     fos |= FOS_OKBUTTONNEEDSINTERACTION;
   }
 
-  dialog->SetOptions(fos);
+  HRESULT hr = dialog->SetOptions(fos);
+  if (FAILED(hr)) {
+    return false;
+  }
  
   // initial strings
-  dialog->SetTitle(mTitle.get());
+  hr = dialog->SetTitle(mTitle.get());
+  if (FAILED(hr)) {
+    return false;
+  }
 
   if (!mOkButtonLabel.IsEmpty()) {
-    dialog->SetOkButtonLabel(mOkButtonLabel.get());
+    hr = dialog->SetOkButtonLabel(mOkButtonLabel.get());
+    if (FAILED(hr)) {
+      return false;
+    }
   }
 
   if (!aInitialDir.IsEmpty()) {
@@ -386,7 +201,10 @@ nsFilePicker::ShowFolderPicker(const nsString& aInitialDir)
           WinUtils::SHCreateItemFromParsingName(aInitialDir.get(), nullptr,
                                                 IID_IShellItem,
                                                 getter_AddRefs(folder)))) {
-      dialog->SetFolder(folder);
+      hr = dialog->SetFolder(folder);
+      if (FAILED(hr)) {
+        return false;
+      }
     }
   }
 
@@ -398,10 +216,8 @@ nsFilePicker::ShowFolderPicker(const nsString& aInitialDir)
   if (FAILED(dialog->Show(adtw.get())) ||
       FAILED(dialog->GetResult(getter_AddRefs(item))) ||
       !item) {
-    dialog->Unadvise(mFDECookie);
     return false;
   }
-  dialog->Unadvise(mFDECookie);
 
   // results
 
@@ -409,8 +225,14 @@ nsFilePicker::ShowFolderPicker(const nsString& aInitialDir)
   // default save folder.
   RefPtr<IShellItem> folderPath;
   RefPtr<IShellLibrary> shellLib;
-  CoCreateInstance(CLSID_ShellLibrary, nullptr, CLSCTX_INPROC,
-                   IID_IShellLibrary, getter_AddRefs(shellLib));
+  if (FAILED(CoCreateInstance(CLSID_ShellLibrary,
+                              nullptr,
+                              CLSCTX_INPROC_SERVER,
+                              IID_IShellLibrary,
+                              getter_AddRefs(shellLib)))) {
+    return false;
+  }
+
   if (shellLib &&
       SUCCEEDED(shellLib->LoadLibraryFromItem(item, STGM_READ)) &&
       SUCCEEDED(shellLib->GetDefaultSaveFolder(DSFT_DETECT, IID_IShellItem,
@@ -449,22 +271,23 @@ nsFilePicker::ShowFilePicker(const nsString& aInitialDir)
 
   RefPtr<IFileDialog> dialog;
   if (mMode != modeSave) {
-    if (FAILED(CoCreateInstance(CLSID_FileOpenDialog, nullptr, CLSCTX_INPROC,
+    if (FAILED(CoCreateInstance(CLSID_FileOpenDialog,
+                                nullptr,
+                                CLSCTX_INPROC_SERVER,
                                 IID_IFileOpenDialog,
                                 getter_AddRefs(dialog)))) {
       return false;
     }
   } else {
-    if (FAILED(CoCreateInstance(CLSID_FileSaveDialog, nullptr, CLSCTX_INPROC,
+    if (FAILED(CoCreateInstance(CLSID_FileSaveDialog,
+                                nullptr,
+                                CLSCTX_INPROC_SERVER,
                                 IID_IFileSaveDialog,
                                 getter_AddRefs(dialog)))) {
       return false;
     }
   }
 
-  // hook up event callbacks
-  dialog->Advise(this, &mFDECookie);
-
   // options
 
   FILEOPENDIALOGOPTIONS fos = 0;
@@ -483,10 +306,6 @@ nsFilePicker::ShowFilePicker(const nsString& aInitialDir)
     fos |= FOS_DONTADDTORECENT;
   }
 
-  // Msdn claims FOS_NOCHANGEDIR is not needed. We'll add this
-  // just in case.
-  AutoRestoreWorkingPath arw;
-
   // mode specific
   switch(mMode) {
     case modeOpen:
@@ -506,25 +325,45 @@ nsFilePicker::ShowFilePicker(const nsString& aInitialDir)
       break;
   }
 
-  dialog->SetOptions(fos);
+  HRESULT hr = dialog->SetOptions(fos);
+  if (FAILED(hr)) {
+    return false;
+  }
 
   // initial strings
 
   // title
-  dialog->SetTitle(mTitle.get());
+  hr = dialog->SetTitle(mTitle.get());
+  if (FAILED(hr)) {
+    return false;
+  }
 
   // default filename
   if (!mDefaultFilename.IsEmpty()) {
-    dialog->SetFileName(mDefaultFilename.get());
+    // Prevent the shell from expanding environment variables by removing
+    // the % characters that are used to delimit them.
+    nsAutoString sanitizedFilename(mDefaultFilename);
+    sanitizedFilename.ReplaceChar('%', '_');
+
+    hr = dialog->SetFileName(sanitizedFilename.get());
+    if (FAILED(hr)) {
+      return false;
+    }
   }
   
   NS_NAMED_LITERAL_STRING(htmExt, "html");
 
   // default extension to append to new files
   if (!mDefaultExtension.IsEmpty()) {
-    dialog->SetDefaultExtension(mDefaultExtension.get());
+    hr = dialog->SetDefaultExtension(mDefaultExtension.get());
+    if (FAILED(hr)) {
+      return false;
+    }
   } else if (IsDefaultPathHtml()) {
-    dialog->SetDefaultExtension(htmExt.get());
+    hr = dialog->SetDefaultExtension(htmExt.get());
+    if (FAILED(hr)) {
+      return false;
+    }
   }
 
   // initial location
@@ -534,14 +373,24 @@ nsFilePicker::ShowFilePicker(const nsString& aInitialDir)
           WinUtils::SHCreateItemFromParsingName(aInitialDir.get(), nullptr,
                                                 IID_IShellItem,
                                                 getter_AddRefs(folder)))) {
-      dialog->SetFolder(folder);
+      hr = dialog->SetFolder(folder);
+      if (FAILED(hr)) {
+        return false;
+      }
     }
   }
 
   // filter types and the default index
   if (!mComFilterList.IsEmpty()) {
-    dialog->SetFileTypes(mComFilterList.Length(), mComFilterList.get());
-    dialog->SetFileTypeIndex(mSelectedType);
+    hr = dialog->SetFileTypes(mComFilterList.Length(), mComFilterList.get());
+    if (FAILED(hr)) {
+      return false;
+    }
+
+    hr = dialog->SetFileTypeIndex(mSelectedType);
+    if (FAILED(hr)) {
+      return false;
+    }
   }
 
   // display
@@ -549,14 +398,11 @@ nsFilePicker::ShowFilePicker(const nsString& aInitialDir)
   {
     AutoDestroyTmpWindow adtw((HWND)(mParentWidget.get() ?
       mParentWidget->GetNativeData(NS_NATIVE_TMP_WINDOW) : nullptr));
-    AutoTimerCallbackCancel atcc(this, PickerCallbackTimerFunc);
     AutoWidgetPickerState awps(mParentWidget);
 
     if (FAILED(dialog->Show(adtw.get()))) {
-      dialog->Unadvise(mFDECookie);
       return false;
     }
-    dialog->Unadvise(mFDECookie);
   }
 
   // results
@@ -596,9 +442,10 @@ nsFilePicker::ShowFilePicker(const nsString& aInitialDir)
     if (SUCCEEDED(items->GetItemAt(idx, getter_AddRefs(item)))) {
       if (!WinUtils::GetShellItemPath(item, str))
         continue;
-      nsCOMPtr<nsIFile> file = do_CreateInstance("@mozilla.org/file/local;1");
-      if (file && NS_SUCCEEDED(file->InitWithPath(str)))
+      nsCOMPtr<nsIFile> file;
+      if (NS_SUCCEEDED(NS_NewLocalFile(str, false, getter_AddRefs(file)))) {
         mFiles.AppendObject(file);
+      }
     }
   }
   return true;
@@ -623,7 +470,7 @@ nsFilePicker::ShowW(int16_t *aReturnVal)
   // If no display directory, re-use the last one.
   if(initialDir.IsEmpty()) {
     // Allocate copy of last used dir.
-    initialDir = mLastUsedUnicodeDirectory;
+    initialDir = sLastUsedUnicodeDirectory.get();
   }
 
   // Clear previous file selections
@@ -651,10 +498,11 @@ nsFilePicker::ShowW(int16_t *aReturnVal)
   if (mMode == modeSave) {
     // Windows does not return resultReplace, we must check if file
     // already exists.
-    nsCOMPtr<nsIFile> file(do_CreateInstance("@mozilla.org/file/local;1"));
+    nsCOMPtr<nsIFile> file;
+    nsresult rv = NS_NewLocalFile(mUnicodeFile, false, getter_AddRefs(file));
+
     bool flag = false;
-    if (file && NS_SUCCEEDED(file->InitWithPath(mUnicodeFile)) &&
-        NS_SUCCEEDED(file->Exists(&flag)) && flag) {
+    if (NS_SUCCEEDED(rv) && NS_SUCCEEDED(file->Exists(&flag)) && flag) {
       retValue = returnReplace;
     }
   }
@@ -678,14 +526,13 @@ nsFilePicker::GetFile(nsIFile **aFile)
   if (mUnicodeFile.IsEmpty())
       return NS_OK;
 
-  nsCOMPtr<nsIFile> file(do_CreateInstance("@mozilla.org/file/local;1"));
-    
-  NS_ENSURE_TRUE(file, NS_ERROR_FAILURE);
-
-  file->InitWithPath(mUnicodeFile);
-
-  NS_ADDREF(*aFile = file);
+  nsCOMPtr<nsIFile> file;
+  nsresult rv = NS_NewLocalFile(mUnicodeFile, false, getter_AddRefs(file));
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
 
+  file.forget(aFile);
   return NS_OK;
 }
 
@@ -800,8 +647,13 @@ nsFilePicker::AppendFilter(const nsAString& aTitle, const nsAString& aFilter)
 void
 nsFilePicker::RememberLastUsedDirectory()
 {
-  nsCOMPtr<nsIFile> file(do_CreateInstance("@mozilla.org/file/local;1"));
-  if (!file || NS_FAILED(file->InitWithPath(mUnicodeFile))) {
+  if (IsPrivacyModeEnabled()) {
+    // Don't remember the directory if private browsing was in effect
+    return;
+  }
+
+  nsCOMPtr<nsIFile> file;
+  if (NS_FAILED(NS_NewLocalFile(mUnicodeFile, false, getter_AddRefs(file)))) {
     NS_WARNING("RememberLastUsedDirectory failed to init file path.");
     return;
   }
@@ -816,11 +668,7 @@ nsFilePicker::RememberLastUsedDirectory()
     return;
   }
 
-  if (mLastUsedUnicodeDirectory) {
-    free(mLastUsedUnicodeDirectory);
-    mLastUsedUnicodeDirectory = nullptr;
-  }
-  mLastUsedUnicodeDirectory = ToNewUnicode(newDir);
+  sLastUsedUnicodeDirectory.reset(ToNewUnicode(newDir));
 }
 
 bool
diff --git a/system/interface/windows/nsFilePicker.h b/system/interface/windows/nsFilePicker.h
index 7a3fbbe07..050b6acaf 100644
--- a/system/interface/windows/nsFilePicker.h
+++ b/system/interface/windows/nsFilePicker.h
@@ -10,7 +10,6 @@
 #include <windows.h>
 
 #include "nsIFile.h"
-#include "nsITimer.h"
 #include "nsISimpleEnumerator.h"
 #include "nsCOMArray.h"
 #include "nsBaseFilePicker.h"
@@ -41,11 +40,9 @@ protected:
  * Native Windows FileSelector wrapper
  */
 
-class nsFilePicker :
-  public IFileDialogEvents,
-  public nsBaseWinFilePicker
-{
-  virtual ~nsFilePicker();
+class nsFilePicker : public nsBaseWinFilePicker {
+  virtual ~nsFilePicker() = default;
+
 public:
   nsFilePicker();
 
@@ -54,9 +51,6 @@ public:
 
   NS_DECL_ISUPPORTS
 
-  // IUnknown's QueryInterface
-  STDMETHODIMP QueryInterface(REFIID refiid, void** ppvResult);
-
   // nsIFilePicker (less what's in nsBaseFilePicker and nsBaseWinFilePicker)
   NS_IMETHOD GetFilterIndex(int32_t *aFilterIndex);
   NS_IMETHOD SetFilterIndex(int32_t aFilterIndex);
@@ -67,15 +61,6 @@ public:
   NS_IMETHOD ShowW(int16_t *aReturnVal);
   NS_IMETHOD AppendFilter(const nsAString& aTitle, const nsAString& aFilter);
 
-  // IFileDialogEvents
-  HRESULT STDMETHODCALLTYPE OnFileOk(IFileDialog *pfd);
-  HRESULT STDMETHODCALLTYPE OnFolderChanging(IFileDialog *pfd, IShellItem *psiFolder);
-  HRESULT STDMETHODCALLTYPE OnFolderChange(IFileDialog *pfd);
-  HRESULT STDMETHODCALLTYPE OnSelectionChange(IFileDialog *pfd);
-  HRESULT STDMETHODCALLTYPE OnShareViolation(IFileDialog *pfd, IShellItem *psi, FDE_SHAREVIOLATION_RESPONSE *pResponse);
-  HRESULT STDMETHODCALLTYPE OnTypeChange(IFileDialog *pfd);
-  HRESULT STDMETHODCALLTYPE OnOverwrite(IFileDialog *pfd, IShellItem *psi, FDE_OVERWRITE_RESPONSE *pResponse);
-
 protected:
   /* method from nsBaseFilePicker */
   virtual void InitNative(nsIWidget *aParent,
@@ -87,9 +72,6 @@ protected:
   bool IsPrivacyModeEnabled();
   bool IsDefaultPathLink();
   bool IsDefaultPathHtml();
-  void SetDialogHandle(HWND aWnd);
-  bool ClosePickerIfNeeded();
-  static void PickerCallbackTimerFunc(nsITimer *aTimer, void *aPicker);
 
   nsCOMPtr<nsILoadContext> mLoadContext;
   nsCOMPtr<nsIWidget>    mParentWidget;
@@ -100,10 +82,13 @@ protected:
   nsCOMArray<nsIFile>    mFiles;
   static char            mLastUsedDirectory[];
   nsString               mUnicodeFile;
-  static char16_t      *mLastUsedUnicodeDirectory;
-  HWND                   mDlgWnd;
   bool                   mRequireInteraction;
 
+  struct FreeDeleter {
+    void operator()(void* aPtr) { ::free(aPtr); }
+  };
+  static mozilla::UniquePtr<char16_t[], FreeDeleter> sLastUsedUnicodeDirectory;
+
   class ComDlgFilterSpec
   {
   public:
@@ -129,7 +114,6 @@ protected:
   };
 
   ComDlgFilterSpec       mComFilterList;
-  DWORD                  mFDECookie;
 };
 
 #endif // nsFilePicker_h__