1770787 - [QM_TRY] Failures in dom/indexedDB/ActorsParent.cpp:FactoryOp::DirectoryOpen

Status: RESOLVED FIXED

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

Description

[Jan Rio Krause]

This bugs keeps track of the semi-automatic monitoring of QM_TRY failures in dom/indexedDB/ActorsParent.cpp:FactoryOp::DirectoryOpen.

Comment 1

[Jan Rio Krause]

Taken from Attachment 9277802 [details].

Clients	Sessions	Hits	Anchor	Stack
1	1	1	dom/indexedDB/ActorsParent.cpp:FactoryOp::DirectoryOpen	dom/indexedDB/ActorsParent.cpp#15392:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR <- dom/indexedDB/ActorsParent.cpp#15797:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR
1	1	1	dom/indexedDB/ActorsParent.cpp:FactoryOp::DirectoryOpen	dom/indexedDB/ActorsParent.cpp#15392:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR <- dom/indexedDB/ActorsParent.cpp#15797:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR

Comment 2

[Jens Stutte [:jstutte]]

I think these are coming from here.

FactoryOp::DirectoryLockAcquired swallows the error without reporting it to its caller. It seems the consequence is just to not call Run().

It looks as if we could have better (more specific) errors here. The first could better be NS_ERROR_ILLEGAL_DURING_SHUTDOWN ?

And the shutdown check could probably be made also earlier in the chain, like in DirectoryLockImpl::NotifyOpenListener, doing Invalidate() ?

Comment 3

[Jan Varga [:janv]]

We don't know what was the first failure in FactoryOp::SendToIOThread. It can be the shutdown or the actor being destroyed.

It doesn't make sense to report anything to the caller in FactoryOp::DirectoryLockAcquired because that's just a notification. Reporting to the caller would be like propagating errors in Run methods.

Comment 4

[Jan Varga [:janv]]

It would be nice if we could easily see what context was set.

Comment 5

[Jens Stutte [:jstutte]]

(In reply to Jan Varga [:janv] from comment #3)

We don't know what was the first failure in FactoryOp::SendToIOThread. It can be the shutdown or the actor being destroyed.

Yes, although the actor being destroyed is most probably a consequence of shutdown? If not: We could split that if to be even more specific.

It doesn't make sense to report anything to the caller in FactoryOp::DirectoryLockAcquired because that's just a notification. Reporting to the caller would be like propagating errors in Run methods.

OK. Still I assume we should do something on every error, like calling Invalidate() ? (Having in mind the shutdown hangs with DirectoryLocks...)

Comment 6

[Jan Varga [:janv]]

Once a directory lock is acquired, it's up to a quota client to release it. Invalidate is only useful for a pending directory lock when a quota client waits for the lock being acquired. In that case, Invalidate would cause DirectoryLockFailed and it would advance the state machine.

We currently invalidate pending directory locks in the final stage of QuotaManager::Shutdown (after the I/O thread shutdown), we may want to investigate if the invalidation could happen sooner (given that quota clients are now expected to abort existing operations when the shutdown is requested).

Anyway, the reported error stack looks harmless to me, it's just that we added a context for shutdown and now we see some errors. In this case, I believe the error doesn't cause a problem. The only problem is the extra noise which we could try to silence by adding a QM_TRY warning. For example by changing the QM_TRY to QM_WARNONLY_TRY in FactoryOp::DirectoryLockAcquired.

Comment 7

[Jan Rio Krause]

Taken from Attachment 9278772 [details].

Clients	Sessions	Hits	Anchor	Stack
2	2	7	dom/indexedDB/ActorsParent.cpp:FactoryOp::DirectoryOpen	dom/indexedDB/ActorsParent.cpp#15392:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR <- dom/indexedDB/ActorsParent.cpp#15797:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR
1	1	1	dom/indexedDB/ActorsParent.cpp:FactoryOp::DirectoryOpen	dom/indexedDB/ActorsParent.cpp#15392:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR <- dom/indexedDB/ActorsParent.cpp#15797:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR
1	1	1	dom/indexedDB/ActorsParent.cpp:FactoryOp::DirectoryOpen	dom/indexedDB/ActorsParent.cpp#15392:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR <- dom/indexedDB/ActorsParent.cpp#15797:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR
1	1	1	dom/indexedDB/ActorsParent.cpp:FactoryOp::DirectoryOpen	dom/indexedDB/ActorsParent.cpp#15392:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR <- dom/indexedDB/ActorsParent.cpp#15797:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR

Comment 8

[Jan Rio Krause]

Attachment: Bug 1770787 - Replace `QM_TRY` with `QM_WARNONLY_TRY`. r=#dom-storage-reviewers

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b394e712036f
Replace `QM_TRY` with `QM_WARNONLY_TRY`. r=dom-storage-reviewers,janv

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b394e712036f

Comment 11

[Jan Rio Krause]

Taken from Attachment 9279436 [details].

Clients	Sessions	Hits	Anchor (Context)	Stack
1	1	99	dom/indexedDB/ActorsParent.cpp:FactoryOp::DirectoryOpen (dom::quota::QuotaManager::Shutdown)	dom/indexedDB/ActorsParent.cpp#15392:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR <- dom/indexedDB/ActorsParent.cpp#15797:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR
1	1	4	dom/indexedDB/ActorsParent.cpp:FactoryOp::DirectoryOpen (dom::quota::QuotaManager::Shutdown)	dom/indexedDB/ActorsParent.cpp#15392:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR <- dom/indexedDB/ActorsParent.cpp#15797:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR
1	1	1	dom/indexedDB/ActorsParent.cpp:FactoryOp::DirectoryOpen (dom::quota::QuotaManager::Shutdown)	dom/indexedDB/ActorsParent.cpp#15392:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR <- dom/indexedDB/ActorsParent.cpp#15797:NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR