1274075 - IndexedDB ObjectStore AddOrPut operation crashes when attempting to send too much data via an IPC pipe

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect, P1)

Description

[Jukka Jylänki]

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1271102#c9, reporting as a separate entry from bug 1271102.

STR:

Visit https://s3.amazonaws.com/mozilla-games/tmp/2016-05-05-PlatformerGame-profiling/PlatformerGame-HTML5-Shipping.html?playback&cpuprofiler&webglprofiler&expandhelp&tracegl=30

This will crash in call stack

mozilla::ipc::ProcessLink::SendMessageW
mozilla::ipc::MessageChannel::Send
mozilla::dom::indexedDB::PBackgroundIDBTransactionChild::SendPBackgroundIDBRequestConstructor
mozilla::dom::IDBTransaction::StartRequest
mozilla::dom::IDBObjectStore::AddOrPut
mozilla::dom::IDBObjectStoreBinding::put
js::InternalCallOrConstruct
Interpret
js::RunScript

What the page does is it attempts to XHR in a binary blob of 96.6MB, and store it to IndexedDB as an ArrayBuffer in one store operation.

Some crash reports:

https://crash-stats.mozilla.com/report/index/e83cf8d1-e3e5-4a3d-a24d-0c3ed2160518
https://crash-stats.mozilla.com/report/index/3f4aaf3c-70e4-4559-bfc8-abe8d2160518

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This could possibly use the new code bug 1093357.

Comment 2

[Emil Pasca [:emilpasca]]

I have tested this issue on the latest Firefox release (46.0.1, Build ID: 20160502172042) and on the latest Nightly(49.0a1, Build ID: 20160518030234) and managed to reproduce it, but only on the Nightly version with e10s enabled. I've visited the page provided in comment 0 and Nightly crashes.

Comment 3

[Ting-Yu Chou [:ting] (away)]

I'll see what I can do with this.

Comment 4

[Ting-Yu Chou [:ting] (away)]

The cloned value goes to IPC::ParamTraits<nsTArray<unsigned char>>::Write for sending the cloneInfo_ of ObjectStoreAddPutParams. I am not sure how to apply IPCStream here, wrap the nsTArray by an input stream?

Comment 5

[Ting-Yu Chou [:ting] (away)]

I'll read TypeUtils::ToCacheRequest() and other dom cache API code using IPCStream.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Hmm.  Ideally we would have some kind of string stream that adopted the buffer without copying, but not sure we do.  You might have to copy with our current stream primitives.

Comment 7

[Ting-Yu Chou [:ting] (away)]

Not only SerializedStructuredCloneWriteInfo but also SerializedStructuredCloneReadInfo has a nsTArray<uint8_t>, we will need bug 1274343 for it.

Comment 8

[Andrew McCreight [:mccr8]]

Bug 1264642 and bug 1262671 will help prevent OOM crashes when the message size is large, but it won't keep messages from being that large in the first place, which is what this assertion is.

Comment 9

[Ting-Yu Chou [:ting] (away)]

Just Kanru's patch in bug 1264642 is going to change the StructuredCloneWriteInfo serialization, which may conflict the work here.

Comment 10

[Ting-Yu Chou [:ting] (away)]

(In reply to Ben Kelly [:bkelly] from comment #6)
> Hmm.  Ideally we would have some kind of string stream that adopted the
> buffer without copying, but not sure we do.  You might have to copy with our
> current stream primitives.

NS_NewByteInputStream() can adopt a buffer, but nsStringInputStream is serializable which won't go PSendStream. Should I new a subclass of nsIAsyncInputStream?

Comment 11

[Ting-Yu Chou [:ting] (away)]

Attachment: wip v1

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ting-Yu Chou [:ting] from comment #10)
> NS_NewByteInputStream() can adopt a buffer, but nsStringInputStream is
> serializable which won't go PSendStream. Should I new a subclass of
> nsIAsyncInputStream?

I think we should probably make the AutoIPCStream::Serialize() method:

1) check the length of a serializable stream
2) if its greater than some threshold and non-blocking/async, then use PSendStream
3) otherwise use normal SerializeInputStream()
4) still always try to fall back to PSendStream for things that are not directly serializable

I'm not sure if the string streams implement nsIAsyncInputStream, but they could in theory.  They are definitely non-blocking.

Comment 13

[Ting-Yu Chou [:ting] (away)]

Attachment: wip v2 ipcstream

It seems when the data is large (~180MB here), it could run to ObjectStoreAddOrPutRequestOp::DoDatabaseWork() when PSendStream hasn't received all the buffers, will try AsyncWait() on the reader.

Comment 14

[Ting-Yu Chou [:ting] (away)]

I found it's not easy to switch to AsyncWait() in DoDatabaseWork(), do you have any other suggestions for comment 13?

Comment 15

[Ting-Yu Chou [:ting] (away)]

Probably I should just put the AutoIPCStream in a scope prior and other than sending the IDB request.

Comment 16

[Ting-Yu Chou [:ting] (away)]

With the URL from STR, even though AddOrPut is fixed, parent still runs into the same 128MB limit when NormalTransactionOp::SendSuccessResult().

Comment 17

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #16)
> With the URL from STR, even though AddOrPut is fixed, parent still runs into
> the same 128MB limit when NormalTransactionOp::SendSuccessResult().

The type of response is TObjectStoreGetResponse, and the size matches to the data AddOrPut() sent. It seems the site get the data after it's been added.

Comment 18

[Ting-Yu Chou [:ting] (away)]

The stack:

#0  0x00007fffe619e0d6 in mozilla::ipc::ProcessLink::SendMessage (
    this=0x7fffc372abb0, msg=0x7fffc56c4040)
    at /home/ting/w/fx/mc/ipc/glue/MessageLink.cpp:161
#1  0x00007fffe6197d60 in mozilla::ipc::MessageChannel::Send (
    this=0x7fffc05b4068, aMsg=0x7fffc56c4040)
    at /home/ting/w/fx/mc/ipc/glue/MessageChannel.cpp:780
#2  0x00007fffe647ec05 in mozilla::dom::indexedDB::PBackgroundIDBRequestParent::
Send__delete__ (actor=0x7fffd46f3e68, response=...)
    at /home/ting/w/fx/mc/obj-x86_64-pc-linux-gnu/ipc/ipdl/PBackgroundIDBRequest
Parent.cpp:65
#3  0x00007fffe8ca3915 in mozilla::dom::indexedDB::(anonymous namespace)::Normal
TransactionOp::SendSuccessResult (this=0x7fffd46f3e00)
    at /home/ting/w/fx/mc/dom/indexedDB/ActorsParent.cpp:24922
#4  0x00007fffe8c98672 in mozilla::dom::indexedDB::(anonymous namespace)::Transa
ctionDatabaseOperationBase::RunOnOwningThread (this=0x7fffd46f3e00)
    at /home/ting/w/fx/mc/dom/indexedDB/ActorsParent.cpp:22733
#5  0x00007fffe8c9887c in mozilla::dom::indexedDB::(anonymous namespace)::Transa
ctionDatabaseOperationBase::Run (this=0x7fffd46f3e00)
    at /home/ting/w/fx/mc/dom/indexedDB/ActorsParent.cpp:22776
#6  0x00007fffe59d5ca1 in nsThread::ProcessNextEvent (this=0x7fffccda9d00, 
    aMayWait=true, aResult=0x7fffcccfed0f)
    at /home/ting/w/fx/mc/xpcom/threads/nsThread.cpp:1025

