Closed Bug 1339710 Opened 3 years ago Closed 3 years ago

Merge SlicedInputStream and nsIPartialFileInputStream

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(5 files, 4 obsolete files)

nsIPartialFileInputStream is nsIFileInputStream + SlicedInputStream.
We can get rid of it.
Assignee: nobody → amarchesini
Attachment #8837453 - Flags: review?(bugs)
Attachment #8837453 - Attachment is obsolete: true
Attachment #8837453 - Flags: review?(bugs)
Attachment #8837542 - Flags: review?(bugs)
Comment on attachment 8837454 [details] [diff] [review]
part 2 - Remove nsIPartialFileInputStream

ok, this is hard to review. Need to compare ::Read, ::Tell, ::Seek and ::Available
implementations to ensure index/length handling is the same, and also that flags are handled the same way...

>-  aRv = NS_NewPartialLocalFileInputStream(aStream, mFile, mStart, mLength,
>-                                          -1, -1, sFileStreamFlags);
>+  if (mWholeFile) {
>+    stream.forget(aStream);
>+    return;
>+  }
>+
>+  RefPtr<SlicedInputStream> slicedInputStream =
>+    new SlicedInputStream(stream, mStart, mLength);
>+  slicedInputStream.forget(aStream);
Since SlicedInputStream has different implementation to PartialLocalFileInputStream, make sure we have tests for passing odd start and end indexes to
blob.slice. Test cases when too large start or end index is passed as param.

>-NS_INTERFACE_MAP_BEGIN(nsPartialFileInputStream)
>-    NS_INTERFACE_MAP_ENTRY(nsIInputStream)
>-    NS_INTERFACE_MAP_ENTRY(nsIPartialFileInputStream)
>-    NS_INTERFACE_MAP_ENTRY(nsILineInputStream)
So why we don't need nsILineInputStream anymore?

