1682326 - Chrome IndexedDB operations seem to be potentially started after shutdown started

Status: ASSIGNED

(Core :: Storage: Quota Manager, task)

Description

[Simon Giesecke [:sg] [he/him]]

As part of the changes of Bug 1681670, it turned out that it's necessary to call AbortAllOperations on the IndexedDB quota client a second time during after shutdown started, since some Chrome IndexedDB might otherwise be blocking shutdown in certain situations (e.g. as part of the Windows talos other test suite, as seen here: https://treeherder.mozilla.org/jobs?repo=try&revision=6728cf81a93636366877896fdfd63fb0df6e78bd&selectedTaskRun=BzdB_awKSXOVJyY__moUdA.0). Other platforms might be affected as well, and it might be just timing differences that let this show up under Windows more frequently.

As a workaround, a second call to AbortAllOperations was added to the shutdown procedure in QuotaManager::Shutdown, which should be removed again once the root cause has been identified. It shouldn't be possible to start any IndexedDB operations after shutdown started, and there are several checks in place already that ought to ensure this (see https://searchfox.org/mozilla-central/search?q=if+%28NS_WARN_IF%28QuotaClient%3A%3AIsShuttingDown&path=dom%2FindexedDB%2FActorsParent.cpp&case=false&regexp=false), but maybe there's a gap in these checks.

Comment 1

[Jens Stutte [:jstutte]]

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

As part of the changes of Bug 1681670, it turned out that it's necessary to call AbortAllOperations on the IndexedDB quota client a second time during after shutdown started,

I can see only one call to AbortAllOperations which is the here cited "workaround" only for IDB.

But I see we implemented AbortAllOperations for all client types without ever using it apparently.

Besides all the technical details, I would imagine that a "good" shutdown sequence at QuotaManager level would basically read like:

• (*quotaManager->mClients)[type]->AbortAllOperations();
• (*quotaManager->mClients)[type]->InitiateShutdownWorkThreads();
• after 1-5 sec: (*quotaManager->mClients)[type]->ForceKillActors();
• after 45 sec - crash with annotations

under the assumption, that after we set QuotaManager::IsShuttingDown() to return true (which needs to happen before we call AbortAllOperations) we refuse all new operations on all clients.

Assuming that ForceKillActors is meant to kill actors that hold channels to the content process only, we even might want to do this without waiting 5 secs. If this assumption is right, we might also want to find a better name here?

There might be some implicit, internal abort during InitiateShutdownWorkThreads() for some clients right now, but I think it would be good to make this step explicit for all clients.

Still this bug is probably meaningful in the sense of:

It shouldn't be possible to start any IndexedDB operations after shutdown started, and there are several checks in place already that ought to ensure this (see https://searchfox.org/mozilla-central/search?q=if+%28NS_WARN_IF%28QuotaClient%3A%3AIsShuttingDown&path=dom%2FindexedDB%2FActorsParent.cpp&case=false&regexp=false), but maybe there's a gap in these checks.

but I would want to have more clarity on wether we need/want a general AbortAllOperations first and maybe spin-off another bug here.

Comment 2

[Jens Stutte [:jstutte]]

Attachment: Bug 1682326: Refactor QuotaManager::Shutdown for readability. r?#dom-storage-reviewers

Comment 3

[Jens Stutte [:jstutte]]

Attachment: Bug 1682326: Enable abort operations for local storage. r?#dom-storage-reviewers

Depends on D139616

Comment 4

[Jens Stutte [:jstutte]]

Attachment: Bug 1682326: Enable abort operations for cache. r?#dom-storage-reviewers

Depends on D139617

Comment 5

[Jens Stutte [:jstutte]]

Attachment: Bug 1682326: Enable abort operations for all clients. r?#dom-storage-reviewers

Depends on D139618

Comment 6

[Jens Stutte [:jstutte]]

I moved the patches to enable AbortAllOperations on shutdown to bug 1757144.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca05c0655eb8
Refactor QuotaManager::Shutdown for readability. r=dom-storage-reviewers,janv

Comment 8

[Norisz Fay [:noriszfay]]

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

Comment 9

[Jens Stutte [:jstutte]]

For the records: The refactoring here just moved the AbortAllOperations here, we still do not know why we need this.

Comment 10

[Jens Stutte [:jstutte]]

And we should push some more tests without to see if we still need this.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jstutte, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 12

[Jens Stutte [:jstutte]]

No.