Closed
Bug 1454618
Opened 7 years ago
Closed 7 years ago
FileReaderSync must work also after a self.close()
Categories
(Core :: DOM: Workers, enhancement, P2)
Tracking
()
RESOLVED
FIXED
mozilla61
| Tracking | Status | |
|---|---|---|
| firefox61 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
Details
Attachments
(1 file)
|
3.92 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
Currently we don't support this feature if the inputStream of the blob is non-blocking.
|
Assignee | |
Comment 1•7 years ago
|
||
Attachment #8968474 -
Flags: review?(bugmail)
|
||
Updated•7 years ago
|
Priority: -- → P2
|
||
Comment 2•7 years ago
|
||
Comment on attachment 8968474 [details] [diff] [review]
canceling.patch
Review of attachment 8968474 [details] [diff] [review]:
-----------------------------------------------------------------
r=asuth but note the test currently fails and I'd really like the requested comment or something similar to be added.
::: dom/file/FileReaderSync.cpp
@@ +442,5 @@
>
> WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> MOZ_ASSERT(workerPrivate);
>
> + AutoSyncLoopHolder syncLoop(workerPrivate, Terminating);
Restating: Like previous bugs in this family of cleanups, we're just changing aFailStatus so that if the current status is Closing, the sync loop will still be created.
::: dom/workers/WorkerPrivate.cpp
@@ +4124,5 @@
> already_AddRefed<nsIEventTarget>
> WorkerPrivate::CreateNewSyncLoop(WorkerStatus aFailStatus)
> {
> AssertIsOnWorkerThread();
> + MOZ_ASSERT(aFailStatus >= Terminating,
Nice informative assertion!
::: dom/workers/test/test_fileReaderSync_when_closing.html
@@ +8,5 @@
> +</head>
> +<body>
> + <script type="application/javascript">
> +
> +var url = SimpleTest.getTestFileURL("script_createFile.js");
I think this choice was made for ease-of-test-writing, but without a comment it's hard to know the intent. I think it'd be helpful to add a comment like the below:
"""
In order to exercise FileReaderSync::SyncRead's syncLoop-using AsyncWait() path, we need to provide a stream that will both 1) not have all the data immediately available (eliminating memory-backed Blobs) and 2) return NS_BASE_STREAM_WOULD_BLOCK. Under e10s, any Blob/File sourced from the parent process (as loadChromeScript performs) will be backed by an IPCBlobInputStream and will behave this way on first use (when it is in the eInit state). For ease of testing, we reuse script_createFile.js which involves a file on disk, but a memory-backed Blob from the parent process would be equally fine. Under non-e10s, this File will not do the right thing because a synchronous nsFileInputStream will be made directly available and the AsyncWait path won't be taken, but the test will still pass.
"""
@@ +9,5 @@
> +<body>
> + <script type="application/javascript">
> +
> +var url = SimpleTest.getTestFileURL("script_createFile.js");
> +script = SpecialPowers.loadChromeScript(url);
I know this was copied-and-pasted, but I think this wants to be "var script...".
@@ +23,5 @@
> +
> + var b = new Blob([workerCode+'workerCode();']);
> + var w = new Worker(URL.createObjectURL(b));
> + w.onmessage = function(e) {
> + ok(e.data, "Hello world!", "The blob content is OK!");
Something is wrong here, but I'm not sure what your intent was. script_createFile.js creates a 0-length file on disk, so if this was an is() call, this would fail. This also currently fails because e.data is an empty string which is falsey. I do agree that it would be better to have some content in the file since there are a lot of potential failure modes that could return an empty string due to truncating the stream/etc.
Attachment #8968474 -
Flags: review?(bugmail) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b80d13e112b
FileReaderSync must work also after a self.close(), r=asuth
|
||
Comment 4•7 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•