Closed Bug 1339713 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

52 Branch
Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: kanru, Assigned: baku)

Details

(Keywords: crash)

Crash Data

Attachments

(7 files, 2 obsolete files)

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: nobody → amarchesini
Flags: needinfo?(amarchesini)
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)
Attachment #8843286 - Flags: review?(bugs)
Attachment #8843287 - Flags: review?(bugs)
Attached patch part 4 - PWebSocket (obsolete) — Splinter Review
Attachment #8843288 - Flags: review?(bugs)
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-
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+
smaug, can you take this patch?
Attachment #8843288 - Attachment is obsolete: true
Flags: needinfo?(bugs)
> 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.
(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).
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-
Attachment #8845004 - Attachment is obsolete: true
Attachment #8845283 - Flags: review?(bugs)
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
Flags: needinfo?(amarchesini)
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)
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?
Attachment #8843286 - Flags: approval-mozilla-aurora?
Attachment #8843287 - Flags: approval-mozilla-aurora?
Attachment #8843289 - Flags: approval-mozilla-aurora?
Attachment #8843291 - Flags: approval-mozilla-aurora?
Attachment #8844788 - Flags: approval-mozilla-aurora?
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: