Make AutoIPCStream favor PSendStream when passed giant string streams

RESOLVED FIXED in Firefox 51

Status

()

Core
IPC
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bkelly, Assigned: jdm)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 unaffected, firefox50+ wontfix, firefox51+ fixed, firefox52+ fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Currently AutoIPCStream will attempt to use direct serialization when passed a particular stream.  String streams are just converted to a buffer directly.

This is problematic when we get passed an extremely large string stream.  We have experienced a number of OOM problems with trying to directly send fixed length streams with many 100s MB of data.

It would be nice to expose an "expected buffer size" or "should I be directly serialized" API on nsIIPCSerializableInputStream.  Then AutoIPCStream could use this to prefer PSendStream for these giant streams.

This would help bugs like bug 1277681
(Assignee)

Comment 3

a year ago
Created attachment 8793758 [details] [diff] [review]
Make AutoIPCStream favour PSendStream for large input streams

Try seems to approve of this change.
Attachment #8793758 - Flags: review?(bkelly)
(Assignee)

Updated

a year ago
Assignee: nobody → josh
Status: NEW → ASSIGNED
(Reporter)

Comment 4

a year ago
Comment on attachment 8793758 [details] [diff] [review]
Make AutoIPCStream favour PSendStream for large input streams

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

Looks reasonable.  Thanks for taking this!  I think for a sweeping stream change like this perhaps :froydnj should review as well.

