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

RESOLVED FIXED in Firefox 55

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: kanru, Assigned: baku)

Tracking

({crash})

52 Branch
mozilla55
Unspecified
Windows 10
Points:
---

Firefox Tracking Flags

(firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

Details

(crash signature)

Attachments

(7 attachments, 2 obsolete attachments)

Reporter

Description

2 years ago
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.
Flags: needinfo?(amarchesini)
Assignee

Updated

2 years ago
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Assignee

Comment 1

2 years ago
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.
Attachment #8843285 - Flags: review?(bugs)
Assignee

Comment 2

2 years ago
Attachment #8843286 - Flags: review?(bugs)
Assignee

Comment 3

2 years ago
Attachment #8843287 - Flags: review?(bugs)
Assignee

Comment 4

2 years ago
Posted patch part 4 - PWebSocket (obsolete) — Splinter Review
Attachment #8843288 - Flags: review?(bugs)
Assignee

Comment 5

2 years ago
Attachment #8843289 - Flags: review?(bugs)
So this relies on some undocumented side effect of AutoIPCStream?
I'm trying to understand what it means to not pass FileDescriptors around.
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 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).
Attachment #8843288 - Flags: review?(bugs) → review-
Attachment #8843289 - Flags: review?(bugs) → review+
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.
Attachment #8843291 - Flags: review?(bugs) → review+
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.
Attachment #8843287 - Flags: review?(bugs) → review+
Comment on attachment 8843285 [details] [diff] [review]
part 1 - PWebBrowserPersistDocument

So, remove the useless postData and postFiles variables.
Attachment #8843285 - Flags: review?(bugs) → review+
Attachment #8843286 - Flags: review?(bugs) → review+
Assignee

Comment 15

2 years ago
smaug, can you take this patch?
Attachment #8843288 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Assignee

Comment 16

2 years ago
> 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.
Then hide the method somehow. Make it a protected method of some class and make the accepted callers friend classes or something?
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.
Attachment #8844788 - Flags: review+
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.
Flags: needinfo?(bugs)
er, this are about child to parent, so shouldn't affect to that.
Assignee

Comment 21

2 years ago
(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).
Assignee

Comment 22

2 years ago
This bit was in the PChildToParentStream patches. It's needed for landing this.
Flags: needinfo?(bugs)
Comment on attachment 8845004 [details] [diff] [review]
part 5 - fixing AutoIPCStream when inputStream is null

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># 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
Flags: needinfo?(bugs)
Attachment #8845004 - Flags: review-
Assignee

Comment 24

2 years ago
Attachment #8845004 - Attachment is obsolete: true
Attachment #8845283 - Flags: review?(bugs)
Attachment #8845283 - Flags: review?(bugs) → review+

Comment 25

2 years ago
Pushed by amarchesini@mozilla.com:
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
Still seeing recent reports on trunk :(
https://crash-stats.mozilla.com/report/index/f513d6ce-f009-41ff-b8ee-a99bd2170316
Flags: needinfo?(amarchesini)
Assignee

Comment 28

2 years ago
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?
Flags: needinfo?(amarchesini)
Filed bug 1347904. What can/should we do in this bug with respect to branch backports?
Flags: needinfo?(amarchesini)
Assignee

Comment 30

2 years ago
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
Flags: needinfo?(amarchesini)
Attachment #8843285 - Flags: approval-mozilla-aurora?
Assignee

Updated

2 years ago
Attachment #8843286 - Flags: approval-mozilla-aurora?
Assignee

Updated

2 years ago
Attachment #8843287 - Flags: approval-mozilla-aurora?
Assignee

Updated

2 years ago
Attachment #8843289 - Flags: approval-mozilla-aurora?
Assignee

Updated

2 years ago
Attachment #8843291 - Flags: approval-mozilla-aurora?
Assignee

Updated

2 years ago
Attachment #8844788 - Flags: approval-mozilla-aurora?
Assignee

Updated

2 years ago
Attachment #8845283 - Flags: approval-mozilla-aurora?
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-.
Attachment #8843285 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8843286 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8843287 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8843289 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8843291 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8844788 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8845283 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.