Bug 923922, part 2 - Allow DirPickerBuildFileListTasks to be cancelled, and cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory. r=smaug
authorJonathan Watt <jwatt@jwatt.org>
Tue, 08 Oct 2013 18:47:08 +0100
changeset 164470 aa9527b63f674b8c7ff21722e45fe298e4254f24
parent 164469 271b0129916dd8c5a666a3e0c30c31e9004b876b
child 164471 435b379ab3b8fa4f56b81592c6dcc4c0e5b4dfe5
push id3066
push userakeybl@mozilla.com
push dateMon, 09 Dec 2013 19:58:46 +0000
treeherdermozilla-beta@a31a0dce83aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs923922
milestone27.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 923922, part 2 - Allow DirPickerBuildFileListTasks to be cancelled, and cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory. r=smaug
content/html/content/src/HTMLInputElement.cpp
content/html/content/src/HTMLInputElement.h
--- a/content/html/content/src/HTMLInputElement.cpp
+++ b/content/html/content/src/HTMLInputElement.cpp
@@ -472,68 +472,16 @@ private:
   nsCOMPtr<nsIFile> mTopDir;
   nsCOMPtr<nsIFile> mTopDirsParent; // May be mTopDir if no parent
   nsCOMPtr<nsIFile> mNextFile;
   nsTArray<nsCOMPtr<nsISimpleEnumerator> > mDirEnumeratorStack;
 };
 
 NS_IMPL_ISUPPORTS1(DirPickerRecursiveFileEnumerator, nsISimpleEnumerator)
 
-class DirPickerFileListBuilderTask MOZ_FINAL
-  : public nsRunnable
-{
-public:
-  DirPickerFileListBuilderTask(HTMLInputElement* aInput, nsIFile* aTopDir)
-    : mInput(aInput)
-    , mTopDir(aTopDir)
-  {}
-
-  NS_IMETHOD Run() {
-    if (!NS_IsMainThread()) {
-      // Build up list of nsDOMFileFile objects on this dedicated thread:
-      nsCOMPtr<nsISimpleEnumerator> iter =
-        new DirPickerRecursiveFileEnumerator(mTopDir);
-      bool hasMore = true;
-      nsCOMPtr<nsISupports> tmp;
-      while (NS_SUCCEEDED(iter->HasMoreElements(&hasMore)) && hasMore) {
-        iter->GetNext(getter_AddRefs(tmp));
-        nsCOMPtr<nsIDOMFile> domFile = do_QueryInterface(tmp);
-        MOZ_ASSERT(domFile);
-        mFileList.AppendElement(domFile);
-        mInput->SetFileListProgress(mFileList.Length());
-      }
-      return NS_DispatchToMainThread(this);
-    }
-
-    // Now back on the main thread, set the list on our HTMLInputElement:
-    if (mFileList.IsEmpty()) {
-      return NS_OK;
-    }
-    // The text control frame (if there is one) isn't going to send a change
-    // event because it will think this is done by a script.
-    // So, we can safely send one by ourself.
-    mInput->SetFiles(mFileList, true);
-    mInput->MaybeDispatchProgressEvent(true); // last progress event
-    nsresult rv =
-      nsContentUtils::DispatchTrustedEvent(mInput->OwnerDoc(),
-                                           static_cast<nsIDOMHTMLInputElement*>(mInput.get()),
-                                           NS_LITERAL_STRING("change"), true,
-                                           false);
-    // Clear mInput to make sure that it can't lose its last strong ref off the
-    // main thread (which may happen if our dtor runs off the main thread)!
-    mInput = nullptr;
-    return rv;
-  }
-
-private:
-  nsRefPtr<HTMLInputElement> mInput;
-  nsCOMPtr<nsIFile> mTopDir;
-  nsTArray<nsCOMPtr<nsIDOMFile> > mFileList;
-};
-
 /**
  * This may return nullptr if aDomFile's implementation of
  * nsIDOMFile::mozFullPathInternal does not successfully return a non-empty
  * string that is a valid path. This can happen on Firefox OS, for example,
  * where the file picker can create Blobs.
  */
 static already_AddRefed<nsIFile>
 DOMFileToLocalFile(nsIDOMFile* aDomFile)
@@ -549,23 +497,127 @@ DOMFileToLocalFile(nsIDOMFile* aDomFile)
                              getter_AddRefs(localFile));
   NS_ENSURE_SUCCESS(rv, nullptr);
 
   return localFile.forget();
 }
 
 } // anonymous namespace
 
