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

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - rename DirPickerBuildFileListTasks to DirPickerFileListBuilderTask

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Cancel any in-progress DirPickerBuildFileListTasks when the user picks a new 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.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[Jonathan Watt [:jwatt]]

(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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 7

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/271b0129916d
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa9527b63f67

Comment 8

[Jonathan Watt [:jwatt]]

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.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/271b0129916d
https://hg.mozilla.org/mozilla-central/rev/aa9527b63f67