>-nsPartialFileInputStream::Read(char* aBuf, uint32_t aCount, uint32_t* aResult)
>-{
>-    nsresult rv = DoPendingSeek();
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    uint32_t readsize = (uint32_t) TruncateSize(aCount);
I'm having hard time to see if SlicedInputStream does this kind of check before actually reading.
So, please explain why Read is still ok (and possibly fix SlicedInputStream), and also add tests for slice handling or give some links showing that we actually have tests for weird params to slice()
Attachment #8837454 - Flags: review?(bugs) → review-
Comment on attachment 8837542 [details] [diff] [review]
part 1 - SlicedInputStream should be serializable

> SlicedInputStream::Clone(nsIInputStream** aResult)
> {
>+  MOZ_ASSERT(mInputStream);
>+
>+  nsCOMPtr<nsIInputStream> inputStream =
>+    mClonedInputStream ? mClonedInputStream : mInputStream;
>+
>   nsCOMPtr<nsIInputStream> clonedStream;
>   nsCOMPtr<nsIInputStream> replacementStream;
> 
>-  nsresult rv = NS_CloneInputStream(mInputStream, getter_AddRefs(clonedStream),
>+  nsresult rv = NS_CloneInputStream(inputStream, getter_AddRefs(clonedStream),
>                                     getter_AddRefs(replacementStream));
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     return rv;
>   }
> 
>   if (replacementStream) {
>-    mInputStream = replacementStream.forget();
>+    MOZ_ASSERT(!mClonedInputStream);
>+    mClonedInputStream = replacementStream.forget();
>   }
> 
>   nsCOMPtr<nsIInputStream> sis =
>     new SlicedInputStream(clonedStream, mStart, mLength);
> 
>   sis.forget(aResult);
>   return NS_OK;
> }
I don't understand mClonedInputStream handling. Why do we need to keep a reference to it?

Please explain and document why and how it is used.
Attachment #8837542 - Flags: review?(bugs) → review-
Documentation added. The reason why we need mClonedInputStream is because, when NS_CloneInputStream is called, it can return back a replacement stream in case the inputStream is not a nsIClonableInputStream. This replacement stream is a pipe stream and, before this patch, we used to store it in mInputStream.

But a pipe stream is not IPC serializable and, if a stream is closed and then serialized, everything fails: we must use the original mInputStream and keep alive the replacement stream.
Attachment #8837542 - Attachment is obsolete: true
Attachment #8837967 - Flags: review?(bugs)
Comment on attachment 8837454 [details] [diff] [review]
part 2 - Remove nsIPartialFileInputStream

https://dxr.mozilla.org/mozilla-central/source/xpcom/tests/gtest/TestSlicedInputStream.cpp

We have tests for SlicedInputStream. Also with values out of range.
If you find something uncovered by the test, I can expand it.
Attachment #8837454 - Flags: review- → review?(bugs)
(In reply to Andrea Marchesini [:baku] from comment #6)
> Created attachment 8837967 [details] [diff] [review]
> part 1 - SlicedInputStream should be serializable
> 
> Documentation added. The reason why we need mClonedInputStream is because,
> when NS_CloneInputStream is called, it can return back a replacement stream
> in case the inputStream is not a nsIClonableInputStream. This replacement
> stream is a pipe stream and, before this patch, we used to store it in
> mInputStream.
> 
> But a pipe stream is not IPC serializable and, if a stream is closed and
> then serialized, everything fails: we must use the original mInputStream and
> keep alive the replacement stream.
But shouldn't you keep the original stream alive in the clone, not clone in the original?
What if one clones the stream twice? You end up replacing mClonedInputStream value.
(In reply to Andrea Marchesini [:baku] from comment #7)
> Comment on attachment 8837454 [details] [diff] [review]
> part 2 - Remove nsIPartialFileInputStream
> 
> https://dxr.mozilla.org/mozilla-central/source/xpcom/tests/gtest/
> TestSlicedInputStream.cpp
> 
> We have tests for SlicedInputStream. Also with values out of range.
> If you find something uncovered by the test, I can expand it.

Do we have tests for Blob.slice? Since that is the web phasing API which might change here. I don't care about
Gecko internal tests in this case, since they don't check that SlicedInputStream and nsIPartialFileInputStream work the same way (only that they both work in some way).
https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.h#274

If the original inputStream is not clonable (nsIClonableInputStream), NS_CloneInputStream creates a pipe stream and returns it as replacement stream. The replacement stream is clonable, and this is the reason why, if sliced stream is cloned twice we use mClonedInputStream instead the original one.
There is also an assertion to be sure that, if we receive a replacement Stream, we don't already have mClonedInputStream yet.

About testing the slice, yes, we have tests:
https://dxr.mozilla.org/mozilla-central/source/dom/file/tests/test_fileapi_slice.html
https://dxr.mozilla.org/mozilla-central/source/dom/file/tests/test_blobconstructor.html

but note that, out of range values are normalized before sending them to CreateSlice() methods.
Assertions aren't enough. The API implementation explicitly needs to prevent some usage (so that the clone doesn't for example get used in certain way in JS/opt builds).

We have had recently security critical bugs because we've assumed assertions catch the issues, when they don't.
Hmm, I guess assertion is fine in this case, but I still don't understand why we have mClonedInputStream.
I would expect always a new clone stream and store pointer to the original stream in the new SlicedInputStream object.
I feel like I'm missing something here. Want to clarify?
What my code does is following the comment of NS_CloneInputStream: 
 * Clone the provided source stream in the most efficient way possible.  This
 * first attempts to QI to nsICloneableInputStream to use Clone().  If that is
 * not supported or its cloneable attribute is false, then a fallback clone is
 * provided by copying the source to a pipe.  In this case the caller must
 * replace the source stream with the resulting replacement stream.  The clone
 * and the replacement stream are then cloneable using nsICloneableInputStream
 * without duplicating memory. 

But I don't replace the source stream with the resulting replacement stream because the pipe is not IPC serializable.
Instead I keep both streams: the source and the replacement one. I'll use the replacement stream in the following cloning calls, but I'll use the source stream for IPC serialization.
Attachment #8838125 - Flags: review?(bugs)
(In reply to Andrea Marchesini [:baku] from comment #6)
> But a pipe stream is not IPC serializable and, if a stream is closed and
> then serialized, everything fails: we must use the original mInputStream and
> keep alive the replacement stream.
I don't still understand this. 
If pipe stream is not IPC serializable, how does serializing of the clone of SlicedInputStream work?
Clone's mInputStream points to a pipe stream.
Comment on attachment 8837967 [details] [diff] [review]
part 1 - SlicedInputStream should be serializable

>+NS_INTERFACE_MAP_BEGIN(SlicedInputStream)
>+  NS_INTERFACE_MAP_ENTRY(nsIInputStream)
>+  NS_INTERFACE_MAP_ENTRY(nsICloneableInputStream)
>+  NS_INTERFACE_MAP_ENTRY(nsIAsyncInputStream)
>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIIPCSerializableInputStream,
>+                                     mWeakIPCSerializableInputStream || !mInputStream)
This is part of the surprising result. Original stream is possibly nsIIPCSerializableInputStream, but clone is never.
Currently nsPartialFileInputStream always implements nsIIPCSerializableInputStream, though, it is not cloneable.

Does SlicedInputStream need to be cloneable? If so, I think this should be always also serializable.


Please explain why this rather odd setup and ask review again, or fix the setup :)
Attachment #8837967 - Flags: review?(bugs) → review-
Attachment #8837454 - Flags: review?(bugs) → review+
Comment on attachment 8838125 [details] [diff] [review]
part 3 - SlicedInputStream must be seekable

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># Parent  cf693ee4f9e391a090fe6e4a7548bcae4754ea43
>
>diff --git a/xpcom/io/SlicedInputStream.cpp b/xpcom/io/SlicedInputStream.cpp
>--- a/xpcom/io/SlicedInputStream.cpp
>+++ b/xpcom/io/SlicedInputStream.cpp
>@@ -14,32 +14,36 @@ NS_IMPL_ADDREF(SlicedInputStream);
> NS_IMPL_RELEASE(SlicedInputStream);
> 
> NS_INTERFACE_MAP_BEGIN(SlicedInputStream)
>   NS_INTERFACE_MAP_ENTRY(nsIInputStream)
>   NS_INTERFACE_MAP_ENTRY(nsICloneableInputStream)
>   NS_INTERFACE_MAP_ENTRY(nsIAsyncInputStream)
>   NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIIPCSerializableInputStream,
>                                      mWeakIPCSerializableInputStream || !mInputStream)
>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsISeekableStream,
>+                                     mWeakSeekableInputStream || !mInputStream)
Ok, so since clone gets pipe stream, and that happens to be seekable, this is fine... except
that nsPipeInputStream doesn't really support ::Seek or such.

