1652409 - Crash in [@ mozilla::dom::indexedDB::(anonymous namespace)::ReadCompressedNumber]

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto] (PTO)]

This bug is for crash report bp-6f44dad1-9131-475d-8538-71cd60200713.

Top 10 frames of crashing thread:

0 XUL mozilla::dom::indexedDB:: dom/indexedDB/ActorsParent.cpp:662
1 XUL mozilla::dom::indexedDB:: dom/indexedDB/ActorsParent.cpp:799
2 XUL mozilla::dom::indexedDB:: dom/indexedDB/ActorsParent.cpp:894
3 XUL mozilla::dom::indexedDB:: dom/indexedDB/ActorsParent.cpp:25018
4 XUL mozilla::dom::indexedDB:: dom/indexedDB/ActorsParent.cpp:22717
5 XUL mozilla::dom::indexedDB:: dom/indexedDB/ActorsParent.cpp:22883
6 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1234
7 XUL NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:513
8 XUL mozilla::dom::indexedDB:: dom/indexedDB/ActorsParent.cpp:12991
9 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1234

This appeared in nightly with buildid 20200705095345 but even earlier in the beta channel with buildid 20200703001609. The raw crash reason is MOZ_RELEASE_ASSERT(span_ && (index_ + n) >= 0 && (index_ + n) <= span_->Length()). Somehow it only appears to affect macOS users.

Comment 1

[Simon Giesecke [:sg] [he/him]]

The anonymous namespace part of the signature is different between OS X and Windows. I added the Windows signature, there are crashes as well.

Comment 2

[Simon Giesecke [:sg] [he/him]]

On Windows, the first crash with this signature seems to be from build id 20200629224419

Comment 3

[Simon Giesecke [:sg] [he/him]]

Given the low volume of crash reports, even though that landed almost a week earlier, this is most likely to be caused by https://hg.mozilla.org/mozilla-central/rev/abd728a13d825387c87b1488610aa6d4759a6c13 from Bug 1623278

The particular assertion was introduced by the mentioned patch.

This might be a real regression, a change in signatures or some existing issue that wasn't caught before by an assertion.

Comment 4

[Simon Giesecke [:sg] [he/him]]

This seems to be the case that is already referenced by an XXX comment here: https://searchfox.org/mozilla-central/rev/af5cff556a9482d7aa2267a82f2ccfaf18e797e9/dom/indexedDB/ActorsParent.cpp#659

In my understanding, this is an existing issue that wasn't caught before by an assertion, but caused some error elsewhere before (not sure if a run-time error, a release assertion or just UB).

Jan, WDYT, should we change this to return an error in that case? Probably NS_ERROR_FILE_CORRUPTED?

Comment 5

[Simon Giesecke [:sg] [he/him]]

I guess that's what was ultimately happening before e.g. here: https://searchfox.org/mozilla-central/rev/cc4a5ebe14c5f8b54f744164e53805eec26aab07/dom/indexedDB/ActorsParent.cpp#825-831

So maybe one can view this as a regression of Bug 1623278, which implicitly promoted this from a runtime error to a release assertion. I'll prepare a patch.

Comment 6

[Jan Varga [:janv]]

Yeah, that makes sense, we probably don't have any other options other than returning NS_ERROR_FILE_CORRUPTED.

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1652409 - Ensure that a corrupted file encountered during ReadCompressedNumber does not cause an assertion failure. r=#dom-workers-and-storage

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/223990c25952
Ensure that a corrupted file encountered during ReadCompressedNumber does not cause an assertion failure. r=dom-workers-and-storage-reviewers,janv

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/223990c25952

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact here which justifies Beta backport consideration? Please nominate for approval if yes :)

Comment 11

[Simon Giesecke [:sg] [he/him]]

Comment on attachment 9163165 [details]
Bug 1652409 - Ensure that a corrupted file encountered during ReadCompressedNumber does not cause an assertion failure. r=#dom-workers-and-storage

Beta/Release Uplift Approval Request

• User impact if declined: This only impacts cases of corrupted storage, but it will prevent the parent process from crashing safely in this case, and instead let an error be reported to the content process.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: We don't have STR. (It would be good to have automated tests in general for cases of corrupted storage to better catch things like this.)
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Changes to error handling only to prevent a release assertion to fail.
• String changes made/needed:

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9163165 [details]
Bug 1652409 - Ensure that a corrupted file encountered during ReadCompressedNumber does not cause an assertion failure. r=#dom-workers-and-storage

Approved for 79.0b9.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aeb2043f8ae6