Comment 19

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #15)
> Probably I should just put the AutoIPCStream in a scope prior and other than
> sending the IDB request.

  {
    UniquePtr<AutoIPCStream> autoStream(new AutoIPCStream(...));
    autoStream->Serialize(stream, aManager);
    autoStream->TakeValue();
  }

  SendTheIPC();

The pseudo code is like this, all the stream's data is sent when |autoStream| gets destructed. However it does not work, the actor of PSendStream has been destroyed in parent when parent receives the IPC, because SendStreamChildImpl::OnEnd() gets called in:

https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/ipc/glue/SendStreamChild.cpp#261

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Yea, that's not going to work.  The receiving side must be able to deal with async data so that it can use an NS_AsyncCopy().  You can then trigger the DoDatabaseWork when the NS_AsyncCopy() is complete.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ting-Yu Chou [:ting] from comment #19)
>   {
>     UniquePtr<AutoIPCStream> autoStream(new AutoIPCStream(...));
>     autoStream->Serialize(stream, aManager);
>     autoStream->TakeValue();
>   }
> 
>   SendTheIPC();

Also, I'm sorry, but I didn't understand this is what you meant in your earlier comment.  This is definitely not supported.  (Also, you could just do AutoIPCStream autoStream(...) instead of using UniquePtr<>).

Comment 22

[Ting-Yu Chou [:ting] (away)]

I decided to separate Get OP from this bug because it depends on bug 1274343.

Comment 23

[Ting-Yu Chou [:ting] (away)]

Shmem is another option for resolving this issue, and probably more straightforward.

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 8757851 [details] [diff] [review]
wip v2 ipcstream

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

