Andrea Marchesini [:baku] Assignee
	Description • 6 years ago

https://w3c.github.io/FileAPI/#readAsDataText
6.3.4.3.3 "Initiate an annotated task read operation using the blob argument as input and handle tasks queued on the file reading task source per below."

https://w3c.github.io/FileAPI/#task-read-operation
"Perform a read operation on b with the synchronous flag unset, along with the additional steps below."

https://w3c.github.io/FileAPI/#readOperation
2 "Otherwise, the synchronous flag is unset. Return s and process the rest of this algorithm asynchronously."

	Andrea Marchesini [:baku] Assignee
	Comment 1 • 6 years ago

A bit of extra complexity is needed in case of this:

fr.readAsText(); -> here we start the runnable
fr.abort(); -> here we abort the runnable
fr.readAsText(); -> here a new runnable starts, but we don't want the previous one to call DoAsyncWait().

I also added an: AsyncWait(null) in order to abort any pending reading for scenarios like:
fr.readAsText();
setTimeout(() => { fr.abort(); fr.readAsText(); }, 0);

	Andrea Marchesini [:baku] Assignee
	Comment 2 • 6 years ago

Note the the second scenario works because each read creates a new InputStream, and in FileReader::OnInputStreamReady() we compare the current inputStream with the received as argument:
https://searchfox.org/mozilla-central/rev/d5fc6f3d4746279a7c6fd4f7a98b1bc5d05d6b01/dom/file/FileReader.cpp#663

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 6 years ago

Comment on attachment 9020366 [details] [diff] [review]
file_abort.patch

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># Parent  52728a835110a6cc00df0451903b1aacd26f3959
>
>diff --git a/dom/file/FileReader.cpp b/dom/file/FileReader.cpp
>--- a/dom/file/FileReader.cpp
>+++ b/dom/file/FileReader.cpp
>@@ -83,16 +83,45 @@ public:
>   {}
> 
>   ~FileReaderDecreaseBusyCounter()
>   {
>     mFileReader->DecreaseBusyCounter();
>   }
> };
> 
>+class FileReader::AsyncWaitRunnable final : public Runnable
>+{
>+public:
>+  AsyncWaitRunnable(FileReader* aReader)
>+    : Runnable("FileReader::AsyncWaitRunnable")
>+    , mReader(aReader)
>+    , mAborted(false)
>+  {}
>+
>+  NS_IMETHOD
>+  Run() override
>+  {
>+    if (!mAborted) {
>+      mReader->InitialAsyncWait();
>+    }
>+    return NS_OK;
>+  }
>+
>+  void
>+  Abort()
>+  {
>+    mAborted = true;
>+  }
>+
Or just clear mReader and then null check in Run()


>+public:
>+  RefPtr<FileReader> mReader;
>+  bool mAborted;
... then you don't need mAborted.


>-  mAsyncStream = nullptr;
>+  if (mAsyncStream) {
>+    if (mBusyCount) {
>+      // This will abort any pending reading.
>+      mAsyncStream->AsyncWait(/* callback */ nullptr,
>+                              /* aFlags*/ 0,
>+                              /* aRequestedCount */ 0,
>+                              mTarget);
>+    }

I can't see where mBusyCount is decreased.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 6 years ago

Comment on attachment 9020871 [details] [diff] [review]
file_abort.patch

mBusyCount handling is a bit weird, given that everything expects it is 0 or 1. But not about this bug.

Do we have tests that after abort() everything still works?

	Andrea Marchesini [:baku] Assignee
	Comment 7 • 6 years ago

(In reply to Olli Pettay [:smaug] (high review load) from comment #6)
> Comment on attachment 9020871 [details] [diff] [review]
> file_abort.patch
> 
> mBusyCount handling is a bit weird, given that everything expects it is 0 or
> 1. But not about this bug.

It can also be 2 actually. Here: https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/dom/file/FileReader.cpp#661,679

because the decreasing is done by the RAII class in its DTOR.

> Do we have tests that after abort() everything still works?

Yes, we do have. But probably we were leaking a worker because we were not decreasing the busy count.

	Pulsebot
	Comment 8 • 6 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6babd3b956aa
FileReader should dispatch loadstart asynchronously, r=smaug

	Daniel Varga [:dvarga]
	Comment 9 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6babd3b956aa

	Pulsebot
	Comment 10 • 6 years ago

Backout by dvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2aee8014db6
Backed out changeset 6babd3b956aa for wpt leak at mozilla::dom::FileReader::ReadFileContent

	Pulsebot
	Comment 11 • 6 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d4cb2e9e74
FileReader should dispatch loadstart asynchronously, r=smaug

	Cristina Coroiu [:ccoroiu]
	Comment 12 • 6 years ago

Backed out changeset e6d4cb2e9e74 (bug 1502403) for causing leak at leak at mozilla::dom::FileReader

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/614fead46648b320b58562be24c1e7082356c928

Failure  push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&searchStr=linux%2Cx64%2Casan%2Cweb%2Cplatform%2Ctests%2Cwith%2Ce10s%2Ctest-linux64-asan%2Fopt-web-platform-tests-e10s-9%2Cw-e10s%28wpt9%29&selectedJob=208647342&revision=e6d4cb2e9e74d96e5c93dd2e6f36055a89c1d4ba

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=208647342&repo=mozilla-inbound&lineNumber=12628
task 2018-10-30T14:57:23.414Z] 14:57:23     INFO - ERROR | LeakSanitizer | SUMMARY: AddressSanitizer: 45560 byte(s) leaked in 398 allocation(s).
[task 2018-10-30T14:57:23.414Z] 14:57:23     INFO - LeakSanitizer | To show the addresses of leaked objects add report_objects=1 to LSAN_OPTIONS
[task 2018-10-30T14:57:23.415Z] 14:57:23     INFO - This can be done in testing/mozbase/mozrunner/mozrunner/utils.py
[task 2018-10-30T14:57:23.415Z] 14:57:23     INFO - Allowed depth was 4
[task 2018-10-30T14:57:23.416Z] 14:57:23     INFO - TEST-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::ServiceWorkerRegistration_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-10-30T14:57:23.416Z] 14:57:23     INFO - INFO | LeakSanitizer | Frame EntrySlotOrCreate matched a expected leak
[task 2018-10-30T14:57:23.417Z] 14:57:23     INFO - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at mozilla::dom::FileReader::ReadFileContent, ReadAsArrayBuffer, mozilla::dom::FileReader_Binding::readAsArrayBuffer, mozilla::dom::binding_detail::GenericMethod
[task 2018-10-30T14:57:23.418Z] 14:57:23     INFO - TEST-FAIL | LeakSanitizer | leak at js_arena_calloc, js_pod_arena_calloc, maybe_pod_calloc, js::ShapeTable::init

	Pulsebot
	Comment 16 • 4 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d10fb80f7aa0
FileReader should dispatch loadstart asynchronously, r=smaug,ssengupta

	Pulsebot
	Comment 18 • 4 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d8936074f4b4
FileReader should dispatch loadstart asynchronously, r=smaug,ssengupta