Stop GetFilesHelper runnable when the operation is canceled.

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Created attachment 8755448 [details] [diff] [review]
stop.patch
Attachment #8755448 - Flags: review?(bugs)

Comment 1

2 years ago
Comment on attachment 8755448 [details] [diff] [review]
stop.patch

This seems to be some testing patch, at least it contains such stuff too.
So I'm not sure whether you were supposed to add some debug-only code or what.
Could you upload a new patch.
Attachment #8755448 - Flags: review?(bugs)
(Assignee)

Comment 2

2 years ago
Sorry, forgot to describe what this patch does:

When a GetFilesHelper is unlinked, we don't check if it's doing something in the IO thread.
With this patch, we try to stop the IO operation at any directory listing, and we don't go back to main-thread if that helper has been canceled.

The test tries (because not always the helper is fast enough) to reproduce this issue and to check that the result is correctly generated.
(Assignee)

Updated

2 years ago
Attachment #8755448 - Flags: review?(bugs)

Comment 3

2 years ago
Comment on attachment 8755448 [details] [diff] [review]
stop.patch

>     if (!NS_IsMainThread()) {
>       RunIO();
>+
>+      // If this operation has been canceled, we don't have to go back to
>+      // main-thread.
>+      {
>+        MutexAutoLock lock(mMutex);
>+        if (mCanceled) {
>+          return NS_OK;
>+        }
>+      }
>+
>       return NS_DispatchToMainThread(this);
>     }
> 
>+    MOZ_ASSERT(!mCanceled);
You can't assert this. It is possible that someone has canceled this before we're here.
But you should return early if mCanceled.

>   ExploreDirectory(const nsAString& aDOMPath, nsIFile* aFile)
>   {
>     MOZ_ASSERT(!NS_IsMainThread());
>     MOZ_ASSERT(aFile);
> 
>+  nsString ppath; aFile->GetPath(ppath);
>+printf("EXPLORE %s\n", NS_ConvertUTF16toUTF8(ppath).get());
>+    // We check if this operation has to be terminated at each recursion.
>+    {
>+      MutexAutoLock lock(mMutex);
>+      if (mCanceled) {
>+puts("OUT!");
>+        return NS_OK;
>+      }
>+    }
So you still have this debugging code here without any ifdefs or anything.
Attachment #8755448 - Flags: review?(bugs) → review-
(Assignee)

Comment 4

2 years ago
Created attachment 8755725 [details] [diff] [review]
stop.patch
Attachment #8755448 - Attachment is obsolete: true
Attachment #8755725 - Flags: review?(bugs)

Comment 5

2 years ago
Comment on attachment 8755725 [details] [diff] [review]
stop.patch

I wonder if a helper method to take the mCanceled value safely would have been nice.
Attachment #8755725 - Flags: review?(bugs) → review+
Whiteboard: btpp-active

Comment 6

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fdbb035fd12
Stop GetFilesHelper runnable when the operation is canceled, r=smaug

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0fdbb035fd12
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.