Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Description • 9 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 1 • 9 years ago

/r/9077 - Bug 1166504 - Make nsMultiplexInputStream cloneable.

Pull down this commit:

hg pull -r fc7e7df1535338de85c8f30912d6dc2f12466218 https://reviewboard-hg.mozilla.org/gecko/

	Ben Kelly [:bkelly, not reviewing]
	Comment 2 • 9 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 3 • 9 years ago

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

	Nathan Froyd [:froydnj]
	Comment 5 • 9 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 6 • 9 years ago

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?

	Nathan Froyd [:froydnj]
	Comment 7 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 8 • 9 years ago

Bug 1166504 - Make nsMultiplexInputStream cloneable.

	Nathan Froyd [:froydnj]
	Comment 9 • 9 years ago

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?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 10 • 9 years ago

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

Bug 1166504 - Make nsMultiplexInputStream cloneable.

	Nathan Froyd [:froydnj]
	Comment 11 • 9 years ago

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

https://reviewboard.mozilla.org/r/9077/#review8565

Ship It!

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 12 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf840dcc62a6

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 13 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/47a103414177

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 15 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56536d531e62

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3be7f71fbf72

	Carsten Book [:Tomcat]
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/3be7f71fbf72