1591877 - Crash in [@ InvalidArrayIndex_CRASH | mozilla::dom::quota::QuotaManager::GetUsageForClient]

Status: VERIFIED FIXED

(Core :: Storage: Quota Manager, defect, P3)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

This bug is for crash report bp-15409535-80bd-4247-bdbc-11f7c0191027.

Top 10 frames of crashing thread:

0 libxul.so InvalidArrayIndex_CRASH xpcom/ds/nsTArray.cpp:27
1 libxul.so mozilla::dom::quota::QuotaManager::GetUsageForClient dom/quota/ActorsParent.cpp:4139
2 libxul.so mozilla::dom:: dom/localstorage/ActorsParent.cpp:7807
3 libxul.so mozilla::dom:: dom/localstorage/ActorsParent.cpp:6726
4 libxul.so nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1225
5 libxul.so <name omitted> xpcom/threads/nsThreadUtils.cpp:486
6 libxul.so mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:303
7 libxul.so MessageLoop::Run ipc/chromium/src/base/message_loop.cc:290
8 libxul.so nsThread::ThreadFunc xpcom/threads/nsThread.cpp:458
9 libnspr4.so _pt_root nsprpub/pr/src/pthreads/ptthread.c:201

The raw crash reason is ElementAt(aIndex = 3, aLength = 3).

The unwinding hides the real line where we're crashing, looking at the frames it appears to be this one:

https://searchfox.org/mozilla-central/rev/74cc0f4dce444fe0757e2a6b8307d19e4d0e0212/dom/quota/ActorsParent.cpp#7891

Comment 1

[Jens Stutte [:jstutte]]

So OriginInfo contains ClientUsageArray mClientUsages;, which is initialized to have length:

  static Type TypeMax() {
    if (CachedNextGenLocalStorageEnabled()) {
      return TYPE_MAX;
    }
    return LS;
  }

and never touched afterwards in terms of length until destruction. I see two possible reasons:

1. We changed the pref driving CachedNextGenLocalStorageEnabled() during runtime to false, creating thus smaller mClientUsages but somewhere in our code we still expect to have the longer variant. In any case it looks a bit odd, that the memory layout of an array of a probably serialized object depends on a pref, that might flip anytime.
2. We try to access mClientUsages after that it has been deconstructed (which probably means, the underlying OriginInfo has been freed).

Comment 2

[Tom Tung [:tt, :ttung]]

I got a crash (id: bp-522ee260-3be6-4d7c-a0b2-f78a80210330) with the same signature after I had flipped the pref for LSNG and had reloaded a tab.

Comment 3

[Jens Stutte [:jstutte]]

(In reply to Tom Tung [:tt, :ttung] from comment #2)

I got a crash (id: bp-522ee260-3be6-4d7c-a0b2-f78a80210330) with the same signature after I had flipped the pref for LSNG and had reloaded a tab.

Hi Tom, so you are saying this does reproduce?

Comment 4

[Tom Tung [:tt, :ttung]]

(In reply to Jens Stutte [:jstutte] from comment #3)

(In reply to Tom Tung [:tt, :ttung] from comment #2)

I got a crash (id: bp-522ee260-3be6-4d7c-a0b2-f78a80210330) with the same signature after I had flipped the pref for LSNG and had reloaded a tab.

Hi Tom, so you are saying this does reproduce?

(Edited: the step for reloading a tab without an LSNG storage is not needed to reproduce this crash)
Yes, at least, I reproduced it through:
0. Daily use Nightly (dom.storage.next_gen is true)

1. Turn the pref to false.
2. Restart Nightly.
3. Turn the pref to true.
4. Reload a tab with an LSNG storage (In my case, a Bugzilla page).

I will see if I can provide more details. And I am happy to share my profile if it's needed. :)
The second crash report (https://crash-stats.mozilla.org/report/index/d58648d8-54e5-4387-a652-8e6010210330).
Interestingly, the second time, my Nightly hangs for minutes but I terminated it by Ctrl+c. Not sure some other issue is involved or not.

Comment 5

[Timea Cernea [:tbabos][inactive]]

This crash signature is also reproducible with the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1719453#c1.

Comment 6

[Jan Varga [:janv]]

Attachment: Bug 1591877 - Forbid creation of PBackgroundLSRequest/PBackgroundLSSimpleRequest actors in the parent process if LSNG is not enabled; r=#dom-storage

Comment 7

[Jan Varga [:janv]]

Attachment: Bug 1591877 - Share the same cached pref value of dom.storage.next_gen across all relevant processes; r=#dom-storage

Depends on D119710

Comment 9

[Jan Varga [:janv]]

https://treeherder.mozilla.org/jobs?repo=try&revision=6c5e473be1175b480e59cafd7d5c3bf5a8c299f3

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78cabff7b610
Forbid creation of PBackgroundLSRequest/PBackgroundLSSimpleRequest actors in the parent process if LSNG is not enabled; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/3b175989fe8c
Share the same cached pref value of dom.storage.next_gen across all relevant processes; r=dom-storage-reviewers,jstutte

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/78cabff7b610
https://hg.mozilla.org/mozilla-central/rev/3b175989fe8c

Comment 12

[Jan Varga [:janv]]

Comment on attachment 9230861 [details]
Bug 1591877 - Share the same cached pref value of dom.storage.next_gen across all relevant processes; r=#dom-storage

Beta/Release Uplift Approval Request

• User impact if declined: We would like to start an experiment in 91 which enables LSNG for some users. We can't do that without this crash fix.
• 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:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes are rather small and well tested locally.
• String changes made/needed: None

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9230861 [details]
Bug 1591877 - Share the same cached pref value of dom.storage.next_gen across all relevant processes; r=#dom-storage

Crash fix, approved for 91 beta 3, thanks.

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1656e86faf04
https://hg.mozilla.org/releases/mozilla-beta/rev/d6af2879a42b

Comment 15

[Jan Varga [:janv]]

Comment on attachment 9230861 [details]
Bug 1591877 - Share the same cached pref value of dom.storage.next_gen across all relevant processes; r=#dom-storage

Beta/Release Uplift Approval Request

• User impact if declined: We would like to start an experiment in 90 which enables LSNG for some users. We can't do that without this crash fix.
• 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:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes are rather small and well tested locally.
• String changes made/needed: None

Comment 16

[Timea Cernea [:tbabos][inactive]]

Verified-fixed on the latest Firefox Nightly 92.0a1 (BuildID: 20210715215249) and on the latest Beta 91.0b3 on Windows 7/10, MacOS 10.15 and Ubuntu 20.04.
We were able to reproduce the crashes on the affected builds with all the scenarios provided in this bug and from the duplicates as well.
On the fixed builds, the crashes did not occur anymore.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 9230861 [details]
Bug 1591877 - Share the same cached pref value of dom.storage.next_gen across all relevant processes; r=#dom-storage

approved for 90.0.1

Comment 18

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/99bb6603fa7f
https://hg.mozilla.org/releases/mozilla-release/rev/bf29cf50c10c

Comment 19

[Timea Cernea [:tbabos][inactive]]

Verified fixed on the latest dot Release 90.0.1 on Windows 7/10, MacOS 10.15 and Ubuntu 20.04.