Closed Bug 1454618 Opened 2 years ago Closed 2 years ago

FileReaderSync must work also after a self.close()

Categories

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

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(1 file)

Currently we don't support this feature if the inputStream of the blob is non-blocking.
Attached patch canceling.patchSplinter Review
Attachment #8968474 - Flags: review?(bugmail)
Priority: -- → P2
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
https://hg.mozilla.org/mozilla-central/rev/6b80d13e112b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.