1248550 - Improve idle database maintenance locking

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jan Varga [:janv]]

Idle (daily) database maintenance code doesn't check if there are any Database or FactoryOp objects for the same database and FactoryOp doesn't check if there's a maintenance being performed for the same database.
This can lead to various problems. I have a fix for this currently tested on try.

Comment 1

[Jan Varga [:janv]]

Attachment: fix

Comment 2

[Jan Varga [:janv]]

Attachment: cleanup

This just reorders some methods to match their declaration order. I didn't do that in the main patch for easier reviewing.

Comment 3

[Jan Varga [:janv]]

Comment on attachment 8719740 [details] [diff] [review]
fix

Ben, feel free to comment on this patch.

Before this patch we didn't protect the directory scanning (FindDatabasesForIdleMaintenance) and origin directories were protected by a shared lock only, so database maintenance could interfere with database opening, deleting, transaction operations, etc.

There's now just one shared directory lock for entire <profile>/storage/*/*/idb and database maintenance checks existing FactoryOp and Database objects and on the other hand FactoryOp checks for existing database maintenance and delays itself until the maintenance completes.

Oh, and in theory, a new StartIdleMaintenance() could come while there was one still in progress. That could interfere too.

Hopefully, this patch fixes all the intermittent bugs/crashes/assertions related to idle maintenance and quota stuff.

Comment 4

[Jan Varga [:janv]]

These bugs could be caused by this.

Comment 5

[Jan Varga [:janv]]

And these.

Comment 6

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=586011490547

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8719740 [details] [diff] [review]
fix

Review of attachment 8719740 [details] [diff] [review]:
-----------------------------------------------------------------

I tried, but I cannot review this as is.

Please separate "moving code around" and "behavior changes" into different patches.  Please add comments explaining the states, what they mean, and what the transition rules are between them.  And please explain which threads are involved.  Then I might stand a chance.

Comment 8

[Jan Varga [:janv]]

Attachment: Part 1: Move main idle maintenance logic into a new object Maintenance

This just gets a directory lock for <profile>/storage/*/*/idb and then runs a directory scan on the IO thread producing an array of DirectoryInfo.

Comment 9

[Jan Varga [:janv]]

Attachment: Part 2: Move GetOrCreateThreadPool() to correct place

Just a move, no code changes.

Comment 10

[Jan Varga [:janv]]

Attachment: Part 3: Introduce a new object/runnable for database maintenance

This adds a new object DatabaseMaintenance. The object/runnable is dispatched to the maintenance thread pool for each database collected by Maintenance object.
The database maintenance is not run if there's a live Database object for given database path.

Comment 11

[Jan Varga [:janv]]

Attachment: Part 4: Check FactoryOp objects before running a database maintenance and check for DatabaseMaintenance before dispatching a FactoryOp

Comment 12

[Jan Varga [:janv]]

Attachment: Part 5: Move AutoProgressHandler from QuotaClient to DatabaseMaintenance

No code changes.

Comment 13

[Jan Varga [:janv]]

Attachment: Part 6: Move database maintenance specific methods from QuotaClient to DatabaseMaintenance

No code changes.

Comment 14

[Jan Varga [:janv]]

Attachment: Part 7: Improve the test for idle maintenance to trigger interference between database maintenance and normal database operations

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8722471 [details] [diff] [review]
Part 1: Move main idle maintenance logic into a new object Maintenance

Review of attachment 8722471 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/ActorsParent.cpp
@@ +8931,5 @@
>    FullVacuum(mozIStorageConnection* aConnection,
>               nsIFile* aDatabaseFile);
>  
> +  // Runs on the PBackground thread. Checks to see if there's a queued
> +  // maintanance to run.

"Maintenance" (capital M and spelling)

@@ +9007,5 @@
> +  {
> +    // Just created on the PBackground thread, run immediatelly or added to the
> +    // maintenance queue. The next step is either DirectoryOpenPending if
> +    // IndexedDatabaseManager is running, or CreateIndexedDatabaseManager if
> +    // not.

Newly created on the PBackground thread. Will proceed immediately or be added to the maintenance queue. The next step is either DirectoryOpenPending if IndexedDatabaseManager is running, or CreateIndexedDatabaseManager if not.

@@ +9012,5 @@
> +    Initial = 0,
> +
> +    // Creating IndexedDatabaseManager on the main thread. The next step is
> +    // either Finishing if IndexedDatabaseManager initialization failed, or
> +    // IndexedDatabaseManagerOpen if initialization succeeded.

"Create IndexedDatabaseManager ..." fails/succeeds (present tense)

@@ +9017,5 @@
> +    CreateIndexedDatabaseManager,
> +
> +    // Calling OpenDirectory() on the PBackground thread. The next step is
> +    // DirectoryOpenPending.
> +    IndexedDatabaseManagerOpen,

Call ...

@@ +9026,5 @@
> +    DirectoryOpenPending,
> +
> +    // Waiting to do/doing work on the QuotaManager IO thread. Its next step is
> +    // Finishing.
> +    DirectoryWorkOpen,

The next step

@@ +9036,5 @@
> +    // All done.
> +    Complete
> +  };
> +
> +  nsCOMPtr<nsIEventTarget> mOwningThread;

