There are only 3 places where nsMemory.h is still needed (image/RasterImage.cpp,
gfx/thebes/gfxFT2FontList.cpp, and nsMemory.cpp). Remove the rest.
Differential Revision: https://phabricator.services.mozilla.com/D158213
This patch changes how nsConverterInputStream handles passing data
through to the underlying unicode converter in order to make it more
reliably handle propagating errors and deal with short reads from the
underlying input stream.
This was done by making the code continuously read within the Fill
method until at least one character has been decoded from the input
stream, so that we don't spuriously communicate an EOF to the caller due
to a short read not producing enough bytes for the decoder to produce a
UTF-16 character.
In addition, while making this change it became easier to signal to
the decoder about the final read from the input stream, meaning that
partial characters at the end of the stream will now generate a
replacement character, rather than being ignored.
Differential Revision: https://phabricator.services.mozilla.com/D152682
This patch changes how nsConverterInputStream handles passing data
through to the underlying unicode converter in order to make it more
reliably handle propagating errors and deal with short reads from the
underlying input stream.
This was done by making the code continuously read within the Fill
method until at least one character has been decoded from the input
stream, so that we don't spuriously communicate an EOF to the caller due
to a short read not producing enough bytes for the decoder to produce a
UTF-16 character.
In addition, while making this change it became easier to signal to
the decoder about the final read from the input stream, meaning that
partial characters at the end of the stream will now generate a
replacement character, rather than being ignored.
Differential Revision: https://phabricator.services.mozilla.com/D152682
When `nsMultiplexInputStream` calls `AsyncWait` from within
`OnInputStreamReady`, it is possible for those calls to fail. In that case, we
will fail to notify AsyncWait listeners, which could lead to leaks or stream
hangs. This patch changes the error handling in this situation to instead fire
a pending stream callback after an error when re-dispatching `AsyncWait`.
Depends on D153628
Differential Revision: https://phabricator.services.mozilla.com/D153629
Previously nsMultiplexInputStream would not call `AsyncWait` again when the
callback was cleared, meaning that the underlying wait wouldn't actually be
cancelled. This patch changes the behaviour to more consistently forward the
cancelling call to the underlying stream.
There are still some cases where the call will not be cancelled if a read or
other event caused the stream cursor to advance before canceling, however those
should generally be uncommon.
Differential Revision: https://phabricator.services.mozilla.com/D153628
The biggest set of APIs from ns[T]StringObsolete which are still heavily used
are the string searching APIs. It appears the intention was for these to be
replaced by the `FindInReadable` APIs, however that doesn't appear to have
happened.
In addition, the APIs have some quirks around their handling of mixed character
widths. These APIs generally supported both narrow strings and the native
string type, probably because char16_t string literals weren't available until
c++11. Finally they also used easy-to-confuse unlabeled boolean and integer
optional arguments to control behaviour.
These patches do the following major changes to the searching APIs:
1. The ASCII case-insensitive search method was split out as
LowerCaseFindASCII, rather than using a boolean. This should be less
error-prone and more explicit, and allows the method to continue to use
narrow string literals for all string types (as only ASCII is supported).
2. The other [R]Find methods were restricted to only support arguments with
matching character types. I considered adding a FindASCII method which would
use narrow string literals for both wide and narrow strings but it would've
been the same amount of work as changing all of the literals to unicode
literals.
This ends up being the bulk of the changes in the patch.
3. All find methods were re-implemented using std::basic_string_view's find
algorithm or stl algorithms to reduce code complexity, and avoid the need to
carry around the logic from nsStringObsolete.cpp.
4. The implementations were moved to nsTStringRepr.cpp.
5. An overload of Find was added to try to catch callers which previously
called `Find(..., false)` or `Find(..., true)` to set case-sensitivity, due
to booleans normally implicitly coercing to `index_type`. This should
probably be removed at some point, but may be useful during the transition.
Differential Revision: https://phabricator.services.mozilla.com/D148300
This patch moves EqualsIgnoreCase to ns[T]StringObsolete, and removes
the aCount argument, instead migrating callers to use `StringBeginsWith`
with a case-insensitive comparator.
In addition, nsTStringRepr::Compare was removed and replaced with either
calls to methods like `StringBeginsWith` or the global `Compare` method.
These changes required some modifications at call-sites but should make
the behaviour less surprising and more consistent.
Differential Revision: https://phabricator.services.mozilla.com/D148299
Previously, we could end up in situations where nsMultiplexInputStream
would receive a callback for a previously notified stream after it had
already advanced to the next stream due to the async nature of
notifications. This could cause us to skip a stream accidentally. This
changes the logic to re-call AsyncWait in that situation on the correct
stream rather than skipping streams.
Differential Revision: https://phabricator.services.mozilla.com/D152930
The biggest set of APIs from ns[T]StringObsolete which are still heavily used
are the string searching APIs. It appears the intention was for these to be
replaced by the `FindInReadable` APIs, however that doesn't appear to have
happened.
In addition, the APIs have some quirks around their handling of mixed character
widths. These APIs generally supported both narrow strings and the native
string type, probably because char16_t string literals weren't available until
c++11. Finally they also used easy-to-confuse unlabeled boolean and integer
optional arguments to control behaviour.
These patches do the following major changes to the searching APIs:
1. The ASCII case-insensitive search method was split out as
LowerCaseFindASCII, rather than using a boolean. This should be less
error-prone and more explicit, and allows the method to continue to use
narrow string literals for all string types (as only ASCII is supported).
2. The other [R]Find methods were restricted to only support arguments with
matching character types. I considered adding a FindASCII method which would
use narrow string literals for both wide and narrow strings but it would've
been the same amount of work as changing all of the literals to unicode
literals.
This ends up being the bulk of the changes in the patch.
3. All find methods were re-implemented using std::basic_string_view's find
algorithm or stl algorithms to reduce code complexity, and avoid the need to
carry around the logic from nsStringObsolete.cpp.
4. The implementations were moved to nsTStringRepr.cpp.
5. An overload of Find was added to try to catch callers which previously
called `Find(..., false)` or `Find(..., true)` to set case-sensitivity, due
to booleans normally implicitly coercing to `index_type`. This should
probably be removed at some point, but may be useful during the transition.
Differential Revision: https://phabricator.services.mozilla.com/D148300
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
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
dom/bindings/BindingUtils.cpp(202,62): error: passing object of class type 'typename raw_type<char16_t, int>::type' (aka 'char16ptr_t') through variadic function [-Werror,-Wclass-varargs]
static_cast<unsigned>(errorNumber), funcNameStr.get(),
^
dom/bindings/BindingUtils.cpp(203,26): error: passing object of class type 'typename raw_type<char16_t, int>::type' (aka 'char16ptr_t') through variadic function [-Werror,-Wclass-varargs]
ifaceName.get());
^
dom/media/webrtc/transport/third_party/nICEr/src/ice/ice_component.c(582,15): error: passing object of class type 'nr_transport_addr' (aka 'struct nr_transport_addr_') through variadic function [-Werror,-Wclass-varargs]
component->stream->turn_servers[j].turn_server.addr);
^
toolkit/xre/dllservices/tests/gtest/TestUntrustedModules.cpp(44,45): error: passing object of class type 'typename raw_type<char16_t, int>::type' (aka 'char16ptr_t') through variadic function [-Werror,-Wclass-varargs]
wprintf(L"%s is not registered.\n", aNames[i].get());
^
toolkit/xre/dllservices/tests/gtest/TestUntrustedModules.cpp(49,30): error: passing object of class type 'typename raw_type<char16_t, int>::type' (aka 'char16ptr_t') through variadic function [-Werror,-Wclass-varargs]
wprintf(L"%s:%4d\n", aNames[i].get(), *entry);
^
toolkit/xre/dllservices/tests/gtest/TestUntrustedModules.cpp(248,30): error: passing object of class type 'typename raw_type<char16_t, int>::type' (aka 'char16ptr_t') through variadic function [-Werror,-Wclass-varargs]
wprintf(L"JSON: %s\n", json.get());
^
xpcom/io/nsLocalFileWin.cpp(1647,20): error: passing object of class type 'typename raw_type<char16_t, int>::type' (aka 'char16ptr_t') through variadic function [-Werror,-Wclass-varargs]
NS_ConvertASCIItoUTF16(nsDependentCString(aField)).get());
^
Differential Revision: https://phabricator.services.mozilla.com/D144665
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
dom/media/platforms/wmf/WMFEncoderModule.cpp(31,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case MediaDataEncoder::CodecType::VP9:
^
dom/media/platforms/wmf/WMFEncoderModule.cpp(31,5): note: insert '[[fallthrough]];' to silence this warning
case MediaDataEncoder::CodecType::VP9:
^
[[fallthrough]];
netwerk/test/gtest/TestNamedPipeService.cpp(212,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
default: // error
^
netwerk/test/gtest/TestNamedPipeService.cpp(212,5): note: insert '[[fallthrough]];' to silence this warning
default: // error
^
[[fallthrough]];
widget/windows/KeyboardLayout.cpp(1973,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
default:
^
widget/windows/KeyboardLayout.cpp(1973,5): note: insert 'U_FALLTHROUGH;' to silence this warning
default:
^
U_FALLTHROUGH;
widget/windows/WinMouseScrollHandler.cpp(633,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case SB_PAGEDOWN:
^
widget/windows/WinMouseScrollHandler.cpp(633,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case SB_PAGEDOWN:
^
U_FALLTHROUGH;
widget/windows/WinMouseScrollHandler.cpp(640,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case SB_LINEDOWN:
^
widget/windows/WinMouseScrollHandler.cpp(640,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case SB_LINEDOWN:
^
U_FALLTHROUGH;
widget/windows/nsLookAndFeel.cpp(188,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case ColorID::MozMenuhovertext:
^
widget/windows/nsLookAndFeel.cpp(188,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case ColorID::MozMenuhovertext:
^
U_FALLTHROUGH;
widget/windows/nsLookAndFeel.cpp(194,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case ColorID::Highlighttext:
^
widget/windows/nsLookAndFeel.cpp(194,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case ColorID::Highlighttext:
^
U_FALLTHROUGH;
widget/windows/nsLookAndFeel.cpp(469,15): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case ABE_BOTTOM:
^
widget/windows/nsLookAndFeel.cpp(469,15): note: insert 'U_FALLTHROUGH;' to silence this warning
case ABE_BOTTOM:
^
U_FALLTHROUGH;
widget/windows/nsNativeThemeWin.cpp(2540,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case StyleAppearance::Button:
^
widget/windows/nsNativeThemeWin.cpp(2540,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case StyleAppearance::Button:
^
U_FALLTHROUGH;
widget/windows/nsNativeThemeWin.cpp(3278,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case StyleAppearance::Checkbox:
^
widget/windows/nsNativeThemeWin.cpp(3278,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case StyleAppearance::Checkbox:
^
U_FALLTHROUGH;
widget/windows/nsNativeThemeWin.cpp(3332,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case StyleAppearance::Tabpanel:
^
widget/windows/nsNativeThemeWin.cpp(3332,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case StyleAppearance::Tabpanel:
^
U_FALLTHROUGH;
widget/windows/nsNativeThemeWin.cpp(3461,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case StyleAppearance::Menuarrow: {
^
widget/windows/nsNativeThemeWin.cpp(3461,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case StyleAppearance::Menuarrow: {
^
U_FALLTHROUGH;
widget/windows/nsWindow.cpp(1339,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case eWindowType_toplevel:
^
widget/windows/nsWindow.cpp(1339,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case eWindowType_toplevel:
^
U_FALLTHROUGH;
widget/windows/nsWindow.cpp(1422,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case eWindowType_toplevel:
^
widget/windows/nsWindow.cpp(1422,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case eWindowType_toplevel:
^
U_FALLTHROUGH;
widget/windows/nsWindow.cpp(3379,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case eTransparencyGlass:
^
widget/windows/nsWindow.cpp(3379,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case eTransparencyGlass:
^
U_FALLTHROUGH;
widget/windows/nsWindow.cpp(5595,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case WM_MOUSELEAVE: {
^
widget/windows/nsWindow.cpp(5595,5): note: insert 'U_FALLTHROUGH;' to silence this warning
case WM_MOUSELEAVE: {
^
U_FALLTHROUGH;
xpcom/io/SpecialSystemDirectory.cpp(572,5): error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case Win_Programs: {
^
xpcom/io/SpecialSystemDirectory.cpp(572,5): note: insert '[[fallthrough]];' to silence this warning
case Win_Programs: {
^
[[fallthrough]];
Differential Revision: https://phabricator.services.mozilla.com/D144668
This makes the getters more consistent with getters on other platforms,
and theoretically provides a faster way of getting these directories
from C++ code in the future.
Differential Revision: https://phabricator.services.mozilla.com/D142871
This should make it slightly safer to to try to read from a
nsStorageInputStream while the stream is being concurrently written to
on another thread, though it will still not behave very well as there is
no nsIAsyncInputStream support.
Individual input streams are still non-threadsafe with this change.
Differential Revision: https://phabricator.services.mozilla.com/D142135
These helper methods are similar to the related ones for
nsIInputStream::ReadSegments, and can be used to implement
nsIOutputStream::Write and nsIOutputStream::WriteFrom in terms of
WriteSegments.
There were various places which used manual copies of these methods, which are
being unified.
Differential Revision: https://phabricator.services.mozilla.com/D138334
This change aims to make the way that the nsStringInputStream owns the backing
data buffer more flexible. This has a few impacts, including allowing
arbitrarily large payload sizes on 64-bit platforms, not requiring as complex
checks around borrowed string buffers or nsTArray data storage, and supporting
custom data ownership schemes such as those used by blobs.
The new system uses a separate refcounted object internally to provide the
contiguous backing buffer, with the nsStringInputStream wrapping it and
providing the `nsIInputStream` interface and cursor. This also avoids issues
around the buffer being mutated during reads, as mutating the `nsIInputStream`
no longer mutates the actual data storage object.
Differential Revision: https://phabricator.services.mozilla.com/D135162
While processing third-party module loading events, the process may delayload
shlwapi.dll. This could be a problem if the process is sandboxed and the access
to local files is restricted. Before enabling the third-party modules ping in
socket process, this patch avoids such delayloading.
The first path is `ParamTraits<mozilla::ModuleRecord>::Read` calls `NS_NewLocalFile`
that calls `PathGetDriveNumberW`. This API is simple enough that we define our own
version.
The second path is the ctor of `ProcessedModuleLoadEvent` that is called from
`UntrustedModulesProcessor::CompleteProcessing` calls `PreparePathForTelemetry`
that calls `PathFindFileNameW` and `PathCanonicalizeW`. For this path, instead
of sanitizing a requested name in `ProcessedModuleLoadEvent`'s ctor, we sanitize
it when transferring it to the main process.
Differential Revision: https://phabricator.services.mozilla.com/D134492
Barret Rennie
Jan Varga
Robert Longson
Jari Jalkanen
Cristian Tuns
Alexandre Lissy
Emilio Cobos Álvarez
Jens Stutte
Andrew McCreight
Nika Layzell
Marian-Vasile Laza
Norisz Fay
Fabrice Desré
Andreea Pavel
Jan Rio Krause
serge-sans-paille
criss
Gijs Kruitbosch
Chris Peterson
Butkovits Atila
Iulian Moraru
Randell Jesup
Noemi Erli
Neil Deakin
Sebastian Hengst
Csoregi Natalia
Mike Hommey
Stephen A Pohl
Nathan Froyd
Masatoshi Kimura
Nicholas Rishel
Paul Zuehlcke
Toshihito Kikuchi
Sandor Molnar