Closed Bug 1248550 Opened 4 years ago Closed 4 years ago

Improve idle database maintenance locking

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: janv, Assigned: janv)

References

(Blocks 4 open bugs)

Details

(Whiteboard: dom-triaged btpp-active)

Attachments

(7 files, 2 obsolete files)

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.
Attached patch fix (obsolete) — Splinter Review
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Attachment #8719740 - Flags: review?(khuey)
Attached patch cleanup (obsolete) — Splinter Review
This just reorders some methods to match their declaration order. I didn't do that in the main patch for easier reviewing.
Attachment #8719742 - Flags: review?(khuey)
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.
Flags: needinfo?(bent.mozilla)
Attachment #8719740 - Flags: feedback?(bent.mozilla)
These bugs could be caused by this.
And these.
Blocks: 1244905
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.
Attachment #8719740 - Flags: review?(khuey) → review-
Whiteboard: dom-triaged btpp-active
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.
Attachment #8719740 - Attachment is obsolete: true
Attachment #8719742 - Attachment is obsolete: true
Attachment #8719740 - Flags: feedback?(bent.mozilla)
Attachment #8719742 - Flags: review?(khuey)
Attachment #8722471 - Flags: review?(khuey)
Just a move, no code changes.
Attachment #8722472 - Flags: review?(khuey)
Attachment #8722471 - Attachment description: Patch 1: Move main idle maintenance logic into a new object Maintenance → Part 1: Move main idle maintenance logic into a new object 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.
Attachment #8722473 - Flags: review?(khuey)
Attachment #8722475 - Flags: review?(khuey)
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()?
Attachment #8722471 - Flags: review?(khuey) → review+
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.
Attachment #8722473 - Flags: review?(khuey) → review+
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.
Attachment #8722475 - Flags: review?(khuey) → review+
Please give these patches useful commit messages before you land them.
Flags: needinfo?(bent.mozilla)
Thanks! I'll address your comment and land soon.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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"
I filed bug 1252409 which is about even better directory locking.
Duplicate of this bug: 1236149
You need to log in before you can comment on or make changes to this bug.