::: ipc/glue/IPCStreamUtils.cpp
@@ +129,5 @@
>    nsCOMPtr<nsIIPCSerializableInputStream> serializable =
>      do_QueryInterface(aStream);
> +  uint64_t expectedLength =
> +    serializable ? serializable->ExpectedStreamLength().valueOr(0) : 0;
> +  if (serializable && expectedLength < kTooLargeStream) {

What should we do if its too large here, but its not an async non-blocking stream as required below?  Should we still serialize? Or we could automatically construct a pipe, copy on STS, and send the resulting pipe reader?  As written here the code crashes.

::: ipc/glue/nsIIPCSerializableInputStream.h
@@ +40,5 @@
>    Deserialize(const mozilla::ipc::InputStreamParams& aParams,
>                const FileDescriptorArray& aFileDescriptors) = 0;
> +
> +  virtual mozilla::Maybe<uint64_t>
> +  ExpectedStreamLength() = 0;

Can you document this new method?  What should it return if the stream cannot actually be serialized for some reason?

Also, it might be good to make the name specific to serialization to avoid confusion.  Something like `ExpectedSerializedLength()` or something.

::: netwerk/base/nsBufferedStreams.cpp
@@ +551,5 @@
> +    if (stream) {
> +        return stream->ExpectedStreamLength();
> +    }
> +    uint64_t available;
> +    return NS_SUCCEEDED(Available(&available)) ? Some(available) : Nothing();

This seems wrong.  If the underlying stream is not serializable then we should expect no serialized length, right?

::: netwerk/base/nsMIMEInputStream.cpp
@@ +383,5 @@
> +
> +Maybe<uint64_t>
> +nsMIMEInputStream::ExpectedStreamLength()
> +{
> +    return Nothing();

Shouldn't this get the length from mData?

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +759,5 @@
> +  uint32_t streamCount = mStreams.Length();
> +  for (uint32_t index = 0; index < streamCount; index++) {
> +    nsCOMPtr<nsIIPCSerializableInputStream> stream = do_QueryInterface(mStreams[index]);
> +    if (!stream) {
> +      return Nothing();

continue

@@ +763,5 @@
> +      return Nothing();
> +    }
> +    Maybe<uint64_t> length = stream->ExpectedStreamLength();
> +    if (length.isNothing()) {
> +      return Nothing();

continue
Attachment #8793758 - Flags: review?(bkelly) → feedback+
Blocks: 1288997
[Tracking Requested - why for this release]: Same as bug 1288997 comment 16.
status-firefox49: --- → unaffected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Tracking 52+.
tracking-firefox52: ? → +
Track 51+ as this is related to OOM for large string stream.
tracking-firefox51: ? → +
(Assignee)

Comment 8

a year ago
Created attachment 8795043 [details] [diff] [review]
Make AutoIPCStream favour PSendStream for large input streams

Addressed all of bkelly's comments.
Attachment #8795043 - Flags: review?(nfroyd)
(Assignee)

Updated

a year ago
Attachment #8793758 - Attachment is obsolete: true
Comment on attachment 8795043 [details] [diff] [review]
Make AutoIPCStream favour PSendStream for large input streams

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

r=me with two minor comments.

::: ipc/glue/IPCStreamUtils.cpp
@@ +158,5 @@
> +
> +    rv = NS_AsyncCopy(aStream, sink, target, NS_ASYNCCOPY_VIA_READSEGMENTS, kBufferSize);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +
> +    asyncStream = do_QueryInterface(stream);

I think we can avoid this QI if we used NS_NewPipe2 above:

nsresult rv = NS_NewPipe2(getter_AddRefs(asyncStream), getter_AddRefs(sink), true, false, kBufferSize, UINT32_MAX);

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +767,5 @@
> +      continue;
> +    }
> +    expectedLength += length.value();
> +  }
> +  return Some(expectedLength);

To unconditionally indicate this seems incorrect; what if none of the streams are serializable, in which case the documentation for ExpectedSerializedLength suggests that we should return Nothing()?  What if all of the streams are, but all of the calls to ExpectedSerializedLength return Nothing()?  It seems more correct to have expectedLength start out as Nothing(), and then accumulate sub-lengths into that, if possible.
Attachment #8795043 - Flags: review?(nfroyd) → review+
Tracked for Fx50
tracking-firefox50: ? → +
(Assignee)

Comment 12

a year ago
Created attachment 8795917 [details] [diff] [review]
Make AutoIPCStream favour PSendStream for large input streams
(Assignee)

Updated

a year ago
Attachment #8795043 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 13

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e78c503c4e
Make AutoIPCStream favour PSendStream for large input streams. r=froydnj
Keywords: checkin-needed
(Assignee)

Comment 16

a year ago
Created attachment 8796152 [details] [diff] [review]
Bug 1294450 - Make AutoIPCStream favour PSendStream for large input streams.
Attachment #8795917 - Attachment is obsolete: true
Flags: needinfo?(josh)
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 17

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6856e101c6
Make AutoIPCStream favour PSendStream for large input streams. r=nfroydj
Keywords: checkin-needed

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e6856e101c6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi Josh, could you please nominate uplift to Aurora51 and Beta50?
Flags: needinfo?(josh)
Depends on: 1306979
(Assignee)

Comment 20

a year ago
We probably shouldn't uplift this until we solve bug 1306979, since this appears to have made a different set of crashes much more prominent.
Flags: needinfo?(josh)
(In reply to Josh Matthews [:jdm] from comment #20)
> We probably shouldn't uplift this until we solve bug 1306979, since this
> appears to have made a different set of crashes much more prominent.

Hi Josh, https://bugzilla.mozilla.org/show_bug.cgi?id=1306979#c12. 50 and 51 are marked as unaffected on that one.  Are we ready for comment 19 now?
Flags: needinfo?(josh)
(Assignee)

Comment 22

a year ago
I don't think this is worth uplifting separately from bug 1277681, which converts the HTTP request body to use the code that this bug modifies.
Flags: needinfo?(josh)
Thanks Josh! It appears that the fix isn't quite ready and perhaps too risky to take in the remainder (RC week starting Monday) of beta cycle. This is now a wontfix for 50.
status-firefox50: affected → wontfix
Hi :jdm,
Do you mean the fixes of bug 1277681 & this should be uplift together?
Flags: needinfo?(josh)
(Assignee)

Comment 25

a year ago
Yes, I mean that bug 1277681 and this bug together avoid the large IPC crashes. Independently, neither is a complete fix.
Flags: needinfo?(josh)
Hi :jdm,
could you please nominate this and bug 1277681 uplift to Aurora51?
Flags: needinfo?(josh)
(Assignee)

Comment 27

a year ago
Comment on attachment 8796152 [details] [diff] [review]
Bug 1294450 - Make AutoIPCStream favour PSendStream for large input streams.

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: If either the patch in bug 1277681 or this patch are not uplifted, it will remain impossible to upload files larger than 128mb because the content process will crash.
[Describe test coverage new/current, TreeHerder]: We have lots of existing test coverage for uploads that did not regress with these changes.
[Risks and why]: Low. The changes in this patch are redirecting one code path to another, and both have had lots of testing to shake out bugs in the past.
[String/UUID change made/needed]: None.
Flags: needinfo?(josh)
Attachment #8796152 - Flags: approval-mozilla-aurora?
Comment on attachment 8796152 [details] [diff] [review]
Bug 1294450 - Make AutoIPCStream favour PSendStream for large input streams.

Avoid large IPC crash. Take the patch together with the patch of 1277681 in 51 aurora.
Attachment #8796152 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 29

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e0d815a12d03
status-firefox51: affected → fixed
You need to log in before you can comment on or make changes to this bug.