1275062 - IndexedDB IDB Request Send__delete__ operation crashes when attempting to send too much data via an IPC pipe

Status: RESOLVED FIXED

Description

[Jukka Jylänki]

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1271102#c9, reporting as a separate entry from bug 1271102, even though the crashing line is same as in that bug.

A deterministic crash on current Nightly on Windows at least, a crash in call stack

mozilla::ipc::ProcessLink::SendMessageW
mozilla::ipc::MessageChannel::Send
mozilla::dom::indexedDB::PBackgroundIDBRequestParent::Send__delete__
mozilla::dom::indexedDB::`anonymous namespace'::NormalTransactionOp::SendSuccessResult
mozilla::dom::indexedDB::`anonymous namespace'::TransactionDatabaseOperationBase::RunOnOwningThread
mozilla::dom::indexedDB::`anonymous namespace'::TransactionDatabaseOperationBase::Run
nsThread::ProcessNextEvent

can occur related to IDB operations on large byte arrays. This is similar to bug 1274075.

Crash reports:

https://crash-stats.mozilla.com/report/index/dde0e9ed-3e6b-43cd-be32-bef4e2160523
https://crash-stats.mozilla.com/report/index/2f3e25be-1ed2-47eb-a11a-455a42160523
https://crash-stats.mozilla.com/report/index/8485b481-c264-4804-8e34-428a22160523

Comment 1

[Jim Mathies [:jimm]]

ni for a reminder to chat with idb folks about data chunking.

Comment 3

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

Although bug 1277744 was fixed by limiting the size of the object to be sent from child to parent, it's still possible to hit the assertion of |IPC::Channel::kMaximumMessageSize| check in parent by (IDBObjectStore|IDBIndex).(getAll|getAllKey) which returns object arrays or key arrays at once.
In addition, we should also check the size of the response of (IDBObjectStore|IDBIndex).(get|getKey) as well if the objects are added before the |IPC::Channel::kMaximumMessageSize| check is added.

In short, we should check the size limit of the indexedDB::RequestResponse [1] containing |SerializedStructuredCloneReadInfo| and |Key| as data member at NormalTransactionOp::SendSuccessResult(), and returns NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR and IDB_REPORT_INTERNAL_ERR() to prevent parent process to be crashed for now.

I'd like to take this bug to follow up.

The support of data chunking is the long-term solution to resolve the internal error caused by the implementation.

[1] http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/dom/indexedDB/PBackgroundIDBRequest.ipdl#90-107

Comment 4

[Marion Daly [:mdaly]]

:baku, high priority, Bevis is no longer available to work on it, can you pick this up?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: idb.patch

asuth, what do you think about this approach?

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8944760 [details] [diff] [review]
idb.patch

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

Looks like a good improvement!

Restating: IPC messages have a max size and the IPC subsystem intentionally triggers a crash if we exceed it.  Bug 1277744 fixed a similar problem on the sending side using fundamentally the same logic.  The maximum message size, 256MiB, is used as a base, plus an additional 1MiB (proposed to be 10MiB) of buffer is subtracted off because we're not using the actual IPC wire sizes.

NormalTransactioOp, the superclass of all data-returning operations[1] and all of its subclasses are modified to estimate their wire-sizes by counting up the structured clone data, plus one uint64_t per StructuredCloneFile (which is how we send all IDB out-of-line structured clone data, so blobs, mutablefiles, etc.).  We return a fairly opaque error if we hit the size limit, but we will file a spin-off to improve that once bug 1357463 makes it easy to upgrade to sending ErrorResult instances over the wire.

1: https://github.com/asutherland/asuth-gecko-notes/blob/f76f88c490da126c22dd560c6e1e4ee5ce40b1cc/dom/indexedDB/OVERVIEW.md#classes

::: dom/indexedDB/ActorsParent.cpp
@@ +25729,5 @@
>  {
>    AssertIsOnOwningThread();
>  
>    if (!IsActorDestroyed()) {
> +    static const size_t kMaxIDBMsgOverhead = 1024 * 1024; // 1MB

I propose we bump this up to 10 MiB.  My rationale is:
- We're talking about the parent process crashing, versus the content process crashing in bug 1274075, and parent crashes are much worse.
- The dominant concern in this case is getAll* methods, versus add()/put(), so the potential for our arbitrarily chosen value of 1 MiB (taken from that bug?) to under-compensate for the large N possible in these cases increases.
- Content logic arguably shouldn't be using getAll* for result sets of this size.  They can and will jank the main thread if deserialized on the main thread, etc.  The getAll* methods allow ranges to be specified to subset results, plus there are all the cursor-based methods.  As such, even if we're making the padding amount way too large, it's still a win.

@@ +25740,5 @@
> +    size_t responseSize = kMaxMessageSize;
> +    GetResponse(response, &responseSize);
> +
> +    if (responseSize >= kMaxMessageSize) {
> +      return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;

Suggest following a file-up to improve the returned error that depends on Bug 1357463 which will enable custom error strings.  Right now we're in the parent process and using existing IPC actors that use nsresult, so I think it's reasonable to wait for ErrorResult.  For comparison, bug 1274075 added the more helpful error message of:

      nsPrintfCString("The serialized value is too large"
                      " (size=%zu bytes, max=%zu bytes).",
                      messageSize, kMaxMessageSize));

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca73f9471ede
Introduce a size check of IPC messages for IndexedDB, r=asuth

Comment 8

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/ca73f9471ede

Comment 9

[Marco Castelluccio [:marco] (PTO until August 30)]

Should we uplift to beta?

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8944760 [details] [diff] [review]
idb.patch

Approval Request Comment
[Feature/Bug causing the regression]: IndexedDB
[User impact if declined]: big IPC messages can trigger a crash
[Is this code covered by automated tests?]: n/a
[Has the fix been verified in Nightly?]: n/a
[Needs manual test from QE? If yes, steps to reproduce]: n/a
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no.
[Why is the change risky/not risky?]: GetResponse(), when creating the IPC message, returns also its size. The size is used to know if an error has to be thrown, or the message can be send.
[String changes made/needed]: none

Comment 11

[Liz Henry (:lizzard)]

Comment on attachment 8944760 [details] [diff] [review]
idb.patch

Since we haven't seen any new problems on Nightly caused by this, let's uplift for beta 7.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/24ea37d401ad