Closed
Bug 1166504
Opened 10 years ago
Closed 10 years ago
Make nsMultiplexInputStream cloneable
Categories
(Core :: Networking, defect)
Core
Networking
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.
Assignee | ||
Updated•10 years ago
|
Blocks: ServiceWorkers-v1
Assignee | ||
Comment 1•10 years ago
|
||
/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)
Comment 2•10 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.
Assignee | ||
Comment 3•10 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.
Comment 5•10 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.
Updated•10 years ago
|
Attachment #8607771 -
Flags: review?(nfroyd)
Assignee | ||
Comment 6•10 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?
Flags: needinfo?(nfroyd)
Comment 7•10 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.
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 8•10 years ago
|
||
Bug 1166504 - Make nsMultiplexInputStream cloneable.
Attachment #8613008 -
Flags: review?(nfroyd)
Comment 9•10 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?
Attachment #8613008 -
Flags: review?(nfroyd)
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8613008 [details]
MozReview Request: Bug 1166504 - Make nsMultiplexInputStream cloneable.
Bug 1166504 - Make nsMultiplexInputStream cloneable.
Attachment #8613008 -
Flags: review?(nfroyd)
Comment 11•10 years ago
|
||
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 | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 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
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
Assignee: nobody → nsm.nikhil
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8607771 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(nsm.nikhil)
You need to log in
before you can comment on or make changes to this bug.
Description
•