+class DirPickerFileListBuilderTask MOZ_FINAL
+  : public nsRunnable
+{
+public:
+  DirPickerFileListBuilderTask(HTMLInputElement* aInput, nsIFile* aTopDir)
+    : mPreviousFileListLength(0)
+    , mInput(aInput)
+    , mTopDir(aTopDir)
+    , mFileListLength(0)
+    , mCanceled(0)
+  {}
+
+  NS_IMETHOD Run() {
+    if (!NS_IsMainThread()) {
+      // Build up list of nsDOMFileFile objects on this dedicated thread:
+      nsCOMPtr<nsISimpleEnumerator> iter =
+        new DirPickerRecursiveFileEnumerator(mTopDir);
+      bool hasMore = true;
+      nsCOMPtr<nsISupports> tmp;
+      while (NS_SUCCEEDED(iter->HasMoreElements(&hasMore)) && hasMore) {
+        iter->GetNext(getter_AddRefs(tmp));
+        nsCOMPtr<nsIDOMFile> domFile = do_QueryInterface(tmp);
+        MOZ_ASSERT(domFile);
+        mFileList.AppendElement(domFile);
+        mFileListLength = mFileList.Length();
+        if (mCanceled) {
+          NS_ASSERTION(!mInput, "This is bad - how did this happen?");
+          // There's no point dispatching to the main thread (that doesn't
+          // guarantee that we'll be destroyed there).
+          return NS_OK;
+        }
+      }
+      return NS_DispatchToMainThread(this);
+    }
+
+    // Now back on the main thread, set the list on our HTMLInputElement:
+    if (mCanceled || mFileList.IsEmpty()) {
+      return NS_OK;
+    }
+    MOZ_ASSERT(mInput->mDirPickerFileListBuilderTask,
+               "But we aren't canceled!");
+    if (mInput->mProgressTimer) {
+      mInput->mProgressTimerIsActive = false;
+      mInput->mProgressTimer->Cancel();
+    }
+
+    // The text control frame (if there is one) isn't going to send a change
+    // event because it will think this is done by a script.
+    // So, we can safely send one by ourself.
+    mInput->SetFiles(mFileList, true);
+    mInput->MaybeDispatchProgressEvent(true);        // Last progress event.
+    mInput->mDirPickerFileListBuilderTask = nullptr; // Now null out.
+    nsresult rv =
+      nsContentUtils::DispatchTrustedEvent(mInput->OwnerDoc(),
+                                           static_cast<nsIDOMHTMLInputElement*>(mInput.get()),
+                                           NS_LITERAL_STRING("change"), true,
+                                           false);
+    // Clear mInput to make sure that it can't lose its last strong ref off the
+    // main thread (which may happen if our dtor runs off the main thread)!
+    mInput = nullptr;
+    return rv;
+  }
+
+  void Cancel()
+  {
+    MOZ_ASSERT(NS_IsMainThread() && !mCanceled);
+    // Clear mInput to make sure that it can't lose its last strong ref off the
+    // main thread (which may happen if our dtor runs off the main thread)!
+    mInput = nullptr;
+    mCanceled = 1; // true
+  }
+
+  uint32_t GetFileListLength() const
+  {
+    return mFileListLength;
+  }
+
+  /**
+   * The number of files added to the FileList at the time the last progress
+   * event was fired.
+   *
+   * This is only read/set by HTMLInputElement on the main thread. The reason
+   * that this member is stored here rather than on HTMLInputElement is so that
+   * we don't increase the size of HTMLInputElement for something that's rarely
+   * used.
+   */
+  uint32_t mPreviousFileListLength;
+
+private:
+  nsRefPtr<HTMLInputElement> mInput;
+  nsCOMPtr<nsIFile> mTopDir;
+  nsTArray<nsCOMPtr<nsIDOMFile> > mFileList;
+
+  // We access the list length on both threads, so we need the indirection of
+  // this atomic member to make the access thread safe:
+  mozilla::Atomic<uint32_t> mFileListLength;
+
+  // We'd prefer this member to be bool, but we don't support Atomic<bool>.
+  mozilla::Atomic<uint32_t> mCanceled;
+};
+
+
 NS_IMETHODIMP
 HTMLInputElement::nsFilePickerShownCallback::Done(int16_t aResult)
 {
   if (aResult == nsIFilePicker::returnCancel) {
     return NS_OK;
   }
 
+  mInput->CancelDirectoryPickerScanIfRunning();
+
   int16_t mode;
   mFilePicker->GetMode(&mode);
 
   if (mode == static_cast<int16_t>(nsIFilePicker::modeGetFolder)) {
     // Directory picking is different, since we still need to do more I/O to
     // build up the list of nsDOMFileFile objects. Since this may block for a
     // long time, we need to build the list off on another dedicated thread to
     // avoid blocking any other activities that the browser is carrying out.
@@ -585,24 +637,24 @@ HTMLInputElement::nsFilePickerShownCallb
 
     HTMLInputElement::gUploadLastDir->StoreLastUsedDirectory(
       mInput->OwnerDoc(), pickedDir);
 
     nsCOMPtr<nsIEventTarget> target
       = do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
     NS_ASSERTION(target, "Must have stream transport service");
 
-    mInput->ResetProgressCounters();
     mInput->StartProgressEventTimer();
 
     // DirPickerFileListBuilderTask takes care of calling SetFiles() and
     // dispatching the "change" event.
-    nsRefPtr<DirPickerFileListBuilderTask> event =
+    mInput->mDirPickerFileListBuilderTask =
       new DirPickerFileListBuilderTask(mInput.get(), pickedDir.get());
-    return target->Dispatch(event, NS_DISPATCH_NORMAL);
+    return target->Dispatch(mInput->mDirPickerFileListBuilderTask,
+                            NS_DISPATCH_NORMAL);
   }
 
   // Collect new selected filenames
   nsTArray<nsCOMPtr<nsIDOMFile> > newFiles;
   if (mode == static_cast<int16_t>(nsIFilePicker::modeOpenMultiple)) {
     nsCOMPtr<nsISimpleEnumerator> iter;
     nsresult rv = mFilePicker->GetDomfiles(getter_AddRefs(iter));
     NS_ENSURE_SUCCESS(rv, rv);
@@ -1015,18 +1067,16 @@ static nsresult FireEventForAccessibilit
 
 //
 // construction, destruction
 //
 
 HTMLInputElement::HTMLInputElement(already_AddRefed<nsINodeInfo> aNodeInfo,
                                    FromParser aFromParser)
   : nsGenericHTMLFormElementWithState(aNodeInfo)
-  , mFileListProgress(0)
-  , mLastFileListProgress(0)
   , mType(kInputDefaultType->value)
   , mDisabledChanged(false)
   , mValueChanged(false)
   , mCheckedChanged(false)
   , mChecked(false)
   , mHandlingSelectEvent(false)
   , mShouldInitChecked(false)
   , mParserCreating(aFromParser != NOT_FROM_PARSER)
@@ -2459,16 +2509,30 @@ HTMLInputElement::OpenDirectoryPicker(Er
 {
   if (mType != NS_FORM_INPUT_FILE) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
   }
   InitFilePicker(FILE_PICKER_DIRECTORY);
 }
 
 void
+HTMLInputElement::CancelDirectoryPickerScanIfRunning()
+{
+  if (!mDirPickerFileListBuilderTask) {
+    return;
+  }
+  if (mProgressTimer) {
+    mProgressTimerIsActive = false;
+    mProgressTimer->Cancel();
+  }
+  mDirPickerFileListBuilderTask->Cancel();
+  mDirPickerFileListBuilderTask = nullptr;
+}
+
+void
 HTMLInputElement::StartProgressEventTimer()
 {
   if (!mProgressTimer) {
     mProgressTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
   }
   if (mProgressTimer) {
     mProgressTimerIsActive = true;
     mProgressTimer->Cancel();
@@ -2502,29 +2566,32 @@ HTMLInputElement::MaybeDispatchProgressE
     // ref to make sure we don't die under Cancel() and leave this method
     // running on deleted memory.
     kungFuDeathGrip = this;
 
     mProgressTimerIsActive = false;
     mProgressTimer->Cancel();
   }
 
+  uint32_t fileListLength = mDirPickerFileListBuilderTask->GetFileListLength();
+
   if (mProgressTimerIsActive ||
-      mFileListProgress == mLastFileListProgress) {
+      fileListLength == mDirPickerFileListBuilderTask->mPreviousFileListLength) {
     return;
   }
 
   if (!aFinalProgress) {
     StartProgressEventTimer();
   }
 
-  mLastFileListProgress = mFileListProgress;
+  mDirPickerFileListBuilderTask->mPreviousFileListLength = fileListLength;
 
   DispatchProgressEvent(NS_LITERAL_STRING(PROGRESS_STR),
-                        false, mLastFileListProgress,
+                        false,
+                        mDirPickerFileListBuilderTask->mPreviousFileListLength,
                         0);
 }
 
 void
 HTMLInputElement::DispatchProgressEvent(const nsAString& aType,
                                         bool aLengthComputable,
                                         uint64_t aLoaded, uint64_t aTotal)
 {
--- a/content/html/content/src/HTMLInputElement.h
+++ b/content/html/content/src/HTMLInputElement.h
@@ -27,16 +27,17 @@ class nsIRadioGroupContainer;
 class nsIRadioGroupVisitor;
 class nsIRadioVisitor;
 class nsTextEditorState;
 
 namespace mozilla {
 namespace dom {
 
 class Date;
+class DirPickerFileListBuilderTask;
 
 class UploadLastDir MOZ_FINAL : public nsIObserver, public nsSupportsWeakReference {
 public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSIOBSERVER
 
   /**
    * Fetch the last used directory for this location from the content
@@ -82,16 +83,18 @@ class HTMLInputElement MOZ_FINAL : publi
                                    public nsImageLoadingContent,
                                    public nsIDOMHTMLInputElement,
                                    public nsITextControlElement,
                                    public nsIPhonetic,
                                    public nsIDOMNSEditableElement,
                                    public nsITimerCallback,
                                    public nsIConstraintValidation
 {
+  friend class DirPickerFileListBuilderTask;
+
 public:
   using nsIConstraintValidation::GetValidationMessage;
   using nsIConstraintValidation::CheckValidity;
   using nsIConstraintValidation::WillValidate;
   using nsIConstraintValidation::Validity;
   using nsGenericHTMLFormElementWithState::GetForm;
 
   HTMLInputElement(already_AddRefed<nsINodeInfo> aNodeInfo,
@@ -397,22 +400,18 @@ public:
     SetHTMLBoolAttr(nsGkAtoms::disabled, aValue, aRv);
   }
 
   // XPCOM GetForm() is OK
 
   nsDOMFileList* GetFiles();
 
   void OpenDirectoryPicker(ErrorResult& aRv);
+  void CancelDirectoryPickerScanIfRunning();
 
-  void ResetProgressCounters()
-  {
-    mFileListProgress = 0;
-    mLastFileListProgress = 0;
-  }
   void StartProgressEventTimer();
   void MaybeDispatchProgressEvent(bool aFinalProgress);
   void DispatchProgressEvent(const nsAString& aType,
                              bool aLengthComputable,
                              uint64_t aLoaded, uint64_t aTotal);
 
   // XPCOM GetFormAction() is OK
   void SetFormAction(const nsAString& aValue, ErrorResult& aRv)
@@ -672,23 +671,16 @@ public:
   bool MozIsTextField(bool aExcludePassword);
 
   nsIEditor* GetEditor();
 
   // XPCOM SetUserInput() is OK
 
   // XPCOM GetPhonetic() is OK
 
-  void SetFileListProgress(uint32_t mFileCount)
-  {
-    MOZ_ASSERT(!NS_IsMainThread(),
-               "Why are we calling this on the main thread?");
-    mFileListProgress = mFileCount;
-  }
-
 protected:
   virtual JSObject* WrapNode(JSContext* aCx,
                              JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
 
   // Pull IsSingleLineTextControl into our scope, otherwise it'd be hidden
   // by the nsITextControlElement version.
   using nsGenericHTMLFormElementWithState::IsSingleLineTextControl;
 
@@ -1157,16 +1149,18 @@ protected:
    * Therefor the list of filenames is always owned by this member, never by
    * the frame. Whenever the frame wants to change the filename it has to call
    * SetFileNames to update this member.
    */
   nsTArray<nsCOMPtr<nsIDOMFile> >   mFiles;
 
   nsRefPtr<nsDOMFileList>  mFileList;
 
+  nsRefPtr<DirPickerFileListBuilderTask> mDirPickerFileListBuilderTask;
+
   nsString mStaticDocFileList;
   
   /** 
    * The value of the input element when first initialized and it is updated
    * when the element is either changed through a script, focused or dispatches   
    * a change event. This is to ensure correct future change event firing.
    * NB: This is ONLY applicable where the element is a text control. ie,
    * where type= "text", "email", "search", "tel", "url" or "password".
@@ -1198,29 +1192,16 @@ protected:
   // Default step used when there is no specified step.
   static const Decimal kDefaultStep;
   static const Decimal kDefaultStepTime;
 
   // Float value returned by GetStep() when the step attribute is set to 'any'.
   static const Decimal kStepAny;
 
   /**
-   * The number of files added to the FileList being built off-main-thread when
-   * mType == NS_FORM_INPUT_FILE and the user selects a directory. This is set
-   * off the main thread, read on main thread.
-   */
-  mozilla::Atomic<uint32_t> mFileListProgress;
-
-  /**
-   * The number of files added to the FileList at the time the last progress
-   * event was fired.
-   */
-  uint32_t mLastFileListProgress;
-
-  /**
    * The type of this input (<input type=...>) as an integer.
    * @see nsIFormControl.h (specifically NS_FORM_INPUT_*)
    */
   uint8_t                  mType;
   bool                     mDisabledChanged     : 1;
   bool                     mValueChanged        : 1;
   bool                     mCheckedChanged      : 1;
   bool                     mChecked             : 1;