1502403 - FileReader should dispatch loadstart asynchronously

Status: RESOLVED FIXED

(Core :: DOM: File, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: file_abort.patch

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

Comment 2

[Andrea Marchesini [:baku]]

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

Comment 3

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

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

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># 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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: file_abort.patch

Comment 5

[Andrea Marchesini [:baku]]

Attachment: file_abort.patch

Comment 6

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

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?

Comment 7

[Andrea Marchesini [:baku]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6babd3b956aa
FileReader should dispatch loadstart asynchronously, r=smaug

Comment 9

[Daniel Varga [:dvarga]]

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

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2aee8014db6
Backed out changeset 6babd3b956aa for wpt leak at mozilla::dom::FileReader::ReadFileContent

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d4cb2e9e74
FileReader should dispatch loadstart asynchronously, r=smaug

Comment 12

[Cristina Coroiu [:ccoroiu]]

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

Comment 13

[Andrea Marchesini [:baku]]

I'm not working on this.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: Bug 1502403 - FileReader should dispatch loadstart asynchronously, r?smaug

Comment 15

[Andrea Marchesini [:baku]]

Attachment: Bug 1502403 - FileReader should dispatch loadstart asynchronously, r?smaug,ssengupta

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d10fb80f7aa0
FileReader should dispatch loadstart asynchronously, r=smaug,ssengupta

Comment 17

[Atila Butkovits]

Backed out for build bustages at rules.mk.

Backout link:
https://hg.mozilla.org/integration/autoland/rev/72202ffdcddb4ded847249d1e3841298ef8a2bde

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=d10fb80f7aa06d4d655d705a177883af499d5424&selectedTaskRun=aRTTSyNeTA-H8_PoyL00Pg-0

Log link:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=305631439&repo=autoland&lineNumber=28109

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8936074f4b4
FileReader should dispatch loadstart asynchronously, r=smaug,ssengupta

Comment 19

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d8936074f4b4

Comment 20

[Kohei]

The spec has changed since the bug was filed 2 years ago, and it’s now linking to https://github.com/w3c/FileAPI/issues/119 which is still open.

Comment 21

[Andrea Marchesini [:baku]]

Kohei, thanks for this spec link! I'll file a separate bug.

Comment 22

[Kohei]

Posted a site compatibility note for the change.

Comment 23

[Chris Mills [:cmills]]

I've documented this feature; see https://github.com/mdn/sprints/issues/3424#issuecomment-652932993 for full details.