Bugzilla

(In reply to Haik Aftandilian [:haik] from comment #54)
> I wonder if we could add some diagnostic asserts to Nightly to trigger a crash if we ever get SQLITE_BUSY repeatedly assuming that would help debug this further. And should we land a fix to make it impossible to infinite loop here? 

Note getting SQLITE_BUSY in certain cases is absolutely normal, because we have a few connections using multiple threads, we use PRAGMA busy_timeout to cope with that in Places, we should likely do the same in any component that uses both the sync and async API on the same connection.
By code inspection, that's likely just Places, Cookies and tests. Afaict, Places is the only one that has to do real mixed usage, Cookies only do opening/closing on MT, so maybe it's not strictly necessary. Effectively it looks like fixing Cookies and tests, would leave Places as the only mixed use consumer that is a bit more complex to attack.

> I noticed that in `AsyncExecuteStatements::Run()`, we check `mCancelRequested`. If `mCancelRequested` could be set between the time the runnable is dispatched and when it is executed, presumably it could be set after we have checked in ::Run. Should we be checking this while in the SQLITE_BUSY loop or perhaps checking for any other state indicating a shutdown is happening?

Potentially yes, my only doubt is that accessing it requires a lock on mMutex, that may still be ok considered we don't set it that often.
I'm not completely sure why it's not just an atomic, but there's a long discussion in bug 506805, that may be outdated considered it was about PR_AtomicSet and very old Gcc. That discussion ended up replacing the old PR_ATOMIC with the mutext (https://hg.mozilla.org/mozilla-central/rev/097171b5d15f).
I wonder if today we could use std::atomic<bool>, Andrew was part of that original discussion and maybe has an insight.
Otherwise, I think we can just use the mutex.

(In reply to Haik Aftandilian [:haik] from comment #54)
> I wonder if we could add some diagnostic asserts to Nightly to trigger a crash if we ever get SQLITE_BUSY repeatedly assuming that would help debug this further. And should we land a fix to make it impossible to infinite loop here? 

Note getting SQLITE_BUSY in certain cases is absolutely normal, because we have a few connections using multiple threads, we use PRAGMA busy_timeout to cope with that in Places, we should likely do the same in any component that uses both the sync and async API on the same connection.
By code inspection, that's likely just Places, Cookies and tests. Afaict, Places is the only one that has to do real mixed usage, Cookies only do opening/closing on MT, so maybe it's not strictly necessary. Effectively it looks like fixing Cookies and tests, would leave Places as the only mixed use consumer that is a bit more complex to attack.

> I noticed that in `AsyncExecuteStatements::Run()`, we check `mCancelRequested`. If `mCancelRequested` could be set between the time the runnable is dispatched and when it is executed, presumably it could be set after we have checked in ::Run. Should we be checking this while in the SQLITE_BUSY loop or perhaps checking for any other state indicating a shutdown is happening?

Potentially yes, my only doubt is that accessing it requires a lock on mMutex, that may still be ok considered we don't set it that often.
I'm not completely sure why it's not just an atomic, but there's a long discussion in bug 506805, that may be outdated considered it was about PR_AtomicSet and very old Gcc. That discussion ended up replacing the old PR_ATOMIC with the mutex (https://hg.mozilla.org/mozilla-central/rev/097171b5d15f).
I wonder if today we could use std::atomic<bool>, Andrew was part of that original discussion and maybe has an insight.
Otherwise, I think we can just use the mutex.