1339713 - MOZ_CRASH(IPC message size is too large) with PContent::Msg_PBlobConstructor

Status: RESOLVED FIXED

Description

[Kan-Ru Chen [:kanru] (UTC+8)]

This bug was filed from the Socorro interface and is 
report bp-8b685fc6-12c1-449b-85b6-5ca422170215.
=============================================================

The signature still appears after bug 1288997 has been fixed but has much lower volume.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - PWebBrowserPersistDocument

We should use AutoIPCStream everywhere we can in order to avoid this crash.
AutoIPCStream uses PChildToParentStream in case the size of the package is > 1mb.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - PFTPChannel

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - PUDPSocket

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - PWebSocket

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 5 - KeywordToURI

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 6 - clean up serialize/deserialize methods

Comment 7

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d76c4c732cf78d003818f17a05ff804f576a2bed&selectedJob=81511688
green on try.

Comment 8

[Olli Pettay [:smaug] (vacation May 27-31)]

So this relies on some undocumented side effect of AutoIPCStream?

Comment 9

[Olli Pettay [:smaug] (vacation May 27-31)]

I'm trying to understand what it means to not pass FileDescriptors around.

Comment 10

[Olli Pettay [:smaug] (vacation May 27-31)]

Comment on attachment 8843285 [details] [diff] [review]
part 1 - PWebBrowserPersistDocument

