Last Comment Bug 1166504 - Make nsMultiplexInputStream cloneable
: Make nsMultiplexInputStream cloneable
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: mozilla41
Assigned To: Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
:
: Selena Deckelmann :selenamarie :selena use ni? pronoun: she
Mentors:
: 1165668 (view as bug list)
Depends on:
Blocks: ServiceWorkers-v1
  Show dependency treegraph
 
Reported: 2015-05-19 14:28 PDT by Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
Modified: 2015-06-10 16:28 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
MozReview Request: bz://1166504/nsm (39 bytes, text/x-review-board-request)
2015-05-19 14:31 PDT, Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
no flags Details
MozReview Request: Bug 1166504 - Make nsMultiplexInputStream cloneable. (39 bytes, text/x-review-board-request)
2015-05-29 11:22 PDT, Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
froydnj: review+
Details

Description User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-05-19 14:28:09 PDT
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.
Comment 1 User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-05-19 14:31:05 PDT
Created attachment 8607771 [details]
MozReview Request: bz://1166504/nsm

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

Pull down this commit:

hg pull -r fc7e7df1535338de85c8f30912d6dc2f12466218 https://reviewboard-hg.mozilla.org/gecko/
Comment 2 User image Ben Kelly [:bkelly, not reviewing] 2015-05-19 14:34:16 PDT
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.
Comment 3 User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-05-20 08:12:16 PDT
(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.
Comment 4 User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-05-20 08:15:47 PDT
*** Bug 1165668 has been marked as a duplicate of this bug. ***
Comment 5 User image Nathan Froyd [:froydnj] 2015-05-21 12:48:15 PDT
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.
Comment 6 User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-05-28 21:46:52 PDT
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?
Comment 7 User image Nathan Froyd [:froydnj] 2015-05-29 06:31:31 PDT
(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.
Comment 8 User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-05-29 11:22:06 PDT
Created attachment 8613008 [details]
MozReview Request: Bug 1166504 - Make nsMultiplexInputStream cloneable.

Bug 1166504 - Make nsMultiplexInputStream cloneable.
Comment 9 User image Nathan Froyd [:froydnj] 2015-06-01 08:27:48 PDT
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 10 User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-06-01 12:45:24 PDT
Comment on attachment 8613008 [details]
MozReview Request: Bug 1166504 - Make nsMultiplexInputStream cloneable.

Bug 1166504 - Make nsMultiplexInputStream cloneable.
Comment 11 User image Nathan Froyd [:froydnj] 2015-06-01 12:48:51 PDT
Comment on attachment 8613008 [details]
MozReview Request: Bug 1166504 - Make nsMultiplexInputStream cloneable.

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

Ship It!
Comment 12 User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-06-01 13:39:36 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf840dcc62a6
Comment 13 User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-06-01 15:14:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/47a103414177
Comment 14 User image Wes Kocher (:KWierso) 2015-06-01 18:08:20 PDT
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
Comment 15 User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-06-03 10:29:13 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56536d531e62
Comment 16 User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-06-04 16:27:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3be7f71fbf72
Comment 17 User image Carsten Book [:Tomcat] 2015-06-05 06:31:13 PDT
https://hg.mozilla.org/mozilla-central/rev/3be7f71fbf72
Comment 18 User image Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2015-06-10 08:49:56 PDT
Comment on attachment 8607771 [details]
MozReview Request: bz://1166504/nsm

Note You need to log in before you can comment on or make changes to this bug.