MultipartInputStream should support nsIAsyncInputStream

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 unaffected, firefox55+ fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

a year ago
This is a similar issue to the nsBufferedInputStream: bug 1360887 but for MultipartInputStream.
(Assignee)

Updated

a year ago
Blocks: 1353629
(Assignee)

Comment 1

a year ago
Created attachment 8863832 [details] [diff] [review]
part 1 - nsMultiplexInputStream should implement nsIAsyncInputStream
Assignee: nobody → amarchesini
(Assignee)

Comment 2

a year ago
Created attachment 8863834 [details] [diff] [review]
part 2 - Tests for remote blobs and multipart inputStreams
(Assignee)

Comment 3

a year ago
Created attachment 8863837 [details] [diff] [review]
part 3 - FileReader should support ReadSegments() not returning the whole size
(Assignee)

Updated

a year ago
Attachment #8863832 - Flags: review?(bugs)
(Assignee)

Updated

a year ago
Attachment #8863834 - Flags: review?(bugs)
(Assignee)

Updated

a year ago
Attachment #8863837 - Flags: review?(bugs)
Comment on attachment 8863832 [details] [diff] [review]
part 1 - nsMultiplexInputStream should implement nsIAsyncInputStream


>+nsMultiplexInputStream::AsyncWait(nsIInputStreamCallback* aCallback,
>+                                  uint32_t aFlags,
>+                                  uint32_t aRequestedCount,
>+                                  nsIEventTarget* aEventTarget)
>+{
>+  MutexAutoLock lock(mLock);
>+
>+  if (NS_FAILED(mStatus)) {
>+    return mStatus;
>+  }
>+
>+  if (mAsyncWaitCallback && aCallback) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  mAsyncWaitCallback = aCallback;
>+
>+  if (!mAsyncWaitCallback) {
>+      return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIAsyncInputStream> currentStream =
>+    do_QueryInterface(mStreams[mCurrentStream]);
This is unsafe. Perhaps use SafeElementAt

>+
>+  // We can immedaiately read from this stream.
immediately

>+  if (!currentStream) {
>+    RefPtr<AsyncWaitRunnable> runnable = new AsyncWaitRunnable(this);
>+    if (!aEventTarget) {
>+      aEventTarget = NS_GetCurrentThread();
>+    }
>+
>+    return aEventTarget->Dispatch(runnable, NS_DISPATCH_NORMAL);
>+  }
So we need to label this and target the right event target when aEventTarget isn't passed.
If nothing else, use the event target from SystemGroup.


>+nsMultiplexInputStream::IsAsyncInputStream() const
>+{
>+  for (uint32_t i = 0, len = mStreams.Length(); i < len; ++i) {
>+    nsCOMPtr<nsIAsyncInputStream> substream = do_QueryInterface(mStreams[i]);
>+    if (substream) {
>+      return true;
>+    }
>+  }
>+  return false;
>+}
Why having just one async substream is enough? Why don't we need all of them to be async? This needs a comment.
Attachment #8863832 - Flags: review?(bugs) → review-
Comment on attachment 8863834 [details] [diff] [review]
part 2 - Tests for remote blobs and multipart inputStreams

>+  ok(blob, "CtoPtoC-,ultipart: We have a blob!");
ultipart?
Attachment #8863834 - Flags: review?(bugs) → review+
Comment on attachment 8863837 [details] [diff] [review]
part 3 - FileReader should support ReadSegments() not returning the whole size

I don't understand this patch.
Why the previously reliable DoReadData (either succeeded to read everything or failed) may now read just any random number of data.

Please explain, and either ask review again or fix something.
Attachment #8863837 - Flags: review?(bugs)
(Assignee)

Comment 7

a year ago
Created attachment 8864116 [details] [diff] [review]
part 1 - nsMultiplexInputStream should implement nsIAsyncInputStream
Attachment #8863832 - Attachment is obsolete: true
(Assignee)

Comment 8

a year ago
Created attachment 8864399 [details] [diff] [review]
part 1 - nsMultiplexInputStream should implement nsIAsyncInputStream

When the first AsyncWait() is required, it's better to call that method for each sub-stream: we are reading the full stream and it's better to have all the stream ready to be read.
Attachment #8864116 - Attachment is obsolete: true
Attachment #8864399 - Flags: review?(bugs)
(Assignee)

Comment 9

a year ago
Created attachment 8864400 [details] [diff] [review]
part 3 - FileReader should support ReadSegments() not returning the whole size

It's wrong to assume that reading from an inputStream always succeeds. For instance this is not true for nsPipe and it's not true for multiplexInputStream when it contains nsIAsyncInputStreams.
Attachment #8863837 - Attachment is obsolete: true
Attachment #8864400 - Flags: review?(bugs)
Comment on attachment 8864399 [details] [diff] [review]
part 1 - nsMultiplexInputStream should implement nsIAsyncInputStream

>+    RefPtr<AsyncStreamHelper> helper =
>+      new AsyncStreamHelper(aStream, aAsyncStreams, aEventTarget);
>+    return helper->Run( aFlags, aRequestedCount);
Nit, extra space after (
Attachment #8864399 - Flags: review?(bugs) → review+
Comment on attachment 8864400 [details] [diff] [review]
part 3 - FileReader should support ReadSegments() not returning the whole size

>-    if (mDataFormat != FILE_AS_ARRAYBUFFER) {
>-      mFileData = (char *) realloc(mFileData, mDataLen + aCount);
>-      NS_ENSURE_TRUE(mFileData, NS_ERROR_OUT_OF_MEMORY);
>-    }
>-
>-    uint32_t bytesRead = 0;
>     MOZ_DIAGNOSTIC_ASSERT(mFileData);
>     nsresult rv = mAsyncStream->Read(mFileData + mDataLen, aCount, &bytesRead);
MOZ_RELEASE_ASSERT before this call that (mDataLen + aCount) <= mTotal
Attachment #8864400 - Flags: review?(bugs) → review+

Comment 12

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e3f3edf36a
nsMultiplexInputStream should implement nsIAsyncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab58ab887c6
Tests for remote blobs and multipart inputStreams, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4370dbfde05c
FileReader should support ReadSegments() not returning the whole size, r=smaug
backed out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=96595044&repo=mozilla-inbound after this and the other bug landed
Flags: needinfo?(amarchesini)

Comment 14

a year ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff08391ef68f
Backed out changeset 4370dbfde05c 
https://hg.mozilla.org/integration/mozilla-inbound/rev/8970c4963872
Backed out changeset 1ab58ab887c6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/285b0cbc973e
Backed out changeset c0e3f3edf36a for crashes in  [@ mozilla::Base64EncodeInputStream] and test failures in test_fileReaderSync.xul
(Assignee)

Updated

a year ago
Flags: needinfo?(amarchesini)

Comment 15

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9499b714afc4
nsMultiplexInputStream should implement nsIAsyncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/122644e28040
Tests for remote blobs and multipart inputStreams, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9078bccc0a3
FileReader should support ReadSegments() not returning the whole size, r=smaug
(Assignee)

Updated

a year ago
Duplicate of this bug: 1361748
Moving tracking flags from duplicate bug 1361748.
status-firefox54: --- → unaffected
status-firefox55: --- → affected
tracking-firefox55: --- → +
Duplicate of this bug: 1362345

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9499b714afc4
https://hg.mozilla.org/mozilla-central/rev/122644e28040
https://hg.mozilla.org/mozilla-central/rev/e9078bccc0a3
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.