No reason these don't work on substrings.
Remove StripChars since it has an existing nsTSubstring version. Make
callers use rightly-typed characters for those.
Differential Revision: https://phabricator.services.mozilla.com/D145864
When the NS_AsyncCopy completes, there may still be outstanding
AsyncWait callbacks which have not been invoked yet, due to two
AsyncWait callbacks being registered each time Process() yields (one to
wait for the blocked stream, and the other with WAIT_CLOSURE_ONLY to
handle errors), and only one callback being needed to resume processing.
This change ensures that any outstanding AsyncWait callbacks are
cancelled, breaking references from the streams to the
nsAStreamCallback. Some streams (such as nsPipe and DataPipe) may also
leak if there are oustanding AsyncWait callbacks, due to the need to
keep the stream alive so it can be passed into the callback, which this
helps avoid.
Previously leaks were largely avoided due to the call to `Close()`
cancelling the callbacks for us, however not all callsites specify to
close the source/sink.
This should also help avoid an unnecessary dispatch after the copy
completes due to our AsyncWait callback being invoked when the
source/sink stream is closed.
Differential Revision: https://phabricator.services.mozilla.com/D146130
If we don't do this, we can encounter issues where we'll spuriously close the
stream when the last reference to the input stream is dropped while an
AsyncWait is still pending.
Differential Revision: https://phabricator.services.mozilla.com/D145672
As discovered by TSan, this member could be raced on in some edge-cases if
cloned pipe streams raced on multiple threads to be destroyed. This change
instead moves the `nsIPipe` implementation to be on a seperate type from
`nsPipe`, allowing `mOriginalInput` to no longer need to be tracked.
This technically has slightly different behaviour than the previous
implementation for JS code, as code keeping the `nsIPipe` alive will now also
be keeping the output stream reference alive directly, rather than only holding
the `nsIPipe`, meaning that `pipe.outputStream` can be read multiple times
independently without implicitly closing the underlying stream. Based on a
quick read of the few remaining uses of `nsIPipe` in JS code, I don't think
this will be an issue, and it may actually be more intuitive and consistent.
Differential Revision: https://phabricator.services.mozilla.com/D145362
Before this change, we could run into issues when an async stream
accepts new data and queues a runnable to notify the stream listener,
but before that stream listener's runnable is run, the target thread
reads data from the async stream, emptying it of any available data.
This would mean that the stream appears empty in
nsMultiplexInputStream::OnInputStreamReady, and the rest of the stream
is skipped.
This patch changes the logic in OnInputStreamReady to re-call AsyncWait
in those situations and avoid skipping over the empty stream, preventing
data loss.
This situation came up when running some HTTP tests under rr with the
socket process enabled, as the upload stream can both be read due to an
OnInputStreamReady callback and due to other calls in the HTTP
machinery, leading to this potential race. With the socket process
enabled, it is possible for the upload stream to be an async pipe, which
doesn't happen in the parent process due to stream normalization.
This change makes an assumption about the behaviour of streams
implementing `nsIAsyncInputStream` which I believe is correct right now,
and has been documented. I originally had other patches which modified
`Available()`'s definition to add an extra outparameter or added extra
methods, but this seemed much simpler and accurate based on my memory of
having read the Available() implementations for async streams in-tree.
A gtest was added for the failing situation using both nsPipe and DataPipe.
Differential Revision: https://phabricator.services.mozilla.com/D144451
As serializing IPCStream no longer requires a manager or FileDescriptor array,
the arguments are no longer necessary, and can be removed. The AutoIPCStream
helper can also be removed, as managed actors are no longer used for
serialization, so a delayed start callback is not necessary.
The delayed start parameter is also removed from nsIIPCSerializableInputStream
instances, but is still present as `aAllowLazy` on the toplevel serialization
methods.
Differential Revision: https://phabricator.services.mozilla.com/D141048
This type is no longer necessary, and has various issues due to behaving
incorrectly when used with async streams or streams which are not nsIPipe.
Differential Revision: https://phabricator.services.mozilla.com/D141045
Unfortunately, upload streams used by necko have various odd behaviours
and requirements which happened to be usually preserved by the previous
IPC serialization logic, but were not consistently preserved. This
includes requiring the stream to be synchronous (as some consumers such
as WebExtensions and DevTools appear to read it assuming Available() is
the stream length), seekable (as it needs to be rewound in various
places), and cloneable (as the stream information is often handed out to
other components).
In addition, the WebExtension WebRequest code makes assumptions about
the specific topology of the input stream for optimization purposes,
meaning that nsMultiplexInputStreams need to be preserved.
The way this was previously handled was by copying the entire payload
into a nsStorageStream as an async operation. This happened very
infrequently in out test suite, however, and had some issues. It could
lead to data loss if the stream was a nsMIMEInputStream (as the metadata
would be lost), and would destroy the topology required by WebRequest.
This patch changes the code to instead manually walk and replace streams
in the input stream's data structure, to efficiently copy only the
required data, preserve the invariants, and make the type seekable
before AsyncOpen continues. This helps keep the complexity of the
invariants HTTPChannel depends on out of generic input stream handling
code.
In addition, due to how early this happens, it replaces the need for
PartiallySeekableInputStream which will be removed a later part.
Differential Revision: https://phabricator.services.mozilla.com/D141044
This works only because the only implementation of nsIAsyncInputStreamLength
happened to only run on the current thread. Bug 1754004, changes this, and requires the reference to be threadsafe.
Differential Revision: https://phabricator.services.mozilla.com/D141037
This will improve the efficiency of serializing large data pipes, which in bad
cases can end up serializing very small amounts of data in individual pipes
over IPC, and acts as a compliment to the existing logic for limiting
serialized message sizes. It is also necessary for the changes in bug 1754004,
which require the ability to include FileDescriptor inline in the message,
which could blow out our FileDescriptor count limits if pipe creation was
unlimited.
In some tests, this change reduces the number of pipes required to serialize a
nsIInputStream from over 5000 to 1.
Differential Revision: https://phabricator.services.mozilla.com/D141036
This will help in bug 1754004 as in some cases we will want to clone a
DataPipe, which is non-cloneable, and without this change length information
would be lost.
In the future it might be worth making a more generic mechanism for efficiently
cloning wrapper input streams with a fallback rather than hard-coding this
interface.
Differential Revision: https://phabricator.services.mozilla.com/D141035
This operation is often performed by nsAStreamCopier when switching
between the source and sink streams, in order to enable or disable the
WAIT_CLOSURE_ONLY flag. Failing to reset the wait flags can lead to a
NS_AsyncCopy hanging until the source or sink are closed when is
alternating between waiting on input and output streams. This patch
relaxes the incorrect checks on various input streams.
Differential Revision: https://phabricator.services.mozilla.com/D141034
If we don't do this, we can encounter issues where we'll spuriously close the
stream when the last reference to the input stream is dropped while an
AsyncWait is still pending.
Differential Revision: https://phabricator.services.mozilla.com/D145672
As discovered by TSan, this member could be raced on in some edge-cases if
cloned pipe streams raced on multiple threads to be destroyed. This change
instead moves the `nsIPipe` implementation to be on a seperate type from
`nsPipe`, allowing `mOriginalInput` to no longer need to be tracked.
This technically has slightly different behaviour than the previous
implementation for JS code, as code keeping the `nsIPipe` alive will now also
be keeping the output stream reference alive directly, rather than only holding
the `nsIPipe`, meaning that `pipe.outputStream` can be read multiple times
independently without implicitly closing the underlying stream. Based on a
quick read of the few remaining uses of `nsIPipe` in JS code, I don't think
this will be an issue, and it may actually be more intuitive and consistent.
Differential Revision: https://phabricator.services.mozilla.com/D145362
Before this change, we could run into issues when an async stream
accepts new data and queues a runnable to notify the stream listener,
but before that stream listener's runnable is run, the target thread
reads data from the async stream, emptying it of any available data.
This would mean that the stream appears empty in
nsMultiplexInputStream::OnInputStreamReady, and the rest of the stream
is skipped.
This patch changes the logic in OnInputStreamReady to re-call AsyncWait
in those situations and avoid skipping over the empty stream, preventing
data loss.
This situation came up when running some HTTP tests under rr with the
socket process enabled, as the upload stream can both be read due to an
OnInputStreamReady callback and due to other calls in the HTTP
machinery, leading to this potential race. With the socket process
enabled, it is possible for the upload stream to be an async pipe, which
doesn't happen in the parent process due to stream normalization.
This change makes an assumption about the behaviour of streams
implementing `nsIAsyncInputStream` which I believe is correct right now,
and has been documented. I originally had other patches which modified
`Available()`'s definition to add an extra outparameter or added extra
methods, but this seemed much simpler and accurate based on my memory of
having read the Available() implementations for async streams in-tree.
A gtest was added for the failing situation using both nsPipe and DataPipe.
Differential Revision: https://phabricator.services.mozilla.com/D144451
As serializing IPCStream no longer requires a manager or FileDescriptor array,
the arguments are no longer necessary, and can be removed. The AutoIPCStream
helper can also be removed, as managed actors are no longer used for
serialization, so a delayed start callback is not necessary.
The delayed start parameter is also removed from nsIIPCSerializableInputStream
instances, but is still present as `aAllowLazy` on the toplevel serialization
methods.
Differential Revision: https://phabricator.services.mozilla.com/D141048
This type is no longer necessary, and has various issues due to behaving
incorrectly when used with async streams or streams which are not nsIPipe.
Differential Revision: https://phabricator.services.mozilla.com/D141045
Unfortunately, upload streams used by necko have various odd behaviours
and requirements which happened to be usually preserved by the previous
IPC serialization logic, but were not consistently preserved. This
includes requiring the stream to be synchronous (as some consumers such
as WebExtensions and DevTools appear to read it assuming Available() is
the stream length), seekable (as it needs to be rewound in various
places), and cloneable (as the stream information is often handed out to
other components).
In addition, the WebExtension WebRequest code makes assumptions about
the specific topology of the input stream for optimization purposes,
meaning that nsMultiplexInputStreams need to be preserved.
The way this was previously handled was by copying the entire payload
into a nsStorageStream as an async operation. This happened very
infrequently in out test suite, however, and had some issues. It could
lead to data loss if the stream was a nsMIMEInputStream (as the metadata
would be lost), and would destroy the topology required by WebRequest.
This patch changes the code to instead manually walk and replace streams
in the input stream's data structure, to efficiently copy only the
required data, preserve the invariants, and make the type seekable
before AsyncOpen continues. This helps keep the complexity of the
invariants HTTPChannel depends on out of generic input stream handling
code.
In addition, due to how early this happens, it replaces the need for
PartiallySeekableInputStream which will be removed a later part.
Differential Revision: https://phabricator.services.mozilla.com/D141044
This works only because the only implementation of nsIAsyncInputStreamLength
happened to only run on the current thread. Bug 1754004, changes this, and requires the reference to be threadsafe.
Differential Revision: https://phabricator.services.mozilla.com/D141037
This will improve the efficiency of serializing large data pipes, which in bad
cases can end up serializing very small amounts of data in individual pipes
over IPC, and acts as a compliment to the existing logic for limiting
serialized message sizes. It is also necessary for the changes in bug 1754004,
which require the ability to include FileDescriptor inline in the message,
which could blow out our FileDescriptor count limits if pipe creation was
unlimited.
In some tests, this change reduces the number of pipes required to serialize a
nsIInputStream from over 5000 to 1.
Differential Revision: https://phabricator.services.mozilla.com/D141036
This will help in bug 1754004 as in some cases we will want to clone a
DataPipe, which is non-cloneable, and without this change length information
would be lost.
In the future it might be worth making a more generic mechanism for efficiently
cloning wrapper input streams with a fallback rather than hard-coding this
interface.
Differential Revision: https://phabricator.services.mozilla.com/D141035
This operation is often performed by nsAStreamCopier when switching
between the source and sink streams, in order to enable or disable the
WAIT_CLOSURE_ONLY flag. Failing to reset the wait flags can lead to a
NS_AsyncCopy hanging until the source or sink are closed when is
alternating between waiting on input and output streams. This patch
relaxes the incorrect checks on various input streams.
Differential Revision: https://phabricator.services.mozilla.com/D141034
If we don't do this, we can encounter issues where we'll spuriously close the
stream when the last reference to the input stream is dropped while an
AsyncWait is still pending.
Depends on D145363
Differential Revision: https://phabricator.services.mozilla.com/D145672
As discovered by TSan, this member could be raced on in some edge-cases if
cloned pipe streams raced on multiple threads to be destroyed. This change
instead moves the `nsIPipe` implementation to be on a seperate type from
`nsPipe`, allowing `mOriginalInput` to no longer need to be tracked.
This technically has slightly different behaviour than the previous
implementation for JS code, as code keeping the `nsIPipe` alive will now also
be keeping the output stream reference alive directly, rather than only holding
the `nsIPipe`, meaning that `pipe.outputStream` can be read multiple times
independently without implicitly closing the underlying stream. Based on a
quick read of the few remaining uses of `nsIPipe` in JS code, I don't think
this will be an issue, and it may actually be more intuitive and consistent.
Differential Revision: https://phabricator.services.mozilla.com/D145362
Before this change, we could run into issues when an async stream
accepts new data and queues a runnable to notify the stream listener,
but before that stream listener's runnable is run, the target thread
reads data from the async stream, emptying it of any available data.
This would mean that the stream appears empty in
nsMultiplexInputStream::OnInputStreamReady, and the rest of the stream
is skipped.
This patch changes the logic in OnInputStreamReady to re-call AsyncWait
in those situations and avoid skipping over the empty stream, preventing
data loss.
This situation came up when running some HTTP tests under rr with the
socket process enabled, as the upload stream can both be read due to an
OnInputStreamReady callback and due to other calls in the HTTP
machinery, leading to this potential race. With the socket process
enabled, it is possible for the upload stream to be an async pipe, which
doesn't happen in the parent process due to stream normalization.
This change makes an assumption about the behaviour of streams
implementing `nsIAsyncInputStream` which I believe is correct right now,
and has been documented. I originally had other patches which modified
`Available()`'s definition to add an extra outparameter or added extra
methods, but this seemed much simpler and accurate based on my memory of
having read the Available() implementations for async streams in-tree.
A gtest was added for the failing situation using both nsPipe and DataPipe.
Differential Revision: https://phabricator.services.mozilla.com/D144451
As serializing IPCStream no longer requires a manager or FileDescriptor array,
the arguments are no longer necessary, and can be removed. The AutoIPCStream
helper can also be removed, as managed actors are no longer used for
serialization, so a delayed start callback is not necessary.
The delayed start parameter is also removed from nsIIPCSerializableInputStream
instances, but is still present as `aAllowLazy` on the toplevel serialization
methods.
Differential Revision: https://phabricator.services.mozilla.com/D141048
This type is no longer necessary, and has various issues due to behaving
incorrectly when used with async streams or streams which are not nsIPipe.
Differential Revision: https://phabricator.services.mozilla.com/D141045
Unfortunately, upload streams used by necko have various odd behaviours
and requirements which happened to be usually preserved by the previous
IPC serialization logic, but were not consistently preserved. This
includes requiring the stream to be synchronous (as some consumers such
as WebExtensions and DevTools appear to read it assuming Available() is
the stream length), seekable (as it needs to be rewound in various
places), and cloneable (as the stream information is often handed out to
other components).
In addition, the WebExtension WebRequest code makes assumptions about
the specific topology of the input stream for optimization purposes,
meaning that nsMultiplexInputStreams need to be preserved.
The way this was previously handled was by copying the entire payload
into a nsStorageStream as an async operation. This happened very
infrequently in out test suite, however, and had some issues. It could
lead to data loss if the stream was a nsMIMEInputStream (as the metadata
would be lost), and would destroy the topology required by WebRequest.
This patch changes the code to instead manually walk and replace streams
in the input stream's data structure, to efficiently copy only the
required data, preserve the invariants, and make the type seekable
before AsyncOpen continues. This helps keep the complexity of the
invariants HTTPChannel depends on out of generic input stream handling
code.
In addition, due to how early this happens, it replaces the need for
PartiallySeekableInputStream which will be removed a later part.
Differential Revision: https://phabricator.services.mozilla.com/D141044
This works only because the only implementation of nsIAsyncInputStreamLength
happened to only run on the current thread. Bug 1754004, changes this, and requires the reference to be threadsafe.
Differential Revision: https://phabricator.services.mozilla.com/D141037
This will improve the efficiency of serializing large data pipes, which in bad
cases can end up serializing very small amounts of data in individual pipes
over IPC, and acts as a compliment to the existing logic for limiting
serialized message sizes. It is also necessary for the changes in bug 1754004,
which require the ability to include FileDescriptor inline in the message,
which could blow out our FileDescriptor count limits if pipe creation was
unlimited.
In some tests, this change reduces the number of pipes required to serialize a
nsIInputStream from over 5000 to 1.
Differential Revision: https://phabricator.services.mozilla.com/D141036
This will help in bug 1754004 as in some cases we will want to clone a
DataPipe, which is non-cloneable, and without this change length information
would be lost.
In the future it might be worth making a more generic mechanism for efficiently
cloning wrapper input streams with a fallback rather than hard-coding this
interface.
Differential Revision: https://phabricator.services.mozilla.com/D141035
This operation is often performed by nsAStreamCopier when switching
between the source and sink streams, in order to enable or disable the
WAIT_CLOSURE_ONLY flag. Failing to reset the wait flags can lead to a
NS_AsyncCopy hanging until the source or sink are closed when is
alternating between waiting on input and output streams. This patch
relaxes the incorrect checks on various input streams.
Differential Revision: https://phabricator.services.mozilla.com/D141034
As discovered by TSan, this member could be raced on in some edge-cases if
cloned pipe streams raced on multiple threads to be destroyed. This change
instead moves the `nsIPipe` implementation to be on a seperate type from
`nsPipe`, allowing `mOriginalInput` to no longer need to be tracked.
This technically has slightly different behaviour than the previous
implementation for JS code, as code keeping the `nsIPipe` alive will now also
be keeping the output stream reference alive directly, rather than only holding
the `nsIPipe`, meaning that `pipe.outputStream` can be read multiple times
independently without implicitly closing the underlying stream. Based on a
quick read of the few remaining uses of `nsIPipe` in JS code, I don't think
this will be an issue, and it may actually be more intuitive and consistent.
Depends on D142127
Differential Revision: https://phabricator.services.mozilla.com/D145362
Before this change, we could run into issues when an async stream
accepts new data and queues a runnable to notify the stream listener,
but before that stream listener's runnable is run, the target thread
reads data from the async stream, emptying it of any available data.
This would mean that the stream appears empty in
nsMultiplexInputStream::OnInputStreamReady, and the rest of the stream
is skipped.
This patch changes the logic in OnInputStreamReady to re-call AsyncWait
in those situations and avoid skipping over the empty stream, preventing
data loss.
This situation came up when running some HTTP tests under rr with the
socket process enabled, as the upload stream can both be read due to an
OnInputStreamReady callback and due to other calls in the HTTP
machinery, leading to this potential race. With the socket process
enabled, it is possible for the upload stream to be an async pipe, which
doesn't happen in the parent process due to stream normalization.
This change makes an assumption about the behaviour of streams
implementing `nsIAsyncInputStream` which I believe is correct right now,
and has been documented. I originally had other patches which modified
`Available()`'s definition to add an extra outparameter or added extra
methods, but this seemed much simpler and accurate based on my memory of
having read the Available() implementations for async streams in-tree.
A gtest was added for the failing situation using both nsPipe and DataPipe.
Differential Revision: https://phabricator.services.mozilla.com/D144451
As serializing IPCStream no longer requires a manager or FileDescriptor array,
the arguments are no longer necessary, and can be removed. The AutoIPCStream
helper can also be removed, as managed actors are no longer used for
serialization, so a delayed start callback is not necessary.
The delayed start parameter is also removed from nsIIPCSerializableInputStream
instances, but is still present as `aAllowLazy` on the toplevel serialization
methods.
Differential Revision: https://phabricator.services.mozilla.com/D141048
This type is no longer necessary, and has various issues due to behaving
incorrectly when used with async streams or streams which are not nsIPipe.
Differential Revision: https://phabricator.services.mozilla.com/D141045
Nika Layzell
Randell Jesup
Iulian Moraru
Emilio Cobos Álvarez
Barret Rennie
Butkovits Atila
Noemi Erli
Neil Deakin
Norisz Fay
Sebastian Hengst