Could we possibly get a bit more consistency here, or could you explain why we want this behavior (and ask review again)?
Attachment #8838125 - Flags: review?(bugs) → review-
Can we discuss this issue on IRC?
Attachment #8837967 - Attachment is obsolete: true
Attachment #8838534 - Flags: review?(bugs)
Attachment #8838125 - Attachment is obsolete: true
Attachment #8838535 - Flags: review?(bugs)
Comment on attachment 8838533 [details] [diff] [review]
part 0 - SlicedInputStream + nsICloneableInputStream

>+void
>+SlicedInputStream::SetSourceStream(nsIInputStream* aInputStream)
>+{
>+  MOZ_ASSERT(aInputStream);
>+
>+  mInputStream = aInputStream;
>+
>+  nsCOMPtr<nsICloneableInputStream> cloneableStream =
>+    do_QueryInterface(aInputStream);
>+  if (cloneableStream) {
>+    MOZ_ASSERT(SameCOMIdentity(aInputStream, cloneableStream));
Why is this assert correct? Please make this method to just return early if there isn't SameCOMIdentity


ok, I guess we can do this. It is still unclear to me why SlicedInputStream needs to ever support nsICloneableInputStream, but 
perhaps latter patches explain that.
Attachment #8838533 - Flags: review?(bugs) → review+
Comment on attachment 8838534 [details] [diff] [review]
part 1 - SlicedInputStream should be serializable

>   NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsICloneableInputStream,
>-                                     mWeakCloneableInputStream)
>+                                     mWeakCloneableInputStream || !mInputStream)
!mInputStream doesn't make sense.
::Clone expects that there is valid mWeakCloneableInputStream.


>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIIPCSerializableInputStream,
>+                                     mWeakIPCSerializableInputStream || !mInputStream)
Similar here for !mInputStream. ::Serialize expects mWeakIPCSerializableInputStream to be non-null

