Closed
Bug 1275062
Opened 5 years ago
Closed 3 years ago
IndexedDB IDB Request Send__delete__ operation crashes when attempting to send too much data via an IPC pipe
Categories
(Core :: Storage: IndexedDB, defect, P1)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: jujjyl, Assigned: baku)
References
Details
Crash Data
Attachments
(1 file)
14.35 KB,
patch
|
asuth
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Updated•5 years ago
|
tracking-e10s:
--- → ?
![]() |
||
Updated•5 years ago
|
Priority: -- → P1
![]() |
||
Comment 1•5 years ago
|
||
ni for a reminder to chat with idb folks about data chunking.
Flags: needinfo?(mrbkap)
Updated•5 years ago
|
Updated•5 years ago
|
Updated•4 years ago
|
Crash Signature: [@ mozilla::ipc::ProcessLink::SendMessage | IPC_Message_Name=PBackgroundIDBRequest::Msg___delete__ ]
[@ mozilla::ipc::ProcessLink::SendMessageW | IPC_Message_Name=PBackgroundIDBRequest::Msg___delete__ ]
Comment 3•4 years ago
|
||
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
Assignee: nobody → btseng
Updated•4 years ago
|
Flags: needinfo?(mrbkap)
Updated•3 years ago
|
Assignee: btseng → nobody
Updated•3 years ago
|
Assignee: nobody → amarchesini
Comment 4•3 years ago
|
||
:baku, high priority, Bevis is no longer available to work on it, can you pick this up?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 5•3 years ago
|
||
asuth, what do you think about this approach?
Flags: needinfo?(amarchesini)
Attachment #8944760 -
Flags: review?(bugmail)
Comment 6•3 years ago
|
||
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));
Attachment #8944760 -
Flags: review?(bugmail) → review+
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ca73f9471ede Introduce a size check of IPC messages for IndexedDB, r=asuth
Comment 8•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ca73f9471ede
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 9•3 years ago
|
||
Should we uplift to beta?
status-firefox58:
--- → affected
status-firefox59:
--- → affected
status-firefox-esr52:
--- → affected
Assignee | ||
Comment 10•3 years ago
|
||
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
Attachment #8944760 -
Flags: approval-mozilla-beta?
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.
Attachment #8944760 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 12•3 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/24ea37d401ad
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•