Closed Bug 923922 Opened 6 years ago Closed 6 years ago

Allow DirPickerBuildFileListTasks to be cancelled, and cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(2 files, 1 obsolete file)

DirPickerBuildFileListTasks can be long-lived. We need to cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory and kicks off a new DirPickerBuildFileListTask for that directory.
To avoid races between the cancelling and the setting of the progress counter (i.e. having mLastFileListProgress set on the HTMLInputElement by a discarded DirPickerFileListBuilderTask) I moved the progress counter to the task object.
Attachment #813951 - Flags: review?(bugs)
Blocks: 846931
Attachment #813950 - Flags: review?(bugs) → review+
Comment on attachment 814109 [details] [diff] [review]
part 2 - Cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory


>     // Now back on the main thread, set the list on our HTMLInputElement:
>-    if (mFileList.IsEmpty()) {
This is old code, but I don't understand why we don't dispatch change event.
If we opened and closed the filepicker, we cleared filelist, but then
here we don't necessarily fire change event.
We need to fire change at some point.

r-, but perhaps r- should go to the patch clearing filelist, if we should actually
dispatch change event at that point. It would be at least consistent - always dispatch change
when filelist changes.
Attachment #814109 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #4)
> r-, but perhaps r- should go to the patch clearing filelist

Yeah, as discussed on IRC let's r- bug 912492 instead for now.
Attachment #814109 - Flags: review- → review?(bugs)
Comment on attachment 814109 [details] [diff] [review]
part 2 - Cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory

>   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());
>+        if (mCanceled) {
Please assert that mInput is null here

>+  void Cancel() {
{ should be in the next line

>+  uint32_t GetFileListLength() const {
ditto

>+    return mFileList.Length();
>+  }
Hmm, so you end up accessing mFileList on several threads.
Not too happy with that. May lead to some odd behavior.
Could you please use Atomic to store the length.

You should use Atomic for mCanceled too.
(Not sure we support bool, but uint32_t should work.)
Attachment #814109 - Flags: review?(bugs) → review-
Oh man, clearly I shouldn't be pushing stuff before 9am on a Monday. Just realized that comment 6 was an unconditional r-, not a conditional r+.

That said this is all disabled behind a pref, so I'll err on the side of not churning the code unnecessarily and ping you on IRC about addressing any remaining issues you might have as a follow-up, Smaug.
https://hg.mozilla.org/mozilla-central/rev/271b0129916d
https://hg.mozilla.org/mozilla-central/rev/aa9527b63f67
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 926330
You need to log in before you can comment on or make changes to this bug.