Closed
Bug 1479407
Opened 6 years ago
Closed 6 years ago
nsMultiplexInputStream::AppendElement should be fallible
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
References
Details
Attachments
(1 file)
4.12 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
We should return NS_ERROR_OUT_OF_MEMORY if appending a substream fails.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #8995942 -
Flags: review?(nfroyd)
Comment 2•6 years ago
|
||
Comment on attachment 8995942 [details] [diff] [review] multiplexInputStream.patch Review of attachment 8995942 [details] [diff] [review]: ----------------------------------------------------------------- I don't know that the FallibleTArray changes are necessary and would like to revert them, but I'm not going to insist. ::: xpcom/io/nsMultiplexInputStream.cpp @@ +129,5 @@ > bool IsAsyncInputStreamLength() const; > > Mutex mLock; // Protects access to all data members. > > + FallibleTArray<StreamData> mStreams; We've been trying to reduce the number of places where FallibleTArray is used, and just have people use fallible operations on nsTArray. Is the intent here to ensure that people always use fallible operations on mStreams? @@ +307,5 @@ > > NS_IMETHODIMP > nsMultiplexInputStream::Close() > { > + FallibleTArray<nsCOMPtr<nsIInputStream>> streams; Same comment applies here. I think the case for leaving this as nsTArray is stronger, because this is just a local variable, and so the likelihood of inadvertently using non-fallible operations is lower. @@ +1375,5 @@ > > CheckedInt64 mLength; > bool mNegativeSize; > > + FallibleTArray<nsCOMPtr<nsIAsyncInputStreamLength>> mPendingStreams; Same comment here.
Attachment #8995942 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 3•6 years ago
|
||
I'm totally OK to use the fallible version of nsTArray. How far are we to make the fallible versions the default? And remove the infallible...?
Comment 4•6 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #3) > I'm totally OK to use the fallible version of nsTArray. How far are we to > make the fallible versions the default? And remove the infallible...? I'm not sure I understand the question. We have: - FallibleTArray: only supports fallible AppendElement and the like; - nsTArray: supports both infallible and fallible AppendElement and the like. We had FallibleTArray because once upon a time, we wanted the type of the array to determine the kind of operations you could use on it. But then we decided that it's more important to have the fallibility of the operation be visible at the call site of the operation, rather than the type of variable being operated on. So we added all the fallible operations to nsTArray...at which point FallibleTArray doesn't serve a lot of purpose. Does that address the concern behind the question?
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7fbc1f375d4a nsMultiplexInputStream::AppendElement should be fallible, r=froydnj
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7fbc1f375d4a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•