Closed Bug 1690310 Opened 4 years ago Closed 2 years ago

Hit MOZ_CRASH(CachingDatabaseConnection not thread-safe) at src/xpcom/base/nsISupportsImpl.cpp:43

Categories

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

defect

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox87 --- wontfix
firefox88 --- wontfix
firefox89 --- wontfix
firefox90 --- wontfix
firefox91 --- affected

People

(Reporter: tsmith, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-moderate)

This was found while trying to collect a rr trace for another issue. The test case that was used is unreduced and is very unreliable. I will attach a Pernosco session shortly.

Hit MOZ_CRASH(CachingDatabaseConnection not thread-safe) at src/xpcom/base/nsISupportsImpl.cpp:43

#0 0x3ae862ff52c6 in MOZ_Crash(char const*, int, char const*) src/objdir-ff-debug/dist/include/mozilla/Assertions.h:254:3
#1 0x3ae862ff52c6 in nsAutoOwningThread::AssertCurrentThreadOwnsMe(char const*) const src/xpcom/base/nsISupportsImpl.cpp:43:5
#2 0x3ae8630cd66c in void nsAutoOwningThread::AssertOwnership<42>(char const (&) [42]) const src/objdir-ff-debug/dist/include/nsISupportsImpl.h:60:5
#3 0x3ae869bee253 in mozilla::dom::quota::CachingDatabaseConnection::AssertIsOnConnectionThread() const src/objdir-ff-debug/dist/include/mozilla/dom/quota/CachingDatabaseConnection.h:67:20
#4 0x3ae869f0e4d0 in mozilla::dom::indexedDB::(anonymous namespace)::Database::AssertIsOnConnectionThread() const src/dom/indexedDB/ActorsParent.cpp:2232:20
#5 0x3ae869f0b76c in mozilla::dom::indexedDB::(anonymous namespace)::TransactionBase::AssertIsOnConnectionThread() const src/dom/indexedDB/ActorsParent.cpp:2643:16
#6 0x3ae869f0a99b in mozilla::dom::indexedDB::(anonymous namespace)::TransactionBase::CommitOp::Run() src/dom/indexedDB/ActorsParent.cpp:17803:17
#7 0x3ae869f27320 in mozilla::dom::indexedDB::(anonymous namespace)::ConnectionPool::FinishCallbackWrapper::Run() src/dom/indexedDB/ActorsParent.cpp:8805:26
#8 0x3ae8631bdc13 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1165:16
#9 0x3ae8631c41a6 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:548:10
#10 0x3ae869f1f052 in bool mozilla::SpinEventLoopUntil<(mozilla::ProcessFailureBehavior)1, mozilla::dom::indexedDB::(anonymous namespace)::ConnectionPool::ThreadRunnable::Run()::$_69>(mozilla::dom::indexedDB::(anonymous namespace)::ConnectionPool::ThreadRunnable::Run()::$_69&&, nsIThread*) src/objdir-ff-debug/dist/include/mozilla/SpinEventLoopUntil.h:93:25
#11 0x3ae869f1ed46 in mozilla::dom::indexedDB::(anonymous namespace)::ConnectionPool::ThreadRunnable::Run() src/dom/indexedDB/ActorsParent.cpp:8878:25
#12 0x3ae8631bdc13 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1165:16
#13 0x3ae8631c41a6 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:548:10
#14 0x3ae8643a356f in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:302:20
#15 0x3ae864227196 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:335:10
#16 0x3ae864227114 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:328:3
#17 0x3ae8642270d2 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:310:3
#18 0x3ae8631b9c74 in nsThread::ThreadFunc(void*) src/xpcom/threads/nsThread.cpp:404:10
#19 0x5870883444 in _pt_root src/nsprpub/pr/src/pthreads/ptthread.c:201:5
#20 0x520d09abf6da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
#21 0x78cb2e519a3e in clone /build/glibc-2ORdQG/glibc-2.27/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

A Pernosco session is available here: https://pernos.co/debug/umZip277AMukhG9-uFvZtQ/index.html

The caching database connection has been introduced in bug 1685111 for localstorage, it seems. But it might just be a case that it happened here with this kind of connection and the root cause is elsewhere, Simon?

Flags: needinfo?(sgiesecke)

The CachingDatabaseConnection class was extracted from the IndexedDB code in Bug 1685111. It existed before but was slightly modified. I will check if this is a regression from Bug 1685111 or a preexisting issue.

I gave the Pernosco session a look, and what strikes me here is that the storage connection has been closed before the assertion fails (CachingDatabaseConnection::mStorageConnection is nullptr). I wonder if Database::AssertIsOnConnectionThread (https://searchfox.org/mozilla-central/rev/b2433a832c250c55255e0ee37d05192d04f20427/dom/indexedDB/ActorsParent.cpp#2228-2239) needs to take account of that in the condition. In that case, this were only a bad debug assertion.

Jan, what do you think? I added some notes to the Pernosco notebook as well.

Flags: needinfo?(sgiesecke) → needinfo?(jvarga)

(In reply to Simon Giesecke [:sg] [he/him] from comment #3)

The CachingDatabaseConnection class was extracted from the IndexedDB code in Bug 1685111. It existed before but was slightly modified. I will check if this is a regression from Bug 1685111 or a preexisting issue.

What was modified ?

Sorry for not providing more details. Let me give that a try:

While the first patch did only extract the base class. In the second patch, actually the handling of nsAutoOwningThread was modified, because the localstorage client needs to initialize it lazily, while the existing IndexedDB client always initializes it on construction. So it now allows both ways. Instead of declaring NS_DECL_OWNINGTHREAD, it now has a LazyInitializedOnce<const nsAutoOwningThread> mOwningThread; member that's either initialized on construction or in LazyInit. At first sight, this looks as if it might be related to the problem, but at second sight I couldn't find any actual hint on this. And this seems to be a rare edge case only happening during fuzzing, so it's at least plausible that it existed the same way before Bug 1685111. Obviously, I can't definitely rule out it was regressed by Bug 1685111.

Severity: -- → S3
Priority: -- → P3

Hi Tyson! Is this still reproducing? In case: would it be possible to update the pernosco session on a current central? Thank you!

Flags: needinfo?(jvarga) → needinfo?(twsmith)

The fuzzers are not reporting this issue. In fact I don't see any results so this must have gone away a while ago.

Flags: needinfo?(twsmith) → needinfo?(jstutte)

Then we can probably just close it without further action.

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(jstutte)
Resolution: --- → WORKSFORME
Group: dom-core-security
You need to log in before you can comment on or make changes to this bug.