This is always the PBackground thread.  Why store it as a member variable?

@@ +9090,5 @@
> +
> +    return mAborted;
> +  }
> +
> +  // May be called on any thread, but is more expensive than IsAborted().

This code should not be a fast path.  Just use the atomics everywhere.

@@ +9141,5 @@
> +  OpenDirectory();
> +
> +  // Runs on the PBackground thread. Dispatches to the QuotaManager I/O thread.
> +  nsresult
> +  DirectoryOpen();

Can we name this AfterDirectoryOpen or something?  Having an OpenDirectory and a DirectoryOpen is confusing.

@@ +9153,5 @@
> +  // if any of above methods fails.
> +  void
> +  Finish();
> +
> +  NS_DECL_ISUPPORTS_INHERITED

No reason to override nsISupports, you don't add any interfaces.

@@ +16787,5 @@
>    MOZ_ASSERT(!mShutdownRequested);
>  
> +  RefPtr<Maintenance> maintenance = new Maintenance(this);
> +
> +  mMaintenanceQueue.AppendElement(maintenance);

maintenance.forget()?

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8722473 [details] [diff] [review]
Part 3: Introduce a new object/runnable for database maintenance

Review of attachment 8722473 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/ActorsParent.cpp
@@ +9029,5 @@
> +    // next state is either WaitingForDatabaseMaintenancesToComplete if at least
> +    // one runnable has been dispatched, or Finishing otherwise.
> +    BeginDatabaseMaintenance,
> +
> +    // Waiting for database maintenances to finish on maintenance thread pool.

s/database maintenances/DatabaseMaintenance/

@@ +9046,5 @@
>    RefPtr<QuotaClient> mQuotaClient;
>    PRTime mStartTime;
>    RefPtr<DirectoryLock> mDirectoryLock;
>    nsTArray<DirectoryInfo> mDirectoryInfos;
> +  nsDataHashtable<nsStringHashKey, DatabaseMaintenance*> mDatabaseMaintenances;

You don't actually need a hashtable here, just a counter.  All we ever really use is mDatabaseMaintenances.Count()

@@ +9239,5 @@
>  
> +class DatabaseMaintenance final
> +  : public nsRunnable
> +{
> +  nsCOMPtr<nsIEventTarget> mOwningThread;

Again, why does this need to be a member variable?  It's always the background thread.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8722475 [details] [diff] [review]
Part 4: Check FactoryOp objects before running a database maintenance and check for DatabaseMaintenance before dispatching a FactoryOp

Review of attachment 8722475 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/ActorsParent.cpp
@@ +9148,5 @@
> +  {
> +    AssertIsOnOwningThread();
> +
> +    RefPtr<DatabaseMaintenance> result =
> +      mDatabaseMaintenances.Get(aDatabasePath);

Ah, ok.  Belay the last comment about the hashtable.

@@ +9307,5 @@
>      return mDatabasePath;
>    }
>  
> +  void
> +  WaitForComplete(nsIRunnable* aCallback)

name this WaitForCompletion.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Please give these patches useful commit messages before you land them.

Comment 19

[Jan Varga [:janv]]

Thanks! I'll address your comment and land soon.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Thanks for the patches!

Comment 21

[Jan Varga [:janv]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #15)
> @@ +9153,5 @@
> > +  // if any of above methods fails.
> > +  void
> > +  Finish();
> > +
> > +  NS_DECL_ISUPPORTS_INHERITED
> 
> No reason to override nsISupports, you don't add any interfaces.

Maintenance inherits from nsRunnable and OpenDirectoryListener which both have Addref/Release, so
without the macro I get: "error: member 'AddRef' found in multiple base classes of different types"

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca8e9cc4f661
https://hg.mozilla.org/integration/mozilla-inbound/rev/62808b96eb74
https://hg.mozilla.org/integration/mozilla-inbound/rev/3022115e2a1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4814724f9798
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1bd3e09a8cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/e00fd36ad4ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdd20d022a20
https://hg.mozilla.org/integration/mozilla-inbound/rev/df332ac26b52

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ca8e9cc4f661
https://hg.mozilla.org/mozilla-central/rev/62808b96eb74
https://hg.mozilla.org/mozilla-central/rev/3022115e2a1a
https://hg.mozilla.org/mozilla-central/rev/4814724f9798
https://hg.mozilla.org/mozilla-central/rev/f1bd3e09a8cf
https://hg.mozilla.org/mozilla-central/rev/e00fd36ad4ce
https://hg.mozilla.org/mozilla-central/rev/bdd20d022a20
https://hg.mozilla.org/mozilla-central/rev/df332ac26b52

Comment 24

[Jan Varga [:janv]]

I filed bug 1252409 which is about even better directory locking.