> #include "WebBrowserPersistSerializeChild.h"
> 
> namespace mozilla {
> 
>@@ -57,22 +58,24 @@ WebBrowserPersistDocumentChild::Start(ns
>     ENSURE(aDocument->GetContentType(attrs.contentType()));
>     ENSURE(aDocument->GetCharacterSet(attrs.characterSet()));
>     ENSURE(aDocument->GetTitle(attrs.title()));
>     ENSURE(aDocument->GetReferrer(attrs.referrer()));
>     ENSURE(aDocument->GetContentDisposition(attrs.contentDisposition()));
>     ENSURE(aDocument->GetCacheKey(&(attrs.cacheKey())));
>     ENSURE(aDocument->GetPersistFlags(&(attrs.persistFlags())));
>     ENSURE(aDocument->GetPostData(getter_AddRefs(postDataStream)));
>-    ipc::SerializeInputStream(postDataStream,
>-                              postData,
>-                              postFiles);
> #undef ENSURE
>+
>+    mozilla::ipc::AutoIPCStream autoStream;
>+    autoStream.Serialize(postDataStream,
>+                         static_cast<mozilla::dom::ContentChild*>(Manager()));
>+
>     mDocument = aDocument;
>-    SendAttributes(attrs, postData, postFiles);
>+    SendAttributes(attrs, autoStream.TakeOptionalValue());
Nothing uses postData and postFiles anymore


(But I don't yet understand the overall setup here)

Comment 11

[Olli Pettay [:smaug] (vacation May 27-31)]

Comment on attachment 8843288 [details] [diff] [review]
part 4 - PWebSocket

> class BinaryStreamEvent : public Runnable
> {
> public:
>   BinaryStreamEvent(WebSocketChannelChild *aChild,
>-                    OptionalInputStreamParams *aStream,
>+                    UniquePtr<AutoIPCStream>&& aStream,
>                     uint32_t aLength)
>     : mChild(aChild)
>-    , mStream(aStream)
>+    , mStream(Move(aStream))
>     , mLength(aLength)
>   {
>     MOZ_RELEASE_ASSERT(!NS_IsMainThread());
>     MOZ_ASSERT(aChild);
>   }
>   NS_IMETHOD Run() override
>   {
>     MOZ_ASSERT(NS_IsMainThread());
>     nsresult rv = mChild->SendBinaryStream(mStream, mLength);
>     if (NS_FAILED(rv)) {
>       LOG(("WebSocketChannelChild::BinaryStreamEvent %p "
>            "SendBinaryStream failed (%08" PRIx32 ")\n", this, static_cast<uint32_t>(rv)));
>     }
>     return NS_OK;
>   }
> private:
>-  RefPtr<WebSocketChannelChild>      mChild;
>-  nsAutoPtr<OptionalInputStreamParams> mStream;
>-  uint32_t                             mLength;
>+  RefPtr<WebSocketChannelChild> mChild;
>+  UniquePtr<AutoIPCStream> mStream;
>+  uint32_t mLength;
bkelly recommended not using AutoIPCStream this way on heap where deletion time is random.
So, pass input stream to this class and create AutoIPCStream when needed. There shouldn't be
need to create AutoIPCStream beforehand.
AutoIPCStream is after all designed to be used in RAII way (heap allocation may still happen if store in some stack only object).

Comment 12

[Olli Pettay [:smaug] (vacation May 27-31)]

Comment on attachment 8843291 [details] [diff] [review]
part 6 - clean up serialize/deserialize methods

Drop Internal from the name. I don't understand why that is now needed.

Comment 13

[Olli Pettay [:smaug] (vacation May 27-31)]

Comment on attachment 8843287 [details] [diff] [review]
part 3 - PUDPSocket

> UDPSocketChild::SendBinaryStream(const nsACString& aHost,
>                                  uint16_t aPort,
>                                  nsIInputStream* aStream)
> {
>   NS_ENSURE_ARG(aStream);
> 
>-  OptionalInputStreamParams stream;
>-  nsTArray<mozilla::ipc::FileDescriptor> fds;
>-  SerializeInputStream(aStream, stream, fds);
>-
>-  MOZ_ASSERT(fds.IsEmpty());
>+  mozilla::ipc::AutoIPCStream autoStream;
>+  autoStream.Serialize(aStream,
>+                       static_cast<mozilla::dom::ContentChild*>(gNeckoChild->Manager()));
> 
>   UDPSOCKET_LOG(("%s: %s:%u", __FUNCTION__, PromiseFlatCString(aHost).get(), aPort));
>-  SendOutgoingData(UDPData(stream), UDPSocketAddr(UDPAddressInfo(nsCString(aHost), aPort)));
>+  SendOutgoingData(UDPData(autoStream.TakeOptionalValue()),
AutoIPCStream is a really weird beast. Had to read the implementation to understand that TakeOptionalValue should work here.

Comment 14

[Olli Pettay [:smaug] (vacation May 27-31)]

Comment on attachment 8843285 [details] [diff] [review]
part 1 - PWebBrowserPersistDocument

So, remove the useless postData and postFiles variables.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 4 - PWebSocket

smaug, can you take this patch?

Comment 16

[Andrea Marchesini [:baku]]

> Drop Internal from the name. I don't understand why that is now needed.

I think it's important to avoid people to use these methods. Now they are used only by inputStreams to serialize themselves. This methods should not be used by any other component.

Comment 17

[Olli Pettay [:smaug] (vacation May 27-31)]

Then hide the method somehow. Make it a protected method of some class and make the accepted callers friend classes or something?

Comment 18

[Olli Pettay [:smaug] (vacation May 27-31)]

Comment on attachment 8844788 [details] [diff] [review]
part 4 - PWebSocket

Ok, here we rely on the same thing as in BlobImpl that relevant inputstream impls are threadsafe.

Comment 19

[Olli Pettay [:smaug] (vacation May 27-31)]

Before landing any of this, please test memory usage when uploading some huge (GB) files.

Also, even without uploading, but just using <input type=file> to access some File objects, doesn't this cause us to keep the memory allocated in the child, even though we could just always read it from file?

We need to decrease our memory consumption, not increase.

Comment 20

[Olli Pettay [:smaug] (vacation May 27-31)]

er, this are about child to parent, so shouldn't affect to that.

Comment 21

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] (high review load, queue closed for couple of days) from comment #20)
> er, this are about child to parent, so shouldn't affect to that.

Right. Plus, FileInputStream return Nothing() in ExpectedSerializedLength(). This means that we always serialize them (moving file descriptors).

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 5 - fixing AutoIPCStream when inputStream is null

This bit was in the PChildToParentStream patches. It's needed for landing this.

Comment 23

[Olli Pettay [:smaug] (vacation May 27-31)]

Comment on attachment 8845004 [details] [diff] [review]
part 5 - fixing AutoIPCStream when inputStream is null

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  b273d463b6258b393818f1396d3b5940903cce38
>
>diff --git a/ipc/glue/IPCStreamUtils.cpp b/ipc/glue/IPCStreamUtils.cpp
>--- a/ipc/glue/IPCStreamUtils.cpp
>+++ b/ipc/glue/IPCStreamUtils.cpp
>@@ -255,16 +255,36 @@ CleanupIPCStream(OptionalIPCStream& aVal
> {
>   if (aValue.type() == OptionalIPCStream::Tvoid_t) {
>     return;
>   }
> 
>   CleanupIPCStream(aValue.get_IPCStream(), aConsumedByIPC);
> }
> 
>+// Returns false if the serialization should not proceed. This means that the
>+// inputStream is null.
>+bool
>+NormalizeOptionalValue(nsIInputStream* aStream,
>+                       IPCStream* aValue,
>+                       OptionalIPCStream* aOptionalValue)
>+{
>+  if (aValue) {
>+    return true;
>+  }
This looks wrong. If aStream is null, having just non-null aValue isn't ok.
Or do we want to crash in debug builds then later when serialization asserts?


>+
>+  if (!aStream) {
>+    *aOptionalValue = void_t();
>+    return false;
>+  }
SerializeInputStream already does this.
But ahaa, SerializeInputStreamWithFdsParent doesn't. Should there be a version of SerializeInputStreamWithFdsParent
which takes OptionalIPCStream&, as there is such version of SerializeInputStream


So I don't quite understand this patch.
Wouldn't it be enough to change MOZ_ASSERT(aStream); asserts
to MOZ_ASSERT(aStream || !mValue); and fix SerializeInputStreamWithFdsParent

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 5 - fixing AutoIPCStream when inputStream is null

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4bcf2c7d260
Use IPCStream everywhere - part 1 - PWebBrowserPersistDocument, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/df17e65b3785
Use IPCStream everywhere - part 2 - PFTPChannel, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cdf685ace68
Use IPCStream everywhere - part 3 - PUDPSocket, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e730e60e2db
Use IPCStream everywhere - part 4 - PWebSocket, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f983a51496e0
Use IPCStream everywhere - part 5 - PContent, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/94e2abd5299a
Use IPCStream everywhere - part 6 - fixing AutoIPCStream when inputStream is null, r=smaug

Comment 26

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/a4bcf2c7d260
https://hg.mozilla.org/mozilla-central/rev/df17e65b3785
https://hg.mozilla.org/mozilla-central/rev/0cdf685ace68
https://hg.mozilla.org/mozilla-central/rev/4e730e60e2db
https://hg.mozilla.org/mozilla-central/rev/f983a51496e0
https://hg.mozilla.org/mozilla-central/rev/94e2abd5299a

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Still seeing recent reports on trunk :(
https://crash-stats.mozilla.com/report/index/f513d6ce-f009-41ff-b8ee-a99bd2170316

Comment 28

[Andrea Marchesini [:baku]]

Yes, this can happen: I moved all the inputStream serialization to IPCStream except for PBlob.
PBlob requires a big refactoring and I have done half of it. Still working on it.
Can we have a new bug just for this PBlob+InputStream crash?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Filed bug 1347904. What can/should we do in this bug with respect to branch backports?

Comment 30

[Andrea Marchesini [:baku]]

Comment on attachment 8843285 [details] [diff] [review]
part 1 - PWebBrowserPersistDocument

Approval Request Comment
[Feature/Bug causing the regression]: e10s and nsIInputStream
[User impact if declined]: if the content of the nsIInputStream is too big, we could make the content process to crash.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no more intermittent failures
[Needs manual test from QE? If yes, steps to reproduce]: none
[List of other uplifts needed for the feature/fix]: Bug 1274343
[Is the change risky?]: low
[Why is the change risky/not risky?]: These patches use an IPCStream: instead crashing we create a PChildToParentStream protocol. The risk here is that the deserialized nsIInputStream doesn't implement seek/cloneable and other methods. This is currently fine in our codebase but it could be a problem in the future. 
[String changes made/needed]: none

Comment 31

[Gerry Chang [:gchang]]

Comment on attachment 8843285 [details] [diff] [review]
part 1 - PWebBrowserPersistDocument

After discussing with :baku, we decided to let it ride the train and bake more. Aurora54-.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Doesn't seem likely we're ever going to take this on ESR52 either, though that's a bit disappointing since we're moving a lot of OOM-prone users over to that branch with the deprecation of WinXP.