1460561 - nsMultiplexInputStream should behave correctly when NS_InputStreamIsBuffered() is used

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrea Marchesini [:baku]]

NS_InputStreamIsBuffered() checks if the inputStream implements ReadSegments() correctly.
But if nsMultiplexInputStream contains more than 1 stream, and just the first one supports ReadSegments(), NS_InputStreamIsBuffered() will return true.

See more in bug 1436809.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: multistream.patch

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 8974666 [details] [diff] [review]
multistream.patch

Review of attachment 8974666 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. Looks good, but you might need to have r+ from xpcom peer...

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8974666 [details] [diff] [review]
multistream.patch

Review of attachment 8974666 [details] [diff] [review]:
-----------------------------------------------------------------

This review took way longer than it should have; my fault.

Just to make sure I understand the situation: comment 0 says that NS_InputStreamIsBuffered will only check the first stream for multiplex streams, is that correct?  And so if we add non-buffered streams to a multiplex stream after the first, then NS_InputStreamIsBuffered will return true, when it should be false?

Why are we not fixing this by fixing the NS_InputStreamIsBuffered check and avoiding forcing multiplex stream clients to always accept buffering?

Comment 4

[Andrea Marchesini [:baku]]

See comment 18: https://bugzilla.mozilla.org/show_bug.cgi?id=1436809#c18

"This won't work when nsMultiplexInputStream is hidden behind some other stream. Let's say I'll land the ReadSegments implementation of SlicedInputStream, then we can have something like this:

sliced stream -> muxed stream -> ( string stream + file input stream )"

I also suggested to introduce a new interface and move ReadSegments() in that new interface, but that is a huge amount of work.

Comment 5

[Nathan Froyd [:froydnj]]

And I suppose the sliced stream can't explicitly inform the muxed stream that it wants buffering on all of its substreams?  Or the sliced stream doesn't even know if the underlying stream is a muxed stream?  We could check that the sliced stream's substream is a muxed stream, and force buffering if so...but I guess that leads us to sprinkling checks for muxed streams everywhere, which is unattractive.  And if muxed streams can be created without buffering, and then find themselves requiring buffering at some later point, that creates its own set of issues.

Ugh.  It'd be so much nicer if NS_InputStreamIsBuffered just worked off straight interfaces...but maybe we'd still have this problem.

I think we may have to reluctantly accept buffering everywhere.  Can you please add a test to demonstrate that your patch will make ReadSegments (or buffering, whichever) work on multiplexed streams when you add a buffered stream and then an unbuffered stream?

Comment 6

[Andrea Marchesini [:baku]]

> And I suppose the sliced stream can't explicitly inform the muxed stream
> that it wants buffering on all of its substreams?  Or the sliced stream
> doesn't even know if the underlying stream is a muxed stream?  We could

Sliced don't know that the calling of ReadSegments() is executed in order to know if the stream is buffered.
Sliced (like many other 'meta' streams, such as buffered, mime, IPCBlob and so on) just forwards the request to the underlying stream.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: multistream_test.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54c98a0a7d1603b2c7513e9aacf49d0c689af7fe&selectedJob=179545901

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8979441 [details] [diff] [review]
multistream_test.patch

Review of attachment 8979441 [details] [diff] [review]:
-----------------------------------------------------------------

WFM, thank you!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eea17fb77684
nsMultiplexInputStream should behave correctly when NS_InputStreamIsBuffered() is used, r=michal
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc358abeba45
nsMultiplexInputStream should behave correctly when NS_InputStreamIsBuffered() is used - gtests, r=froydnj

Comment 10

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (bug 1460561) for failures in toolkit/components/extensions/test/mochitest/test_ext_webrequest_upload.html on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=dc358abeba458b601226ccf23beaae07dc5dd323&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=179957369

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=179957369&repo=mozilla-inbound&lineNumber=13011

Backout push: https://hg.mozilla.org/integration/mozilla-inbound/rev/06f3d7764da18886a3caeb282528ad11a267d21d

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 1

In order to make tests happy, GetStream() must return the original stream and not the buffered one.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8982007 [details] [diff] [review]
part 1

Review of attachment 8982007 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +254,5 @@
> +    nsCOMPtr<nsIInputStream> bufferedStream;
> +    nsresult rv = NS_NewBufferedInputStream(getter_AddRefs(bufferedStream),
> +                                            stream.forget(), 4096);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    stream = bufferedStream;

bufferedStream.forget()?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ea106440d1b
nsMultiplexInputStream should behave correctly when NS_InputStreamIsBuffered() is used, r=michal, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/fae4d7e57052
nsMultiplexInputStream should behave correctly when NS_InputStreamIsBuffered() is used - gtests, r=froydnj

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1ea106440d1b
https://hg.mozilla.org/mozilla-central/rev/fae4d7e57052