1361443 - MultipartInputStream should support nsIAsyncInputStream

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

This is a similar issue to the nsBufferedInputStream: bug 1360887 but for MultipartInputStream.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - nsMultiplexInputStream should implement nsIAsyncInputStream

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - Tests for remote blobs and multipart inputStreams

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - FileReader should support ReadSegments() not returning the whole size

Comment 4

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

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.

Comment 5

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

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?

Comment 6

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

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.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 1 - nsMultiplexInputStream should implement nsIAsyncInputStream

Comment 8

[Andrea Marchesini [:baku]]

Attachment: 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.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: 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.

Comment 10

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

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 (

Comment 11

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

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Carsten Book [:Tomcat]]

backed out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=96595044&repo=mozilla-inbound after this and the other bug landed

Comment 14

[Pulsebot]

Backout by [e-mail]:
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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Julien Cristau [:jcristau]]

Moving tracking flags from duplicate bug 1361748.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9499b714afc4
https://hg.mozilla.org/mozilla-central/rev/122644e28040
https://hg.mozilla.org/mozilla-central/rev/e9078bccc0a3