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)
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: kanru, Assigned: baku)
Details
(Keywords: crash)
Crash Data
Attachments
(7 files, 2 obsolete files)
7.36 KB,
patch
|
smaug
:
review+
gchang
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
8.07 KB,
patch
|
smaug
:
review+
gchang
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
5.92 KB,
patch
|
smaug
:
review+
gchang
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
7.33 KB,
patch
|
smaug
:
review+
gchang
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
13.36 KB,
patch
|
smaug
:
review+
gchang
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
9.30 KB,
patch
|
smaug
:
review+
gchang
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
3.69 KB,
patch
|
smaug
:
review+
gchang
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
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•8 years ago
|
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 1•8 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•8 years ago
|
||
Attachment #8843286 -
Flags: review?(bugs)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8843287 -
Flags: review?(bugs)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8843288 -
Flags: review?(bugs)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8843289 -
Flags: review?(bugs)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8843291 -
Flags: review?(bugs)
Assignee | ||
Comment 7•8 years ago
|
||
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•8 years ago
|
||
smaug, can you take this patch?
Attachment #8843288 -
Attachment is obsolete: true
Flags: needinfo?(bugs)
Assignee | ||
Comment 16•8 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•8 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•8 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•8 years ago
|
||
Attachment #8845004 -
Attachment is obsolete: true
Attachment #8845283 -
Flags: review?(bugs)
Attachment #8845283 -
Flags: review?(bugs) → review+
Comment 25•8 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
Comment 26•8 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•8 years ago
|
status-firefox52:
--- → wontfix
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox-esr52:
--- → affected
Comment 27•8 years ago
|
||
Still seeing recent reports on trunk :(
https://crash-stats.mozilla.com/report/index/f513d6ce-f009-41ff-b8ee-a99bd2170316
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 28•8 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)
Comment 29•8 years ago
|
||
Filed bug 1347904. What can/should we do in this bug with respect to branch backports?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 30•8 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•8 years ago
|
Attachment #8843286 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8843287 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8843289 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8843291 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8844788 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8845283 -
Flags: approval-mozilla-aurora?
Comment 31•8 years ago
|
||
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-
Updated•8 years ago
|
Attachment #8843286 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•8 years ago
|
Attachment #8843287 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•8 years ago
|
Attachment #8843289 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•8 years ago
|
Attachment #8843291 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•8 years ago
|
Attachment #8844788 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•8 years ago
|
Attachment #8845283 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•8 years ago
|
Comment 32•8 years ago
|
||
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.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•