Closed
Bug 1502403
Opened 6 years ago
Closed 4 years ago
FileReader should dispatch loadstart asynchronously
Categories
(Core :: DOM: File, enhancement, P2)
Core
DOM: File
Tracking
()
RESOLVED
FIXED
mozilla79
Tracking | Status | |
---|---|---|
firefox79 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
References
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(2 files, 3 obsolete files)
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."
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);
Attachment #9020366 -
Flags: review?(bugs)
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
Updated•6 years ago
|
Priority: -- → P2
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.
Attachment #9020366 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #9020366 -
Attachment is obsolete: true
Attachment #9020853 -
Flags: review?(bugs)
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #9020853 -
Attachment is obsolete: true
Attachment #9020853 -
Flags: review?(bugs)
Attachment #9020871 -
Flags: review?(bugs)
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?
Attachment #9020871 -
Flags: review?(bugs) → review+
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.
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6babd3b956aa FileReader should dispatch loadstart asynchronously, r=smaug
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6babd3b956aa
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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
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
Updated•6 years ago
|
Status: RESOLVED → REOPENED
status-firefox65:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla65 → ---
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
Flags: needinfo?(amarchesini)
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
Assignee | ||
Updated•6 years ago
|
Assignee: amarchesini → nobody
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 13•6 years ago
|
||
I'm not working on this.
Updated•4 years ago
|
Component: DOM: Core & HTML → DOM: File
Assignee | ||
Comment 14•4 years ago
|
||
Updated•4 years ago
|
Assignee: nobody → amarchesini
Updated•4 years ago
|
Attachment #9154926 -
Attachment is obsolete: true
Assignee | ||
Comment 15•4 years ago
|
||
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
Comment 17•4 years ago
|
||
Backed out for build bustages at rules.mk.
Backout link:
https://hg.mozilla.org/integration/autoland/rev/72202ffdcddb4ded847249d1e3841298ef8a2bde
Log link:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=305631439&repo=autoland&lineNumber=28109
Flags: needinfo?(amarchesini)
Assignee | ||
Updated•4 years ago
|
Flags: needinfo?(amarchesini)
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
Comment 19•4 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 6 years ago → 4 years ago
status-firefox79:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
Comment 20•4 years ago
|
||
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.
Flags: needinfo?(amarchesini)
Keywords: site-compat
Assignee | ||
Comment 21•4 years ago
|
||
Kohei, thanks for this spec link! I'll file a separate bug.
Flags: needinfo?(amarchesini)
Comment 22•4 years ago
|
||
Posted a site compatibility note for the change.
Updated•4 years ago
|
Keywords: dev-doc-needed
Comment 23•4 years ago
|
||
I've documented this feature; see https://github.com/mdn/sprints/issues/3424#issuecomment-652932993 for full details.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•