1274999 - Stop GetFilesHelper runnable when the operation is canceled.

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Attachment: stop.patch

Comment 1

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

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.

Comment 2

[Andrea Marchesini [:baku]]

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.

Comment 3

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

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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: stop.patch

Comment 5

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

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fdbb035fd12
Stop GetFilesHelper runnable when the operation is canceled, r=smaug

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fdbb035fd12