> NS_IMETHODIMP
> SlicedInputStream::Close()
> {
>+  MOZ_ASSERT(mInputStream);

So, if you want to keep the QI implementation as it is, you need to make
this proper error check, not assert.
NS_ENSURE_STATE(mInputStream); perhaps?


> NS_IMETHODIMP
> SlicedInputStream::Available(uint64_t* aLength)
> {
>+  MOZ_ASSERT(mInputStream);
ditto


> SlicedInputStream::ReadSegments(nsWriteSegmentFun aWriter, void* aClosure,
>                                 uint32_t aCount, uint32_t *aResult)
> {
>+  MOZ_ASSERT(mInputStream);
and here


> NS_IMETHODIMP
> SlicedInputStream::IsNonBlocking(bool* aNonBlocking)
> {
>+  MOZ_ASSERT(mInputStream);
and here


> SlicedInputStream::GetCloneable(bool* aCloneable)
> {
>+  MOZ_ASSERT(mInputStream);
>   MOZ_ASSERT(mWeakCloneableInputStream);
here check both mInputStream and mWeakCloneableInputStream and return early if either is null

> SlicedInputStream::Clone(nsIInputStream** aResult)
> {
>+  MOZ_ASSERT(mInputStream);
>   MOZ_ASSERT(mWeakCloneableInputStream);
and here

SlicedInputStream::CloseWithStatus(nsresult aStatus)
> {
>+  MOZ_ASSERT(mInputStream);
>+
return early if mInputStream is null


> SlicedInputStream::AsyncWait(nsIInputStreamCallback* aCallback,
>                              uint32_t aFlags,
>                              uint32_t aRequestedCount,
>                              nsIEventTarget* aEventTarget)
> {
>+  MOZ_ASSERT(mInputStream);
ditto


>+SlicedInputStream::Serialize(mozilla::ipc::InputStreamParams& aParams,
>+                             FileDescriptorArray& aFileDescriptors)
>+{
>+  MOZ_ASSERT(mInputStream);
>+  MOZ_ASSERT(mWeakIPCSerializableInputStream);
early return if these are null, but I guess since there is no error reporting in this method, keeping the assertions is ok too, to catch
at least some issues in debug builds.


>+SlicedInputStream::Deserialize(const mozilla::ipc::InputStreamParams& aParams,
>+                               const FileDescriptorArray& aFileDescriptors)
>+{
and ok, in this case it is fine if mInputStream and mWeakIPCSerializableInputStream are null
Perhaps even assert that mInputStream and mWeakIPCSerializableInputStream are null.


>+mozilla::Maybe<uint64_t>
>+SlicedInputStream::ExpectedSerializedLength()
>+{
>+  MOZ_ASSERT(mInputStream);
>+  MOZ_ASSERT(mWeakIPCSerializableInputStream);
return 0 if mWeakIPCSerializableInputStream is null
Attachment #8838534 - Flags: review?(bugs) → review+
Comment on attachment 8838535 [details] [diff] [review]
part 3 - SlicedInputStream must be seekable

>+SlicedInputStream::Seek(int32_t aWhence, int64_t aOffset)
>+{
>+  MOZ_ASSERT(mInputStream);
>+  MOZ_ASSERT(mWeakSeekableInputStream);
null check, not asserts

>+NS_IMETHODIMP
>+SlicedInputStream::Tell(int64_t *aResult)
* goes with the type

>+{
>+  MOZ_ASSERT(mInputStream);
>+  MOZ_ASSERT(mWeakSeekableInputStream);
null check these.

>+NS_IMETHODIMP
>+SlicedInputStream::SetEOF()
>+{
>+  MOZ_ASSERT(mInputStream);
>+  MOZ_ASSERT(mWeakSeekableInputStream);
null check
Attachment #8838535 - Flags: review?(bugs) → review+
This is needed otherwise non-e10s XHR tests will consider the stream a buffered stream. We don't want that. The issue starts with NS_InputStreamIsBuffered(). This tried to read 1 byte but, the writerFunc will return error just to see if the stream is buffered.
Attachment #8839512 - Flags: review?(bugs)
Attachment #8839512 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7561692c1bc7
SlicedInputStream should not be nsICloneableInputStream if the source stream is not, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4761a15d45fd
SlicedInputStream should be an nsIIPCSerializableInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c685dca493f5
Remove nsIPartialFileInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb4e1c2aada
SlicedInputStream should be nsISeekableStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c0ec55b44dc
Remove the implementation of SlicedInputStream::ReadSegmenets, r=smaug
sorry had to back this out for GTest failures in TestSlicedInputStream.StartBiggerThan like:
https://treeherder.mozilla.org/logviewer.html#?job_id=79261781&repo=mozilla-inbound&lineNumber=3321

https://hg.mozilla.org/integration/mozilla-inbound/rev/94e98e7e3227
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b42bc5273c
SlicedInputStream should not be nsICloneableInputStream if the source stream is not, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d14d2a3ec60
SlicedInputStream should be an nsIIPCSerializableInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/78926e4f66c2
Remove nsIPartialFileInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/efd6af45175f
SlicedInputStream should be nsISeekableStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d7fcf276fdd
Remove the implementation of SlicedInputStream::ReadSegments, r=smaug
Flags: needinfo?(amarchesini)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.