Closed Bug 1166504 Opened 10 years ago Closed 10 years ago

Make nsMultiplexInputStream cloneable

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: nsm, Assigned: nsm)

References

Details

Attachments

(1 file, 1 obsolete file)

Attempting to send formdata or other nsIXHRSendables results in a multiplex input stream. This should be cloneable when possible so we don't clone it using a pipe and replace the original stream in HttpChannelChild with the pipe when attempting to set a Request body for the serviceworker. Pipe input stream is not IPC serializable.
Attached file MozReview Request: bz://1166504/nsm (obsolete) —
/r/9077 - Bug 1166504 - Make nsMultiplexInputStream cloneable. Pull down this commit: hg pull -r fc7e7df1535338de85c8f30912d6dc2f12466218 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8607771 - Flags: review?(nfroyd)
https://reviewboard.mozilla.org/r/9075/#review7701 ::: xpcom/io/nsMultiplexInputStream.cpp:806 (Diff revision 1) > + nsCOMPtr<nsIMultiplexInputStream> clone = new nsMultiplexInputStream(); I think you need to copy the internal state of the nsMultiplexInputStream as well. For example, mCurrentStream, mStartedReadingCurrent, mStatus, etc.
(In reply to Ben Kelly [:bkelly] from comment #2) > https://reviewboard.mozilla.org/r/9075/#review7701 > > ::: xpcom/io/nsMultiplexInputStream.cpp:806 > (Diff revision 1) > > + nsCOMPtr<nsIMultiplexInputStream> clone = new nsMultiplexInputStream(); > > I think you need to copy the internal state of the nsMultiplexInputStream as > well. For example, mCurrentStream, mStartedReadingCurrent, mStatus, etc. Do we really want that, they are all markers of the stream already being read. We should only be cloning streams that haven't started being read yet. Maybe fail GetCloneable() if a read is in progress.
https://reviewboard.mozilla.org/r/9077/#review7863 The patch is fine as-is, but something needs to be done about comment 2 and comment 3--either code changes to implement comment 3 or a justification of why the current behavior is OK. I think that, even if we're in the middle of reading some streams from a multiplex stream, we clone the multiplex stream, and then we try to read from the clone that The Right Thing (i.e. we'll start reading from where we were before) will happen. We'll just spend some extra time checking the streams we've already read from for EOF. So changes may not be needed, but some tests to confirm that's the case would be good. ::: xpcom/io/nsMultiplexInputStream.cpp:829 (Diff revision 1) > + asInputStream.forget(aClone); You should be able to do just: clone.forget(aClone); and the compiler will take care of the necessary casting for you. No need to QI and refcount things unnecessarily. ::: xpcom/io/nsMultiplexInputStream.cpp:806 (Diff revision 1) > + nsCOMPtr<nsIMultiplexInputStream> clone = new nsMultiplexInputStream(); I think this should be nsRefPtr<nsMultiplexInpuStream>, just to keep things concrete where they can be concrete. But if that change makes the change below with returning things harder, then the code as-is is fine.
Attachment #8607771 - Flags: review?(nfroyd)
Nathan, nsMultiplexInputStream asserts if the AppendStream() stream is not at its beginning (https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsMultiplexInputStream.cpp?from=nsMultiplexInputStream.cpp#141). Should I circumvent that by just filling in the array or should we just fail the clone if the multiplex stream has already started reading?
Flags: needinfo?(nfroyd)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #6) > Nathan, nsMultiplexInputStream asserts if the AppendStream() stream is not > at its beginning > (https://dxr.mozilla.org/mozilla-central/source/xpcom/io/ > nsMultiplexInputStream.cpp?from=nsMultiplexInputStream.cpp#141). Should I > circumvent that by just filling in the array or should we just fail the > clone if the multiplex stream has already started reading? I don't understand what "filling in the array" means in this context. Can you clarify? Given the use case from comment 0, it sounds like failing the clone if we've already started reading is OK. I'm not familiar with how the streams would be used in the scenario described in comment 0, though.
Flags: needinfo?(nfroyd)
Comment on attachment 8613008 [details] MozReview Request: Bug 1166504 - Make nsMultiplexInputStream cloneable. https://reviewboard.mozilla.org/r/9077/#review8551 Looks good, would like to see just a few more tests to make sure we're covering all the bases. ::: xpcom/tests/gtest/TestCloneInputStream.cpp:145 (Diff revision 2) > + // Stream that has been read should fail. Thanks for adding these tests. More requests for tests: 1) Test that cloning a stream where we've finished reading the first stream, but haven't started reading the second fails. 2) Test that cloning a stream where we've started reading the second stream fails. 3) Test that cloning a stream where we've read everything out of the stream fails. ::: xpcom/io/nsMultiplexInputStream.cpp:842 (Diff revision 2) > + clone.forget(aClone); Did it work to make things |nsRefPtr<nsMultiplexInputStream> clone| or not?
Attachment #8613008 - Flags: review?(nfroyd)
Comment on attachment 8613008 [details] MozReview Request: Bug 1166504 - Make nsMultiplexInputStream cloneable. Bug 1166504 - Make nsMultiplexInputStream cloneable.
Attachment #8613008 - Flags: review?(nfroyd)
Comment on attachment 8613008 [details] MozReview Request: Bug 1166504 - Make nsMultiplexInputStream cloneable. https://reviewboard.mozilla.org/r/9077/#review8565 Ship It!
Attachment #8613008 - Flags: review?(nfroyd) → review+
Assignee: nobody → nsm.nikhil
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Flags: needinfo?(nsm.nikhil)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: