1726714 - Harmonize QuotaManager shutdown flags

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, task, P3)

Description

[Jens Stutte [:jstutte]]

We currently have three ways of saying the same:

• gShutdown
• QuotaManager::mShutdownStarted
• AppShutdown::GetCurrentShutdownPhase() >= ShutdownPhase::AppShutdownQM

The overall assumption seems to be, that during the parent process lifetime:

• only one QuotaManager object is ever created
• this sole instance is shutdown only during process shutdown during ShutdownPhase::AppShutdownQM

In the following snippet we use two out of three flags within very few lines:

void QuotaManager::Shutdown() {
  AssertIsOnOwningThread();
  MOZ_ASSERT(!mShutdownStarted);

  // Setting this flag prevents the service from being recreated and prevents
  // further storagess from being created.
  if (gShutdown.exchange(true)) {
    NS_ERROR("Shutdown more than once?!");
  }

  StopIdleMaintenance();

  mShutdownStartedAt.init(TimeStamp::NowLoRes());
  mShutdownStarted = true;

A possible explanation for bug 1726711 could be a re-entrance in the shutdown sequence, which we do not prevent here.

We thus want to reduce the number of ways of saying the same thing (QuotaManager is shutting down) and prevent shutdown from being called twice. Note that it is not clear if re-entrance might happen here on the same object or a newly created one after we already successful shut down a former instance (see also patch D123137 which might be just one way of resurrecting/keeping alive a QuotaManager during shutdown).

Comment 1

[Jan Varga [:janv]]

QuotaManager::mShutdownStarted is just an internal detail to catch multiple QuotaManager::Shutdown calls.

gShutdown is never set back to false, so any QuotaManager::GetOrCreate call after gShutdown is set to false will fail.

Comment 2

[Jan Varga [:janv]]

So mShutdownStarted isn't used by quota clients at all, they only use gShutdown via IsShuttingDown which can be called on any thread since the value is atomic and only switched from false to true once.

However, we could do one thing, move these two methods:
https://searchfox.org/mozilla-central/rev/12770bd668c0a6bdaa8eb96ad9507c6febe8d23d/dom/indexedDB/ActorsParent.cpp#4916
https://searchfox.org/mozilla-central/rev/12770bd668c0a6bdaa8eb96ad9507c6febe8d23d/dom/indexedDB/ActorsParent.cpp#4926
to mozilla/dom/Client.h so it will be possible to use them by all clients (some of them actually have the same methods defined independently)

Actually we need to investigate if we still need these methods at all, given the shutdown of quota clients is now parallel.

Comment 3

[Jens Stutte [:jstutte]]

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

QuotaManager::mShutdownStarted is just an internal detail to catch multiple QuotaManager::Shutdown calls.

Yes, and I want to make this more explicit by a rename.

gShutdown is never set back to false, so any QuotaManager::GetOrCreate call after gShutdown is set to false will fail.

I'd like to remove it entirely in favor of AppShutdown::GetCurrentShutdownPhase()

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

Actually we need to investigate if we still need these methods at all, given the shutdown of quota clients is now parallel.

I'll take a look, thanks for the hint!

Comment 4

[Jens Stutte [:jstutte]]

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

However, we could do one thing, move these two methods:
https://searchfox.org/mozilla-central/rev/12770bd668c0a6bdaa8eb96ad9507c6febe8d23d/dom/indexedDB/ActorsParent.cpp#4916
https://searchfox.org/mozilla-central/rev/12770bd668c0a6bdaa8eb96ad9507c6febe8d23d/dom/indexedDB/ActorsParent.cpp#4926
to mozilla/dom/Client.h so it will be possible to use them by all clients (some of them actually have the same methods defined independently)

Hmm, we already use QuotaManager::IsShuttingdown() there:
https://searchfox.org/mozilla-central/rev/12770bd668c0a6bdaa8eb96ad9507c6febe8d23d/dom/cache/Context.cpp#376,387

Actually I am not sure if we need any of those Client methods at all? Are they just helpers to have those thread assertions?

Comment 5

[Jens Stutte [:jstutte]]

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

However, we could do one thing, move these two methods:
https://searchfox.org/mozilla-central/rev/12770bd668c0a6bdaa8eb96ad9507c6febe8d23d/dom/indexedDB/ActorsParent.cpp#4916
https://searchfox.org/mozilla-central/rev/12770bd668c0a6bdaa8eb96ad9507c6febe8d23d/dom/indexedDB/ActorsParent.cpp#4926
to mozilla/dom/Client.h so it will be possible to use them by all clients (some of them actually have the same methods defined independently)

Actually we need to investigate if we still need these methods at all, given the shutdown of quota clients is now parallel.

So the assumption would be that once the QuotaManager is in shutdown, all QuotaClients should consider themselves to be in shutdown, too, and we can just rely on QuotaManager::IsShuttingDown() everywhere.

Comment 6

[Jens Stutte [:jstutte]]

Attachment: Bug 1726714: Remove redundant QuotaManager's IsShuttingDown functions. r?#dom-storage

Comment 7

[Jan Varga [:janv]]

(In reply to Jens Stutte [:jstutte] from comment #5)

Actually we need to investigate if we still need these methods at all, given the shutdown of quota clients is now parallel.

So the assumption would be that once the QuotaManager is in shutdown, all QuotaClients should consider themselves to be in shutdown, too, and we can just rely on QuotaManager::IsShuttingDown() everywhere.

Yes, but before we had parallel shutdown of quota clients, only specific clients could be in shutdown, so clients could check if given client is shutting down for real.
It seems this is not needed anymore, because we switched to parallel shutdown and it can't happen anymore that only particular clients are being shutdown.

Comment 8

[Jens Stutte [:jstutte]]

So the lifecycle issue will be addressed directly by the patch on bug 1726711.
Here we reduce the scope then to cleanup the different shutdown flags.

Comment 9

[Jens Stutte [:jstutte]]

Attachment: Bug 1726714 - Use AppShutdown::IsInOrBeyond to avoid QM shutdown re-entrance; r=#dom-storage-reviewers

Comment 10

[Jens Stutte [:jstutte]]

Attachment: Bug 1726714: Move IsShuttingDownOn(Non)BackgroundThread functions to Client base class. r?#dom-storage-reviewers

Comment 11

[Jens Stutte [:jstutte]]

Attachment: Bug 1726714: Remove shutdown state from quota clients. r?#dom-storage-reviewers

Depends on D140802

Comment 12

[Jens Stutte [:jstutte]]

Attachment: Bug 1726714: Remove redundant QuotaManager::ShutdownStarted. r?#dom-storage-reviewers

Depends on D140805

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02a879db0412
Move IsShuttingDownOn(Non)BackgroundThread functions to Client base class. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/be15460c8677
Remove shutdown state from quota clients. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/b0cd757518a0
Remove redundant QuotaManager::ShutdownStarted. r=dom-storage-reviewers,janv

Comment 14

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/02a879db0412
https://hg.mozilla.org/mozilla-central/rev/be15460c8677
https://hg.mozilla.org/mozilla-central/rev/b0cd757518a0