Closed
Bug 1339710
Opened 7 years ago
Closed 7 years ago
Merge SlicedInputStream and nsIPartialFileInputStream
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
Details
Attachments
(5 files, 4 obsolete files)
39.58 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
3.73 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
10.99 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
5.77 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.76 KB,
text/plain
|
smaug
:
review+
|
Details |
nsIPartialFileInputStream is nsIFileInputStream + SlicedInputStream. We can get rid of it.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8837453 -
Flags: review?(bugs)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8837454 -
Flags: review?(bugs)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8837453 -
Attachment is obsolete: true
Attachment #8837453 -
Flags: review?(bugs)
Attachment #8837542 -
Flags: review?(bugs)
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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-
Assignee | ||
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
(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.
Comment 9•7 years ago
|
||
(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).
Assignee | ||
Comment 10•7 years ago
|
||
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.
Comment 11•7 years ago
|
||
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.
Comment 12•7 years ago
|
||
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.
Comment 13•7 years ago
|
||
I feel like I'm missing something here. Want to clarify?
Assignee | ||
Comment 14•7 years ago
|
||
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.
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8838125 -
Flags: review?(bugs)
Comment 16•7 years ago
|
||
(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 17•7 years ago
|
||
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-
Updated•7 years ago
|
Attachment #8837454 -
Flags: review?(bugs) → review+
Comment 18•7 years ago
|
||
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-
Assignee | ||
Comment 19•7 years ago
|
||
Can we discuss this issue on IRC?
Assignee | ||
Comment 20•7 years ago
|
||
Attachment #8838533 -
Flags: review?(bugs)
Assignee | ||
Comment 21•7 years ago
|
||
Attachment #8837967 -
Attachment is obsolete: true
Attachment #8838534 -
Flags: review?(bugs)
Assignee | ||
Comment 22•7 years ago
|
||
Attachment #8838125 -
Attachment is obsolete: true
Attachment #8838535 -
Flags: review?(bugs)
Comment 23•7 years ago
|
||
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 24•7 years ago
|
||
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 25•7 years ago
|
||
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+
Assignee | ||
Comment 26•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8839512 -
Flags: review?(bugs) → review+
Comment 27•7 years ago
|
||
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
Comment 28•7 years ago
|
||
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)
Comment 29•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(amarchesini)
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e3b42bc5273c https://hg.mozilla.org/mozilla-central/rev/8d14d2a3ec60 https://hg.mozilla.org/mozilla-central/rev/78926e4f66c2 https://hg.mozilla.org/mozilla-central/rev/efd6af45175f https://hg.mozilla.org/mozilla-central/rev/0d7fcf276fdd
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•