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.

/r/9077 - Bug 1166504 - Make nsMultiplexInputStream cloneable. Pull down this commit: hg pull -r fc7e7df1535338de85c8f30912d6dc2f12466218 https://reviewboard-hg.mozilla.org/gecko/

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.

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?

(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.

Bug 1166504 - Make nsMultiplexInputStream cloneable.

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?

Comment on attachment 8613008 [details] MozReview Request: Bug 1166504 - Make nsMultiplexInputStream cloneable. Bug 1166504 - Make nsMultiplexInputStream cloneable.

Comment on attachment 8613008 [details] MozReview Request: Bug 1166504 - Make nsMultiplexInputStream cloneable. https://reviewboard.mozilla.org/r/9077/#review8565 Ship It!

Backed out for being the likely cause of mochitest-e10s-4 permafail: https://treeherder.mozilla.org/logviewer.html#?job_id=10336004&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/3ce3e50dae0f