::: xpcom/io/nsStringStream.cpp
@@ +286,5 @@
> +    nsCOMPtr<nsIInputStreamCallback> callback;
> +    callback = NS_NewInputStreamReadyEvent(aCallback, aTarget);
> +    callback->OnInputStreamReady(this);
> +  } else {
> +    aCallback->OnInputStreamReady(this);

nit: I don't think you can call this callback directly, you will need to repost to the current thread.

Comment 25

[Ting-Yu Chou [:ting] (away)]

Attachment: wip v3 shmem

Use shmem for sending the data of SerializedStructuredCloneWriteInfo.

Comment 26

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8757851 [details] [diff] [review]
wip v2 ipcstream

># HG changeset patch
># User Ting-Yu Chou <[e-mail]>
># Date 1464604395 -28800
>#      Mon May 30 18:33:15 2016 +0800
># Node ID 7b33fd22cc06ec7005bc15c4a2c129ab0bbac78c
># Parent  5511d54a3f172c1d68f98cc55dce4de1d0ba1b51
>[mq]: bug-1274075
>
>diff --git a/dom/indexedDB/ActorsParent.cpp b/dom/indexedDB/ActorsParent.cpp
>--- a/dom/indexedDB/ActorsParent.cpp
>+++ b/dom/indexedDB/ActorsParent.cpp
>@@ -51,16 +51,17 @@
> #include "mozilla/dom/quota/FileStreams.h"
> #include "mozilla/dom/quota/OriginScope.h"
> #include "mozilla/dom/quota/QuotaManager.h"
> #include "mozilla/dom/quota/UsageInfo.h"
> #include "mozilla/ipc/BackgroundParent.h"
> #include "mozilla/ipc/BackgroundUtils.h"
> #include "mozilla/ipc/InputStreamParams.h"
> #include "mozilla/ipc/InputStreamUtils.h"
>+#include "mozilla/ipc/IPCStreamUtils.h"
> #include "mozilla/ipc/PBackground.h"
> #include "mozilla/storage/Variant.h"
> #include "nsCharSeparatedTokenizer.h"
> #include "nsClassHashtable.h"
> #include "nsCOMPtr.h"
> #include "nsDataHashtable.h"
> #include "nsEscape.h"
> #include "nsHashKeys.h"
>@@ -14704,38 +14705,38 @@ TransactionBase::VerifyRequestParams(con
> 
>   RefPtr<FullObjectStoreMetadata> objMetadata =
>     GetMetadataForObjectStoreId(aParams.objectStoreId());
>   if (NS_WARN_IF(!objMetadata)) {
>     ASSERT_UNLESS_FUZZING();
>     return false;
>   }
> 
>-  if (NS_WARN_IF(aParams.cloneInfo().data().IsEmpty())) {
>+  if (NS_WARN_IF(!aParams.cloneInfo().length())) {
>     ASSERT_UNLESS_FUZZING();
>     return false;
>   }
> 
>   if (objMetadata->mCommonMetadata.autoIncrement() &&
>       objMetadata->mCommonMetadata.keyPath().IsValid() &&
>       aParams.key().IsUnset()) {
>     const SerializedStructuredCloneWriteInfo cloneInfo = aParams.cloneInfo();
> 
>     if (NS_WARN_IF(!cloneInfo.offsetToKeyProp())) {
>       ASSERT_UNLESS_FUZZING();
>       return false;
>     }
> 
>-    if (NS_WARN_IF(cloneInfo.data().Length() < sizeof(uint64_t))) {
>+    if (NS_WARN_IF(cloneInfo.length() < sizeof(uint64_t))) {
>       ASSERT_UNLESS_FUZZING();
>       return false;
>     }
> 
>     if (NS_WARN_IF(cloneInfo.offsetToKeyProp() >
>-                   (cloneInfo.data().Length() - sizeof(uint64_t)))) {
>+                   (cloneInfo.length() - sizeof(uint64_t)))) {
>       ASSERT_UNLESS_FUZZING();
>       return false;
>     }
>   } else if (NS_WARN_IF(aParams.cloneInfo().offsetToKeyProp())) {
>     ASSERT_UNLESS_FUZZING();
>     return false;
>   }
> 
>@@ -25291,16 +25292,27 @@ ObjectStoreAddOrPutRequestOp::DoDatabase
>     return rv;
>   }
> 
>   rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("osid"), osid);
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     return rv;
>   }
> 
>+  const SerializedStructuredCloneWriteInfo& cloneInfo = mParams.cloneInfo();
>+  auto* cloneData = reinterpret_cast<char*>(malloc(cloneInfo.length()));
>+  if (NS_WARN_IF(!cloneData)) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  } else {
>+    nsCOMPtr<nsIInputStream> stream = DeserializeIPCStream(cloneInfo.data());
>+    uint32_t bytesRead;
>+    stream->Read(cloneData, cloneInfo.length(), &bytesRead);
>+    MOZ_ASSERT(bytesRead == cloneInfo.length());
>+  }
>+
>   MOZ_ASSERT(!keyUnset || mMetadata->mCommonMetadata.autoIncrement(),
>              "Should have key unless autoIncrement");
> 
>   int64_t autoIncrementNum = 0;
> 
>   if (mMetadata->mCommonMetadata.autoIncrement()) {
>     if (keyUnset) {
>       autoIncrementNum = mMetadata->mNextAutoIncrementId;
>@@ -25313,41 +25325,38 @@ ObjectStoreAddOrPutRequestOp::DoDatabase
> 
>       key.SetFromInteger(autoIncrementNum);
>     } else if (key.IsFloat() &&
>                key.ToFloat() >= mMetadata->mNextAutoIncrementId) {
>       autoIncrementNum = floor(key.ToFloat());
>     }
> 
>     if (keyUnset && keyPath.IsValid()) {
>-      const SerializedStructuredCloneWriteInfo& cloneInfo = mParams.cloneInfo();
>       MOZ_ASSERT(cloneInfo.offsetToKeyProp());
>-      MOZ_ASSERT(cloneInfo.data().Length() > sizeof(uint64_t));
>+      MOZ_ASSERT(cloneInfo.length() > sizeof(uint64_t));
>       MOZ_ASSERT(cloneInfo.offsetToKeyProp() <=
>-                 (cloneInfo.data().Length() - sizeof(uint64_t)));
>+                 (cloneInfo.length() - sizeof(uint64_t)));
> 
>       // Special case where someone put an object into an autoIncrement'ing
>       // objectStore with no key in its keyPath set. We needed to figure out
>       // which row id we would get above before we could set that properly.
>       uint8_t* keyPropPointer =
>-        const_cast<uint8_t*>(cloneInfo.data().Elements() +
>-                             cloneInfo.offsetToKeyProp());
>+        reinterpret_cast<uint8_t*>(cloneData + cloneInfo.offsetToKeyProp());
>       uint64_t keyPropValue =
>         ReinterpretDoubleAsUInt64(static_cast<double>(autoIncrementNum));
> 
>       LittleEndian::writeUint64(keyPropPointer, keyPropValue);
>     }
>   }
> 
>   key.BindToStatement(stmt, NS_LITERAL_CSTRING("key"));
> 
>   // Compress the bytes before adding into the database.
>-  const char* uncompressed =
>-    reinterpret_cast<const char*>(mParams.cloneInfo().data().Elements());
>-  size_t uncompressedLength = mParams.cloneInfo().data().Length();
>+  const char* uncompressed = reinterpret_cast<const char*>(cloneData);
>+  size_t uncompressedLength = cloneInfo.length();
> 
>   // We don't have a smart pointer class that calls free, so we need to
>   // manage | compressed | manually.
>   {
>     size_t compressedLength = snappy::MaxCompressedLength(uncompressedLength);
> 
>     char* compressed = static_cast<char*>(malloc(compressedLength));
>     if (NS_WARN_IF(!compressed)) {
>diff --git a/dom/indexedDB/IDBObjectStore.cpp b/dom/indexedDB/IDBObjectStore.cpp
>--- a/dom/indexedDB/IDBObjectStore.cpp
>+++ b/dom/indexedDB/IDBObjectStore.cpp
>@@ -36,19 +36,21 @@
> #include "mozilla/dom/IDBObjectStoreBinding.h"
> #include "mozilla/dom/StructuredCloneHolder.h"
> #include "mozilla/dom/StructuredCloneTags.h"
> #include "mozilla/dom/indexedDB/PBackgroundIDBSharedTypes.h"
> #include "mozilla/dom/ipc/BlobChild.h"
> #include "mozilla/dom/ipc/BlobParent.h"
> #include "mozilla/dom/ipc/nsIRemoteBlob.h"
> #include "mozilla/ipc/BackgroundChild.h"
>+#include "mozilla/ipc/IPCStreamUtils.h"
> #include "mozilla/ipc/PBackgroundSharedTypes.h"
> #include "nsCOMPtr.h"
> #include "nsQueryObject.h"
>+#include "nsStringStream.h"
> #include "ProfilerHelpers.h"
> #include "ReportInternalError.h"
> #include "WorkerPrivate.h"
> #include "WorkerScope.h"
> 
> // Include this last to avoid path problems on Windows.
> #include "ActorsChild.h"
> 
>@@ -114,25 +116,25 @@ struct IDBObjectStore::StructuredCloneWr
>            this->mBlobOrMutableFiles == aOther.mBlobOrMutableFiles &&
>            this->mDatabase == aOther.mDatabase &&
>            this->mOffsetToKeyProp == aOther.mOffsetToKeyProp;
>   }
> 
>   bool
>   SetFromSerialized(const SerializedStructuredCloneWriteInfo& aOther)
>   {
>-    if (aOther.data().IsEmpty()) {
>+    if (!aOther.length()) {
>       mCloneBuffer.clear();
>     } else {
>-      auto* aOtherBuffer =
>-        reinterpret_cast<uint64_t*>(
>-          const_cast<uint8_t*>(aOther.data().Elements()));
>-      if (!mCloneBuffer.copy(aOtherBuffer, aOther.data().Length())) {
>-        return false;
>-      }
>+      auto* buf = static_cast<char*>(js_malloc(aOther.length()));
>+      nsCOMPtr<nsIInputStream> stream = DeserializeIPCStream(aOther.data());
>+      uint32_t bytesRead;
>+      stream->Read(buf, aOther.length(), &bytesRead);
>+      MOZ_ASSERT(bytesRead == aOther.length());
>+      mCloneBuffer.adopt(reinterpret_cast<uint64_t*>(buf), aOther.length());
>     }
> 
>     mBlobOrMutableFiles.Clear();
> 
>     mOffsetToKeyProp = aOther.offsetToKeyProp();
>     return true;
>   }
> };
>@@ -1316,36 +1318,33 @@ IDBObjectStore::AddOrPut(JSContext* aCx,
>   StructuredCloneWriteInfo cloneWriteInfo(mTransaction->Database());
>   nsTArray<IndexUpdateInfo> updateInfo;
> 
>   aRv = GetAddInfo(aCx, value, aKey, cloneWriteInfo, key, updateInfo);
>   if (aRv.Failed()) {
>     return nullptr;
>   }
> 
>-  FallibleTArray<uint8_t> cloneData;
>-  if (NS_WARN_IF(!cloneData.SetLength(cloneWriteInfo.mCloneBuffer.nbytes(),
>-                                      fallible))) {
>-    aRv = NS_ERROR_OUT_OF_MEMORY;
>-    return nullptr;
>-  }
>-
>-  // XXX Remove this
>-  memcpy(cloneData.Elements(), cloneWriteInfo.mCloneBuffer.data(),
>-         cloneWriteInfo.mCloneBuffer.nbytes());
>-
>-  cloneWriteInfo.mCloneBuffer.clear();
>+  nsCOMPtr<nsIInputStream> cloneStream;
>+  NS_NewByteInputStream(getter_AddRefs(cloneStream),
>+                        reinterpret_cast<char*>(cloneWriteInfo.mCloneBuffer.data()),
>+                        cloneWriteInfo.mCloneBuffer.nbytes(),
>+                        NS_ASSIGNMENT_DEPEND);
> 
>   ObjectStoreAddPutParams commonParams;
>   commonParams.objectStoreId() = Id();
>-  commonParams.cloneInfo().data().SwapElements(cloneData);
>+  commonParams.cloneInfo().length() = cloneWriteInfo.mCloneBuffer.nbytes();
>   commonParams.cloneInfo().offsetToKeyProp() = cloneWriteInfo.mOffsetToKeyProp;
>   commonParams.key() = key;
>   commonParams.indexUpdateInfos().SwapElements(updateInfo);
> 
>+  UniquePtr<AutoIPCStream> autoStream(new AutoIPCStream(commonParams.cloneInfo().data()));
>+  autoStream->Serialize(cloneStream, mTransaction->GetBackgroundManager());
>+  autoStream->TakeValue();
>+
>   // Convert any blobs or mutable files into DatabaseOrMutableFile.
>   nsTArray<StructuredCloneWriteInfo::BlobOrMutableFile>& blobOrMutableFiles =
>     cloneWriteInfo.mBlobOrMutableFiles;
> 
>   if (!blobOrMutableFiles.IsEmpty()) {
>     const uint32_t count = blobOrMutableFiles.Length();
> 
>     FallibleTArray<DatabaseOrMutableFile> fileOrMutableFileActors;
>diff --git a/dom/indexedDB/IDBTransaction.cpp b/dom/indexedDB/IDBTransaction.cpp
>--- a/dom/indexedDB/IDBTransaction.cpp
>+++ b/dom/indexedDB/IDBTransaction.cpp
>@@ -282,16 +282,24 @@ IDBTransaction::SetBackgroundActor(index
>   AssertIsOnOwningThread();
>   MOZ_ASSERT(aBackgroundActor);
>   MOZ_ASSERT(!mBackgroundActor.mNormalBackgroundActor);
>   MOZ_ASSERT(mMode != VERSION_CHANGE);
> 
>   mBackgroundActor.mNormalBackgroundActor = aBackgroundActor;
> }
> 
>+PBackgroundChild*
>+IDBTransaction::GetBackgroundManager()
>+{
>+  return mMode == VERSION_CHANGE ?
>+    mBackgroundActor.mVersionChangeBackgroundActor->Manager()->Manager()->Manager() :
>+    mBackgroundActor.mNormalBackgroundActor->Manager()->Manager()->Manager();
>+}
>+
> BackgroundRequestChild*
> IDBTransaction::StartRequest(IDBRequest* aRequest, const RequestParams& aParams)
> {
>   AssertIsOnOwningThread();
>   MOZ_ASSERT(aRequest);
>   MOZ_ASSERT(aParams.type() != RequestParams::T__None);
> 
>   BackgroundRequestChild* actor = new BackgroundRequestChild(aRequest);
>diff --git a/dom/indexedDB/IDBTransaction.h b/dom/indexedDB/IDBTransaction.h
>--- a/dom/indexedDB/IDBTransaction.h
>+++ b/dom/indexedDB/IDBTransaction.h
>@@ -5,16 +5,17 @@
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #ifndef mozilla_dom_idbtransaction_h__
> #define mozilla_dom_idbtransaction_h__
> 
> #include "mozilla/Attributes.h"
> #include "mozilla/dom/IDBTransactionBinding.h"
> #include "mozilla/dom/IDBWrapperCache.h"
>+#include "mozilla/ipc/BackgroundChild.h"
> #include "nsAutoPtr.h"
> #include "nsCycleCollectionParticipant.h"
> #include "nsIRunnable.h"
> #include "nsString.h"
> #include "nsTArray.h"
> 
> class nsPIDOMWindowInner;
> 
>@@ -38,16 +39,18 @@ class BackgroundRequestChild;
> class BackgroundTransactionChild;
> class BackgroundVersionChangeTransactionChild;
> class IndexMetadata;
> class ObjectStoreSpec;
> class OpenCursorParams;
> class RequestParams;
> }
> 
>+using mozilla::ipc::PBackgroundChild;
>+
> class IDBTransaction final
>   : public IDBWrapperCache
>   , public nsIRunnable
> {
>   friend class indexedDB::BackgroundCursorChild;
>   friend class indexedDB::BackgroundRequestChild;
> 
>   class WorkerFeature;
>@@ -149,16 +152,19 @@ public:
> 
>     if (mMode == VERSION_CHANGE) {
>       mBackgroundActor.mVersionChangeBackgroundActor = nullptr;
>     } else {
>       mBackgroundActor.mNormalBackgroundActor = nullptr;
>     }
>   }
> 
>+  PBackgroundChild*
>+  GetBackgroundManager();
>+
>   indexedDB::BackgroundRequestChild*
>   StartRequest(IDBRequest* aRequest, const indexedDB::RequestParams& aParams);
> 
>   void
>   OpenCursor(indexedDB::BackgroundCursorChild* aBackgroundActor,
>              const indexedDB::OpenCursorParams& aParams);
> 
>   void
>diff --git a/dom/indexedDB/PBackgroundIDBSharedTypes.ipdlh b/dom/indexedDB/PBackgroundIDBSharedTypes.ipdlh
>--- a/dom/indexedDB/PBackgroundIDBSharedTypes.ipdlh
>+++ b/dom/indexedDB/PBackgroundIDBSharedTypes.ipdlh
>@@ -1,17 +1,19 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> include protocol PBackgroundIDBDatabaseFile;
> include protocol PBackgroundMutableFile;
> include protocol PBlob;
>+include protocol PSendStream;
> 
> include DOMTypes;
>+include IPCStream;
> 
> include "mozilla/dom/indexedDB/SerializationHelpers.h";
> include "mozilla/dom/quota/SerializationHelpers.h";
> 
> using struct mozilla::null_t
>   from "ipc/IPCMessageUtils.h";
> 
> using struct mozilla::void_t
>@@ -57,17 +59,18 @@ union BlobOrMutableFile
> struct SerializedStructuredCloneReadInfo
> {
>   uint8_t[] data;
>   BlobOrMutableFile[] blobs;
> };
> 
> struct SerializedStructuredCloneWriteInfo
> {
>-  uint8_t[] data;
>+  IPCStream data;
>+  size_t length;
>   uint64_t offsetToKeyProp;
> };
> 
> struct IndexUpdateInfo
> {
>   int64_t indexId;
>   Key value;
>   Key localizedValue;
>diff --git a/dom/indexedDB/PBackgroundIDBTransaction.ipdl b/dom/indexedDB/PBackgroundIDBTransaction.ipdl
>--- a/dom/indexedDB/PBackgroundIDBTransaction.ipdl
>+++ b/dom/indexedDB/PBackgroundIDBTransaction.ipdl
>@@ -2,16 +2,18 @@
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> include protocol PBackgroundIDBCursor;
> include protocol PBackgroundIDBDatabase;
> include protocol PBackgroundIDBDatabaseFile;
> include protocol PBackgroundIDBRequest;
> include protocol PBackgroundMutableFile;
>+include protocol PFileDescriptorSet;
>+include protocol PSendStream;
> 
> include PBackgroundIDBSharedTypes;
> 
> namespace mozilla {
> namespace dom {
> namespace indexedDB {
> 
> protocol PBackgroundIDBTransaction
>diff --git a/dom/indexedDB/PBackgroundIDBVersionChangeTransaction.ipdl b/dom/indexedDB/PBackgroundIDBVersionChangeTransaction.ipdl
>--- a/dom/indexedDB/PBackgroundIDBVersionChangeTransaction.ipdl
>+++ b/dom/indexedDB/PBackgroundIDBVersionChangeTransaction.ipdl
>@@ -3,16 +3,18 @@
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> include protocol PBackgroundIDBCursor;
> include protocol PBackgroundIDBDatabase;
> include protocol PBackgroundIDBDatabaseFile;
> include protocol PBackgroundIDBRequest;
> include protocol PBackgroundMutableFile;
> include protocol PBlob;
>+include protocol PFileDescriptorSet;
>+include protocol PSendStream;
> 
> include PBackgroundIDBSharedTypes;
> 
> namespace mozilla {
> namespace dom {
> namespace indexedDB {
> 
> protocol PBackgroundIDBVersionChangeTransaction
>diff --git a/ipc/glue/IPCStreamUtils.cpp b/ipc/glue/IPCStreamUtils.cpp
>--- a/ipc/glue/IPCStreamUtils.cpp
>+++ b/ipc/glue/IPCStreamUtils.cpp
>@@ -115,18 +115,25 @@ SerializeInputStream(nsIInputStream* aSt
> {
>   MOZ_ASSERT(aStream);
>   MOZ_ASSERT(aManager);
> 
>   // First attempt simple stream serialization
>   nsCOMPtr<nsIIPCSerializableInputStream> serializable =
>     do_QueryInterface(aStream);
>   if (serializable) {
>-    SerializeInputStreamWithFdsChild(aStream, aValue, aManager);
>-    return;
>+    static const uint64_t kMaxBytesUsingFd = 1024 * 1024;
>+    bool nonBlocking;
>+    uint64_t available;
>+    MOZ_ALWAYS_SUCCEEDS(aStream->IsNonBlocking(&nonBlocking));
>+    MOZ_ALWAYS_SUCCEEDS(aStream->Available(&available));
>+    if (!nonBlocking || available < kMaxBytesUsingFd) {
>+      SerializeInputStreamWithFdsChild(aStream, aValue, aManager);
>+      return;
>+    }
>   }
> 
>   // As a fallback, attempt to stream the data across using a SendStream
>   // actor.  This will fail for blocking streams.
>   nsCOMPtr<nsIAsyncInputStream> asyncStream = do_QueryInterface(aStream);
>   aValue = SendStreamChild::Create(asyncStream, aManager);
> 
>   if (!aValue.get_PSendStreamChild()) {
>diff --git a/xpcom/io/nsIStringStream.idl b/xpcom/io/nsIStringStream.idl
>--- a/xpcom/io/nsIStringStream.idl
>+++ b/xpcom/io/nsIStringStream.idl
>@@ -1,23 +1,23 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>-#include "nsIInputStream.idl"
>+#include "nsIAsyncInputStream.idl"
> 
> /**
>  * nsIStringInputStream
>  *
>  * Provides scriptable and specialized C++-only methods for initializing a
>  * nsIInputStream implementation with a simple character array.
>  */
> [scriptable, uuid(450cd2d4-f0fd-424d-b365-b1251f80fd53)]
>-interface nsIStringInputStream : nsIInputStream
>+interface nsIStringInputStream : nsIAsyncInputStream
> {
>     /**
>      * SetData - assign data to the input stream (copied on assignment).
>      *
>      * @param data    - stream data
>      * @param dataLen - stream data length (-1 if length should be computed)
>      *
>      * NOTE: C++ code should consider using AdoptData or ShareData to avoid
>diff --git a/xpcom/io/nsStringStream.cpp b/xpcom/io/nsStringStream.cpp
>--- a/xpcom/io/nsStringStream.cpp
>+++ b/xpcom/io/nsStringStream.cpp
>@@ -41,16 +41,17 @@ public:
>   NS_DECL_THREADSAFE_ISUPPORTS
>   NS_DECL_NSIINPUTSTREAM
>   NS_DECL_NSISTRINGINPUTSTREAM
>   NS_DECL_NSISEEKABLESTREAM
>   NS_DECL_NSISUPPORTSPRIMITIVE
>   NS_DECL_NSISUPPORTSCSTRING
>   NS_DECL_NSIIPCSERIALIZABLEINPUTSTREAM
>   NS_DECL_NSICLONEABLEINPUTSTREAM
>+  NS_DECL_NSIASYNCINPUTSTREAM
> 
>   nsStringInputStream()
>   {
>     Clear();
>   }
> 
>   explicit nsStringInputStream(const nsStringInputStream& aOther)
>     : mOffset(aOther.mOffset)
>@@ -97,23 +98,25 @@ NS_IMPL_RELEASE(nsStringInputStream)
> NS_IMPL_CLASSINFO(nsStringInputStream, nullptr, nsIClassInfo::THREADSAFE,
>                   NS_STRINGINPUTSTREAM_CID)
> NS_IMPL_QUERY_INTERFACE_CI(nsStringInputStream,
>                            nsIStringInputStream,
>                            nsIInputStream,
>                            nsISupportsCString,
>                            nsISeekableStream,
>                            nsIIPCSerializableInputStream,
>-                           nsICloneableInputStream)
>+                           nsICloneableInputStream,
>+                           nsIAsyncInputStream)
> NS_IMPL_CI_INTERFACE_GETTER(nsStringInputStream,
>                             nsIStringInputStream,
>                             nsIInputStream,
>                             nsISupportsCString,
>                             nsISeekableStream,
>-                            nsICloneableInputStream)
>+                            nsICloneableInputStream,
>+                            nsIAsyncInputStream)
> 
> /////////
> // nsISupportsCString implementation
> /////////
> 
> NS_IMETHODIMP
> nsStringInputStream::GetType(uint16_t* aType)
> {
>@@ -259,16 +262,43 @@ nsStringInputStream::ReadSegments(nsWrit
> NS_IMETHODIMP
> nsStringInputStream::IsNonBlocking(bool* aNonBlocking)
> {
>   *aNonBlocking = true;
>   return NS_OK;
> }
> 
> /////////
>+// nsIAsyncInputStream implementation
>+/////////
>+
>+NS_IMETHODIMP
>+nsStringInputStream::CloseWithStatus(nsresult aStatus)
>+{
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsStringInputStream::AsyncWait(nsIInputStreamCallback* aCallback,
>+                               uint32_t aFlags,
>+                               uint32_t aRequestedCount,
>+                               nsIEventTarget* aTarget)
>+{
>+  if (aTarget) {
>+    nsCOMPtr<nsIInputStreamCallback> callback;
>+    callback = NS_NewInputStreamReadyEvent(aCallback, aTarget);
>+    callback->OnInputStreamReady(this);
>+  } else {
>+    aCallback->OnInputStreamReady(this);
>+  }
>+  
>+  return NS_OK;
>+}
>+
>+/////////
> // nsISeekableStream implementation
> /////////
> 
> NS_IMETHODIMP
> nsStringInputStream::Seek(int32_t aWhence, int64_t aOffset)
> {
>   if (Closed()) {
>     return NS_BASE_STREAM_CLOSED;

Comment 27

[Ting-Yu Chou [:ting] (away)]

Kanru told me his patch in bug 1264642 will turn the uint8_t data array of SerializedStructuredCloneWriteInfo to a buffer list, and will not fail the assertion. I think it's probably the best way for resolving this issue, because IPCStream needs to accumulate the buffers in receiving side, and shmem still needs to allocate a big chunk of contiguous memory.

Comment 28

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

(In reply to Ting-Yu Chou [:ting] from comment #27)
> Kanru told me his patch in bug 1264642 will turn the uint8_t data array of
> SerializedStructuredCloneWriteInfo to a buffer list, and will not fail the
> assertion. I think it's probably the best way for resolving this issue,
> because IPCStream needs to accumulate the buffers in receiving side, and
> shmem still needs to allocate a big chunk of contiguous memory.

I was wrong about the assertion. Message::size() reports the total size of the message so the assertion will still fail for the test case. Bug 1264642 can avoid the allocation of large contiguous memory but cannot avoid sending them through IPC. Given bug 1262671 has already landed, do you think we can remove the assertion? We might still want to catch large allocations but we can do that in malloc().

Comment 29

[Andrew McCreight [:mccr8]]

I think that's more of a question for Bill, the IPC module owner.

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is a tricky case. Sending lots of data over IPC is usually bad, even when we send it in chunks, because we often end up deserializing it into a huge nsString or something. In this case we wouldn't be though.

I could imagine changing the assertion to identify particular bad cases--like the nsString thing--but I don't think it should be lifted entirely.

Comment 31

[Ting-Yu Chou [:ting] (away)]

Now I prefer to use Shmem to fix this instead of IPCStream because 1) the accumulation in receiving side could cause performance drawback, 2) the change will be more straightforward, but I'd like to hear your opinions.

Comment 32

[Andrew McCreight [:mccr8]]

(In reply to Ting-Yu Chou [:ting] from comment #31)
> Now I prefer to use Shmem to fix this instead of IPCStream because 1) the
> accumulation in receiving side could cause performance drawback, 2) the
> change will be more straightforward, but I'd like to hear your opinions.

I don't know much about this code, but that sounds reasonable to me.

Comment 33

[Ting-Yu Chou [:ting] (away)]

I'll resume the work once Kanru's patch in bug 1264642 for replacing the byte array of SerializedStructuredClone[Write|Read]Info to SerializedStructuredCloneBuffer gets landed.

Comment 34

[Jukka Jylänki]

Hey, just wanted to note that this bug is becoming quite a big blocker for a project that Mozilla contracts out with a partner. Given that we are rolling out e10s, we have a concern if we are shipping this site out while e10s-shipping Firefox will crash on it. I wonder how large of an undertaking we are talking about for the fix to this item (and its dependency)? It would be very helpful in giving us a sense on how to proceed with the project.

Here is a crash report from latest Nightly, looks quite similar than the one from comment 0: https://crash-stats.mozilla.com/report/index/f596f656-7df4-4484-805e-48cfa2160808

Comment 35

[Ting-Yu Chou [:ting] (away)]

NI for comment 34.

Comment 36

[Ting-Yu Chou [:ting] (away)]

:billm, I'd like to make Pickle::WriteBytes() to write to a Shmem if the size is over a limit. Do you have any concerns with that?

Comment 37

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

Bug 1264642 is almost finished. I expect to land it in one or two days. Which means we are targeting release 51.

Comment 38

[Jukka Jylänki]

Great, thanks, this is very valuable information for us!

Comment 39

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm wondering why this is still failing. The original post says that we're storing ~100MB in indexeddb. However, the crash report in comment 34 claims we're sending a message containing ~700MB. That's really big!

Even with Kan-Ru's patches, the assertion is still going to fail. I can see raising the limit in the assertion, but not to 700MB. Even with Kan-Ru's patches, here are all the copies of the data we'll have in memory (possibly at once):

Sender side:
A. The original ArrayBuffer: 700MB contiguous
B. The array buffer in structured clone format: 700MB non-contiguous
C. The structured clone data copied to an IPC message: 700MB non-contiguous

Receiver side:
D. The IPC message, eventually transferred to structured clone buffer to be stored: 700MB non-contiguous

That's a total of 2.8GB, potentially!

Jukka, do you have any idea why so much data is being stored? Does the game actually try to store a 700MB ArrayBuffer? Or are we doing something stupid in Firefox that inflates the size of the data being stored?

(In reply to Ting-Yu Chou [:ting] from comment #36)
> :billm, I'd like to make Pickle::WriteBytes() to write to a Shmem if the
> size is over a limit. Do you have any concerns with that?

I don't think that would help. That would convert (C) to a contiguous shmem, which is actually worse. However, that shmem could be shared with (D). We might be able to modify the structured clone code so that it could use the shmem directly to represent the structured clone data, but that would take some work. And it would still only save one copy.

I'm not sure what we can realistically do here in a limited time-frame.

Comment 40

[Ting-Yu Chou [:ting] (away)]

(In reply to Bill McCloskey (:billm) from comment #39)
> (In reply to Ting-Yu Chou [:ting] from comment #36)
> > :billm, I'd like to make Pickle::WriteBytes() to write to a Shmem if the
> > size is over a limit. Do you have any concerns with that?
> 
> I don't think that would help. That would convert (C) to a contiguous shmem,
> which is actually worse. However, that shmem could be shared with (D). We
> might be able to modify the structured clone code so that it could use the
> shmem directly to represent the structured clone data, but that would take
> some work. And it would still only save one copy.

The reason I'd like to use Shmem is to avoid the 256MB upper limit assertion when the IPC message is that big. Otherwise it means we do not allow any oversized IPC, is this what we want?

Comment 41

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Ting-Yu Chou [:ting] from comment #40)
> The reason I'd like to use Shmem is to avoid the 256MB upper limit assertion
> when the IPC message is that big. Otherwise it means we do not allow any
> oversized IPC, is this what we want?

The only purpose of the assertion is to make sure we don't send too much data. If we wanted to avoid the assertion, we could just remove it. My concern with that is that we're wasting a ton of memory right now. The assertion at least let's us know that's happening.

Comment 42

[Ting-Yu Chou [:ting] (away)]

In that case, would a telemetry be more appropriate?

Comment 43

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I guess that would be okay. But the fact is still that this code is very likely to crash in 32-bit Firefox just because of address space exhaustion.

Comment 44

[Jukka Jylänki]

Oh sorry, the crash report from comment 34 was from a different test case than the one in comment 0. It is possible that the test case in comment 34 attempts to store 700MB, since it is a game that has assets close to a gigabyte range.

For comment 0, I added annotation prints to the test case, which shows the amount of data it is attempting to store. It prints out something like this:

Stored file PlatformerGame.data.js.gz to IndexedDB cache. (of type String: 7593 characters.)
Stored file Utility.js.gz to IndexedDB cache. (of type String: 55268 characters.)
Stored file PlatformerGame-HTML5-Shipping.asm.js.gz to IndexedDB cache. (of type String: 94797256 characters.)
Successfully compiled asm.js code (total compilation time 10626ms; stored in cache)
Stored file PlatformerGame-HTML5-Shipping.js.gz to IndexedDB cache. (of type String: 83851 characters.)
Stored file PlatformerGame.data.gz to IndexedDB cache. (of type ArrayBuffer: 104251615 bytes)

So the expected storage size for that one is around 200MB.

Comment 45

[Andrew McCreight [:mccr8]]

(In reply to Jukka Jylänki from comment #44)
> So the expected storage size for that one is around 200MB.

(I think comment 0 was when the limit was 128MB, compared to 256MB now, so that case should be okay now.)

Comment 46

[Ting-Yu Chou [:ting] (away)]

(In reply to Andrew McCreight [:mccr8] from comment #45)
> (In reply to Jukka Jylänki from comment #44)
> > So the expected storage size for that one is around 200MB.
> 
> (I think comment 0 was when the limit was 128MB, compared to 256MB now, so
> that case should be okay now.)

Could you confirm that?

Comment 47

[Ting-Yu Chou [:ting] (away)]

With the STR, it does not crash now with my local build nightly.

Comment 48

[Jukka Jylänki]

(In reply to Ting-Yu Chou [:ting] from comment #46)
> (In reply to Andrew McCreight [:mccr8] from comment #45)
> > (In reply to Jukka Jylänki from comment #44)
> > > So the expected storage size for that one is around 200MB.
> > 
> > (I think comment 0 was when the limit was 128MB, compared to 256MB now, so
> > that case should be okay now.)
> 
> Could you confirm that?

I can confirm that the STR from comment 0 no longer crashes. To be clear, while raising the limit until the browser crashes is a great workaround, it is not a fix, since the root bug still exists and it's trivial to make an application that does still crash. Comment 40 sounds to me like a good idea, using a construct that can avoid the limitation altogether, is that still planned, or is that infeasible?

If we do think there needs to be a limit on how much one can send, we should make sure that the IndexedDB operation in question will throw an exception back to JS (or invoke the failure handler of the operation), instead of bringing down the process on an assert() when the limit is violated.

If the JS page code was modified to store a Blob instead of a typed array, would things be any different/better?

Comment 49

[Jan Varga [:janv]]

(In reply to Jukka Jylänki from comment #48)
> If the JS page code was modified to store a Blob instead of a typed array,
> would things be any different/better?

I think if the blob is memory backed then nothing changes.
It will still need to be serialized.

Comment 50

[Ting-Yu Chou [:ting] (away)]

According to comment 39, 41, 42, and 43, I believe replacing the assertion with a telemetry is the best option.

Per comment 39, once bug 1264642 gets landed, only the original ArrayBuffer is a contiguous memory block, which I think is probably the best we can do to avoid address space exhaustion in this case. If we really want to do more, upgrade to 64bit version of Firefox (bug 1274659) seems a better option.

What do you guys think?

Comment 51

[Jan Varga [:janv]]

I think we could enhance the code that sends data through IPC for add/put/get.
The main problem is the byte array for structured clone. We could split the array into smaller chunks and send them gradually. It would be even better if structure cloning produced these chunks.
Anyway, it's not trivial to implement this since current architecture passes the byte array as an argument to IPDL actor constructor or to delete method when the actor is destroyed. We need new methods and synchronization for the chunking.

Comment 52

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

(In reply to Jan Varga [:janv] from comment #51)
> I think we could enhance the code that sends data through IPC for
> add/put/get.
> The main problem is the byte array for structured clone. We could split the
> array into smaller chunks and send them gradually. It would be even better
> if structure cloning produced these chunks.
> Anyway, it's not trivial to implement this since current architecture passes
> the byte array as an argument to IPDL actor constructor or to delete method
> when the actor is destroyed. We need new methods and synchronization for the
> chunking.

This is exactly bug 1264642.

Comment 53

[Jan Varga [:janv]]

Ah sorry, it was already mentioned here.

Comment 54

[Ting-Yu Chou [:ting] (away)]

There's already a telemetry IPC_MESSAGE_SIZE in MessageChannel::Send() [1] when the message size is >=8k.

[1] https://dxr.mozilla.org/mozilla-central/rev/054d4856cea6150a6638e5daf7913713281af97d/ipc/glue/MessageChannel.cpp#767-770

Comment 55

[Jan Varga [:janv]]

Ok, I talked to :ting and :kanru. I think I understand the final outcome of kanru's patches now.
So for IndexedDB we still need to flatten buffers on the parent side before we do the compression and then storing in sqlite.
The flattening can be removed if we fix the compression to take chunks and also sqlite to store data incrementally. That's quite a lot of work, so for now we can convert the IPC message size assertion to a warning maybe, but at the same time we also need to add a check to IndexedDB to return an error when data for add/put is too big (the check needs to be on the child side to bail out early). It seems chrome already does something similar.

Comment 56

[Ting-Yu Chou [:ting] (away)]

(In reply to Jan Varga [:janv] from comment #55)
> bail out early). It seems chrome already does something similar.

https://cs.chromium.org/chromium/src/content/child/indexed_db/indexed_db_dispatcher.cc?sq=package:chromium&dr=CSs&rcl=1471534131&l=414

Comment 57

[Jan Varga [:janv]]

(In reply to Ting-Yu Chou [:ting] from comment #56)
> (In reply to Jan Varga [:janv] from comment #55)
> > bail out early). It seems chrome already does something similar.
> 
> https://cs.chromium.org/chromium/src/content/child/indexed_db/
> indexed_db_dispatcher.cc?sq=package:chromium&dr=CSs&rcl=1471534131&l=414

Ok, so they allow no more than 128 MB for IPC messages. There's 1 MB IDB overhead, so there's 127 MB for key + value.
Also, their code also indicates that messages bigger than 128 MB causes the IPC channel to error out.

Comment 58

[Jan Varga [:janv]]

(In reply to Bill McCloskey (:billm) from comment #39)
> Sender side:
> A. The original ArrayBuffer: 700MB contiguous
> B. The array buffer in structured clone format: 700MB non-contiguous
> C. The structured clone data copied to an IPC message: 700MB non-contiguous
> 
> Receiver side:
> D. The IPC message, eventually transferred to structured clone buffer to be
> stored: 700MB non-contiguous

B,C and D can be greatly reduced if the whole thing works in chunks say at IndexedDB actor level (see my comment 51). I mean that we would progressively send partial structured clone buffers across IPC and process them in the parent one by one (for this we also need to fix the compression and sqlite to take chunks).
So a chunk would be destroyed when it's processed on both sides of the communication.
However, I'm not sure if it's worth it right now.

Comment 59

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

If we add a size check to indexeddb, then we should leave the assertion in. Otherwise we won't find other issues like this.

Also, it does mean that Jukka's code will now die with an exception rather than a crash. Jukka, is there a way you can change the code to break up the indexeddb data into smaller chunks? Apparently Chrome already requires this.

Also, I agree that streaming the data would be a great thing to do, but probably quite a bit of work.

Comment 60

[Andrew McCreight [:mccr8]]

(In reply to Bill McCloskey (:billm) from comment #59)
> Also, it does mean that Jukka's code will now die with an exception rather
> than a crash. Jukka, is there a way you can change the code to break up the
> indexeddb data into smaller chunks? Apparently Chrome already requires this.

Not only does Chrome requires it, but it seems to be using the lower 128MB limit we had for a while. (Rather than the current 256MB.)

Comment 61

[Jukka Jylänki]

Sorry for the delay in responding.

(In reply to Bill McCloskey (:billm) from comment #59)
> If we add a size check to indexeddb, then we should leave the assertion in.
> Otherwise we won't find other issues like this.
> 
> Also, it does mean that Jukka's code will now die with an exception rather
> than a crash. Jukka, is there a way you can change the code to break up the
> indexeddb data into smaller chunks? Apparently Chrome already requires this.

Yeah, getting an exception out that tells that a size limit was exceeded sounds great! (or in IndexedDB parlance, that would call the error handler of the operation?) We can live with a limitation like that and break up the files. When an exception is thrown, it should tell what the browser imposed limit is, and how much data was attempted to be transferred, so that it is clear for the developer what they should do to adapt.

Comment 62

[Jan Varga [:janv]]

(In reply to Jukka Jylänki from comment #61)
> Yeah, getting an exception out that tells that a size limit was exceeded
> sounds great! (or in IndexedDB parlance, that would call the error handler
> of the operation?)

We do serialize the passed JS object before Add()/Put() method returns to the caller.
So we know serialized size synchronously and we can just throw an exception instead
of calling the error handler.

Comment 63

[Ting-Yu Chou [:ting] (away)]

So I guess the remaining things we can do here are:

1) add a size limit to Add()/Put() which raise an error when the data is oversized (I am not sure if this is really what we want)
2) remove the flattening Jan mentioned in comment 55

Jan, should I reassign this to you or someone else?

Comment 64

[Jan Varga [:janv]]

(In reply to Ting-Yu Chou [:ting] from comment #63)
> So I guess the remaining things we can do here are:
> 
> 1) add a size limit to Add()/Put() which raise an error when the data is
> oversized (I am not sure if this is really what we want)

Well, we would be on par with chrome and it's quite easy to implement it.

> 2) remove the flattening Jan mentioned in comment 55

This is much harder to do.

> 
> Jan, should I reassign this to you or someone else?

Maybe bevis could do 1.

Comment 65

[Bevis Tseng[:bevis][:btseng](Exited)]

Instead of adding the size-limit check immediately in IDB,
I'd like to take this to have more discussion on the priority and solution of this bug:
1. In comment 56, we can see that chrome has set the size limitation to 128 MB while our assertion to the size limit is set to 256MB.
2. That means web pages with massive storage IDBRequest running in chrome must be able to run in gecko without problems. (and chrome owns the biggest market share for now)
3. It's an advantage if there is no size-limitation in our implementation. With the increase of the memory availability in end user's hardware, we can expect that the memory footprint of the web page will be increased as well. Hence, eventually, we'll have to remove limitation in ipc channel instead of having this size-limit check in IDB implementation. The size-limit check is just a nice-to-have solution for now, IMHO.

If there is no concern, I'd prefer to lower the priority of this bug instead.

Comment 66

[Jan Varga [:janv]]

3. would definitely be a competitive advantage, but as I said it's a lot of work

Comment 67

[Marco Castelluccio [:marco]]

Reproducible on https://catanuniverse.com/en/game: https://crash-stats.mozilla.com/report/index/819041bc-3deb-4bb1-8a71-e99742160929.

There are a few other URLs where you can probably reproduce with this search: https://crash-stats.mozilla.com/search/?moz_crash_reason=%3DMOZ_CRASH%28IPC%20message%20size%20is%20too%20large%29&ipc_message_name=%3DPBackgroundIDBRequest%3A%3AMsg___delete__&product=Firefox&_sort=-date&_facets=signature&_facets=ipc_message_name&_facets=url&_facets=addons&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-url

Comment 68

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I really think we should add the size limitation to IDB. There will always be some limit. I don't think 128MB is unreasonable. And it's a ton of work to fix all the things that would make storing huge IDB records efficient. This should have high priority since it's an easy way to crash and it looks bad.

Comment 69

[Bevis Tseng[:bevis][:btseng](Exited)]

It's really bad that the crash happens immediately when loading the 1st link in comment 67. :(
We should have a fix for the crash firstly in this bug.

Comment 70

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: WIP Patch: Throw UnknowError if the serialized message size is too large.

WIP to accumulate the size of serialized IDB object according to:
1. size of the written StructuredCloneBuffer.
2. size of the encoded object key.
3. size of the encoded index keys.

The error could be displayed properly in both web console and browser console with the information of maximal message size and the size of the serialized object for developers to refer to.

Note: there is a drawback in current IDB implementation which causes the size of the key and index key double because we have to append these encoded keys along with the serialized object when sending the IPC message to parent.
But this is not a normal use case to have key with large amount of data because it will slow down the performance of indexing.

Waiting for test result in treeherder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=244c10912750&selectedJob=29234455

Comment 71

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #70)
> Note: there is a drawback in current IDB implementation which causes the
> size of the key and index key double because we have to append these encoded
> keys along with the serialized object when sending the IPC message to parent.
while keys(the properties of the object specified by keypath) are also serialized inside StructuredCloneBuffer.

Comment 72

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch: Throw UnknowError if the serialized message size is too large.

1. Revise the test case in WIP to reuse the allocated memory in different tests to prevent OOM problem in treeherder.
2. Test is disabled in android platform because the memory in the test environment is not enough to run the test. (We'll get TIMEOUT in this case.)

Update treeherder result for reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e3993bee675

Comment 73

[Jan Varga [:janv]]

Comment on attachment 8802400 [details] [diff] [review]
(v1) Patch: Throw UnknowError if the serialized message size is too large.

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1276,5 @@
> +  for (size_t i = 0; i < updateInfo.Length(); i++) {
> +    indexUpdateInfoSize += updateInfo[i].value().GetBuffer().Length();
> +    indexUpdateInfoSize += updateInfo[i].localizedValue().GetBuffer().Length();
> +  }
> +

Maybe put a comment here that kMaxIDBMsgOverhead should cover other stuff that we don't include in the calculation (exact calculation would slow down AddOrPut)

@@ +1278,5 @@
> +    indexUpdateInfoSize += updateInfo[i].localizedValue().GetBuffer().Length();
> +  }
> +
> +  static const size_t kMaxIDBMsgOverhead = 1024 * 1024; // 1MB
> +  static const size_t kMaxMessageSize =

I would put these consts first, then the computation of indexUpdateInfoSize and finally messageSize.
The computation of messageSize could use the same order as we use for setting commonParams, that is, cloneWriteInfo.mCloneBuffer.data().Size() + key.GetBuffer().Length() + indexUpdateInfoSize

@@ +1287,5 @@
> +  if (messageSize > kMaxMessageSize) {
> +    IDB_REPORT_INTERNAL_ERR();
> +    aRv.ThrowDOMException(NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR,
> +      nsPrintfCString("The serialized value is too large"
> +                      " (size=%zu bytes, max=%zu bytes).",

This is probably not localizable, but I guess it's ok for now.

::: dom/indexedDB/test/unit/test_maximal_serialized_object_size.js
@@ +8,5 @@
> +function testSteps()
> +{
> +  const name = this.window ? window.location.pathname : "Splendid Test";
> +
> +  let OpenDbRequest = indexedDB.open(name, 1);

Nit: OpenDbRequest -> openDbRequest or just openRequest

@@ +36,5 @@
> +         "Correct error message prefix.");
> +    }
> +  }
> +
> +  const kMessageOverhead = 1; // in MB

Nit: These consts could go right after "const name" in front of the main function.

@@ +37,5 @@
> +    }
> +  }
> +
> +  const kMessageOverhead = 1; // in MB
> +  const kMaxIpcMessageSize = 256; // in MB

I'm a bit worried about this size, we don't really need to use such big messages for testing. You already had to disable this on Android, so it won't be tested there. This might also cause performance problems like bug 1272855.
Maybe we should add a pref for this or something. So we could restrict IDB addOrPut() to even lower sizes (ignoring IPC::Channel::kMaximumMessageSize).

@@ +51,5 @@
> +  info("Verify IDBObjectStore.add() - object key is too large");
> +  chunks.length = 128;
> +  testTooLargeError("add", { id: chunks, index: 0 });
> +
> +  const indexName = "index name";

Nit: again, could be in the front

Comment 74

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #73)
> @@ +37,5 @@
> > +    }
> > +  }
> > +
> > +  const kMessageOverhead = 1; // in MB
> > +  const kMaxIpcMessageSize = 256; // in MB
> 
> I'm a bit worried about this size, we don't really need to use such big
> messages for testing. You already had to disable this on Android, so it
> won't be tested there. This might also cause performance problems like bug
> 1272855.
> Maybe we should add a pref for this or something. So we could restrict IDB
> addOrPut() to even lower sizes (ignoring IPC::Channel::kMaximumMessageSize).

Maybe something like this:
https://bugzilla.mozilla.org/attachment.cgi?id=8802726&action=edit

You could also add a new add() test that is expected to pass with data close to the maximum.

Comment 75

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Patch: Throw UnknowError if the size of the serialized message is too large.

1. Refine the calculation order in AddOrPut().
2. Add a pref to optionally change the size of the limitation for the testing in all platforms. (20MB is set for testing with reasonable testing time.)
3. Add a new add() test that is expected to pass with data close to the maximum.
4. Address the nits in the test case.

Treeherder result looks fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eddbb381ad07

Comment 76

[Jan Varga [:janv]]

Comment on attachment 8806574 [details] [diff] [review]
(v2) Patch: Throw UnknowError if the size of the serialized message is too large.

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

Perfect!

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1753c75341a6
Throw UnknowError if the size of the serialized message is too large. r=janv

Comment 78

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1753c75341a6