1481164 - Crash in InvalidArrayIndex_CRASH | mozilla::dom::indexedDB::(anonymous namespace)::ConnectionPool::NoteIdleDatabase

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

This bug was filed from the Socorro interface and is
report bp-a3293fca-0511-435e-bc0e-103e40180805.
=============================================================

Top 10 frames of crashing thread:

0 libmozglue.dylib MOZ_CrashPrintf mfbt/Assertions.cpp
1 XUL InvalidArrayIndex_CRASH xpcom/ds/nsTArray.cpp:26
2 XUL mozilla::dom::indexedDB:: xpcom/ds/nsTArray.h:1100
3 XUL mozilla::dom::indexedDB:: dom/indexedDB/ActorsParent.cpp:13003
4 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1235
5 XUL NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519
6 XUL mozilla::dom::indexedDB:: xpcom/threads/nsThreadUtils.h:324
7 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1235
8 XUL NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519
9 XUL mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:364

=============================================================

Comment 1

[Marion Daly [:mdaly]]

:jan this is a relatively low freq crasher, on your radar

Comment 2

[[:philipp]]

this issue is regressing in volume in firefox 63.

Comment 3

[Marion Daly [:mdaly]]

Removing NI

Comment 4

[steve]

firefox63:wontfix → is that a good idea?

I may be seeing this crash: https://crash-stats.mozilla.com/report/index/777ee3fa-a1ea-4e5e-8d2c-55db40181120

In case this is the correct bug for this crash (of which I am not entirely sure) this is pretty critical as it prevents any productive work from happening in firefox on that computer. I have 63.0.3 on a second computer with no crash at all. Makes me wonder if the crash is related to the old hardware of the 2011 macbook pro they are happening on?

Can't specifically reproduce but the crash just happenes without me doing anything. I can open Firefox with one tab open (duckduckgo) and do nothing → crash. Just this morning it has crashed at least 7 times. Just now while writing this report.

This is indeed a regression in 63 and was not the case for previous firefox versions.

Any info that could help address this crash? I understand that even ~60 crashes/day are not a lot looking at the entire firefox userbase. for me this crash is so bad that I have to consider switching the browser until this is fixed as I can not be productive with the constant interruptions.

Comment 5

[Gabriele Svelto [:gsvelto]]

I found even more instances of this.

Comment 6

[Gabriele Svelto [:gsvelto]]

I've talked with the user that left comment 4 and he can easily reproduce the bug. If we could provide him with a build with the proper debugging facilities we might get to the bottom of this, Jan is there someone from the IndexedDB team who could help with this?

Comment 7

[Gabriele Svelto [:gsvelto]]

Added one more signature and sorted them so that it's clearer.

Comment 8

[Gabriele Svelto [:gsvelto]]

... and it looks like there might be even more signatures:

https://crash-stats.mozilla.com/search/?signature=~NoteIdleDatabase#facet-signature

I don't have time to sift through all of them now but I will ASAP.

Comment 9

[Gabriele Svelto [:gsvelto]]

Added an ESR signature.

Comment 10

[Tom Tung [:tt, :ttung]]

It crashes at [1].

[1] https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/dom/indexedDB/ActorsParent.cpp#11381

From my initial impression, it seems there is a race for accessing the mIdleDatabases.
For instance, we insert a databaseInfo into it also on a connection thread (which can be different threads) [1-2]. At the same time, we remove an entry on the background thread [3].

Note that [2] and [3] have the same thread assertion (AssertIsOnOwningThread() for ConnectionPool), but the caller [4] of [3] is on the Background thread and the caller of [2] is on owning thread.

[1] https://crash-stats.mozilla.com/report/index/cd71d41e-7282-4874-a323-747800190422
[2] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/indexedDB/ActorsParent.cpp#11380
[3] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/indexedDB/ActorsParent.cpp#11558
[4] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/indexedDB/ActorsParent.cpp#12692

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1481164 - Dispatch to the Background thread even when the Connection is a nullptr in IdleConnectionRunnable::Run();

Comment 12

[Tom Tung [:tt, :ttung]]

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

It crashes at [1].

[1] https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/dom/indexedDB/ActorsParent.cpp#11381

From my initial impression, it seems there is a race for accessing the mIdleDatabases.
For instance, we insert a databaseInfo into it also on a connection thread (which can be different threads) [1-2]. At the same time, we remove an entry on the background thread [3].

Note that [2] and [3] have the same thread assertion (AssertIsOnOwningThread() for ConnectionPool), but the caller [4] of [3] is on the Background thread and the caller of [2] is on owning thread.

[1] https://crash-stats.mozilla.com/report/index/cd71d41e-7282-4874-a323-747800190422
[2] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/indexedDB/ActorsParent.cpp#11380
[3] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/indexedDB/ActorsParent.cpp#11558
[4] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/indexedDB/ActorsParent.cpp#12692

On IRC discussion, Jan catches this issue. It's because the code doesn't make idleConnectionRunnable dispatch itself to the background thread when a connection of the databaseInfo is null. So that it's possible that we can read this variable on a connection thread (which shouldn't be) while there is a write on the Background thread.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/158510da415d
Dispatch to the Background thread even when the Connection is a nullptr in IdleConnectionRunnable::Run(); r=janv

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/158510da415d

Comment 15

[[:philipp]]

should we consider uplifting the patch to 67 beta still at this point?

the [@ mozilla::dom::indexedDB::anonymous namespace'::ConnectionPool::NoteIdleDatabase(struct mozilla::dom::indexedDB::anonymous namespace'::ConnectionPool::DatabaseInfo*)] crash signature would account for 0.5% of browser crashes on 67.0b13 currently...

Comment 16

[Tom Tung [:tt, :ttung]]

Comment on attachment 9060169 [details]
Bug 1481164 - Dispatch to the Background thread even when the Connection is a nullptr in IdleConnectionRunnable::Run();

Beta/Release Uplift Approval Request

• User impact if declined: The patch fixes the place where breaks threading assumption for the case of the idle database if the connection is a nullptr. Thus, that might cause a race or another crash if declined.
• 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): This patch corrects the behavior and shouldn't be risky.
• String changes made/needed:

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9060169 [details]
Bug 1481164 - Dispatch to the Background thread even when the Connection is a nullptr in IdleConnectionRunnable::Run();

Fix for a medium volume crasher on beta, patch evaluated as low risk, uplift approved for 67 beta 15, thanks.

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/74af12527bdc