Bug 108759 – webdatabase: Introduce back-end database classes and a few small fixes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mark Lam]

This is a sub-task of https://bugs.webkit.org/show_bug.cgi?id=107475.  Breaking out as a step for easier review.

This patch includes:

1. introducing dummy classes for DatabaseBackendContext, DatabaseBackendAsync, and DatabaseBackendSync.
    - Currently they are just sub-classes of DatabaseContext, Database, and DatabaseSync respectively.  Though logically, the code refers to their instances as if there are 2 objects (e.g. 1 for Database, and 1 for DatabaseBackendAsync), in the current patch, they are implemented as 2 references to the same 1 object.
    - The purpose of doing this is so that code can start to be refactored to work with either the front-end object or the back-end object without requiring them to be properly split apart yet.  This allows the refactoring to proceed in piece-meal fashion.

    In general, the guiding principal is that back-end code should make no references to the ScriptExecutionContext.  This has not been achieved yet, but we're heading there.

2. introducing class DatabaseBase to hold client-side common functionality between Database and DatabaseSync.

3. some renames of a few functions to make them easier to read/understand.
    - deletingDatabase() renamed to isDeletingDatabase()
    - deletingOrigin() renamed to isDeletingOrigin()
    - !canCreateDatabase() reworked to be isDeletingDatabaseOrOriginFor()

4. Cleaned up some destruction code in ~DatabaseSync().
    - The DatabaseBackend that it inherits from will take care of calling closeDatabase().  This call is redundant and now removed.

5. Added a few FIXME comments for code that is transient in this refactoring effort.
    - These serve as reminders to remove them because they will be stale once more / all of the refactoring is done.

[Mark Lam]

Created attachment 186243 [details]
The patch.

[Early Warning System Bot]

Comment on attachment 186243 [details]
The patch.

Attachment 186243 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16346306

[Early Warning System Bot]

Comment on attachment 186243 [details]
The patch.

Attachment 186243 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16351202

[WebKit Review Bot]

Comment on attachment 186243 [details]
The patch.

Attachment 186243 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16353192

[Mark Lam]

Created attachment 186245 [details]
one more time after touching up all the other port's build files.

[WebKit Review Bot]

Comment on attachment 186245 [details]
one more time after touching up all the other port's build files.

Attachment 186245 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16351218

[WebKit Review Bot]

Comment on attachment 186245 [details]
one more time after touching up all the other port's build files.

Attachment 186245 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16351219

[Peter Beverloo (cr-android ews)]

Comment on attachment 186245 [details]
one more time after touching up all the other port's build files.

Attachment 186245 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16360160

[Build Bot]

Comment on attachment 186245 [details]
one more time after touching up all the other port's build files.

Attachment 186245 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16352245

[Mark Lam]

Created attachment 186253 [details]
revised with chromium changes.

[Build Bot]

Comment on attachment 186253 [details]
revised with chromium changes.

Attachment 186253 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16358366

[Mark Lam]

Created attachment 186268 [details]
Revised backend classes that are friendlier for chromium.

Chromium did not like the previous trick of simulating backend classes by sub-classing the Database class itself.  Chromium was doing a class vtbl comparison that failed because it was not able to see that DatabaseBackendAsync is actually a sub-class of Database.

However, implementing DatabaseBackendAsync as a sub-class of Database is meant as a short term trick to keep the compiler happy while allowing us to refer to start thinking of the back-end and the front-end as distinct objects.  Because eventually, DatabaseBaackendAsync will not be a sub-class of Database, it was better to refactor the implementation than to make the chromium code do a real class-hierachy check.  The result is a better refactor where Database extends both DatabaseBase (front end base class) and DatabaseBackendAsync (backend class).  This achieves the goal of this patch without actually doing the full refactor of Database.  FYI, eventually, we'll want DatabaseBackendAsync to be a separate object that Database delegates to instead of inheriting from.

[Build Bot]

Comment on attachment 186268 [details]
Revised backend classes that are friendlier for chromium.

Attachment 186268 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16360730

[Mark Lam]

Comment on attachment 186268 [details]
Revised backend classes that are friendlier for chromium.

Ready for a review.  The win ews complaints seem to be unrelated to this patch.

[Build Bot]

Comment on attachment 186268 [details]
Revised backend classes that are friendlier for chromium.

Attachment 186268 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16354806

[Brady Eidson]

Comment on attachment 186268 [details]
Revised backend classes that are friendlier for chromium.

View in context: https://bugs.webkit.org/attachment.cgi?id=186268&action=review

Most of my comments are about comments.

But I also have very real concerns.  Possibly ones that could be resolved with... comments.

> Source/WebCore/Modules/webdatabase/Database.cpp:72
> -Database::Database(PassRefPtr<DatabaseContext> databaseContext, const String& name, const String& expectedVersion, const String& displayName, unsigned long estimatedSize)
> -    : DatabaseBackend(databaseContext, name, expectedVersion, displayName, estimatedSize, AsyncDatabase)
> +Database::Database(ScriptExecutionContext* scriptExecutionContext, PassRefPtr<DatabaseBackendContext> databaseContext,
> +    const String& name, const String& expectedVersion, const String& displayName, unsigned long estimatedSize)
> +    : DatabaseBase(scriptExecutionContext)
> +    , DatabaseBackendAsync(databaseContext, name, expectedVersion, displayName, estimatedSize)

Presumably the DatabaseBackendContext here refers to the same ScriptExecutionContext being passed in.  If that's the case, no need for both arguments.

> Source/WebCore/Modules/webdatabase/Database.cpp:-77
> -    ScriptController::initializeThreading();

This is the type of mysterious change I'd hope to see an explanation of in the ChangeLog.  Why was it needed before, and why is it not needed now?

> Source/WebCore/Modules/webdatabase/Database.h:94
> -    Database(PassRefPtr<DatabaseContext>, const String& name, const String& expectedVersion,
> -             const String& displayName, unsigned long estimatedSize);
> +    Database(ScriptExecutionContext*, PassRefPtr<DatabaseBackendContext>, const String& name,
> +        const String& expectedVersion, const String& displayName, unsigned long estimatedSize);
> +    PassRefPtr<DatabaseBackendAsync> backend();
> +

Same comment about DatabaseBackendContext and ScriptExecutionContext possibly being duplicate parameters.

> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:246
>  DatabaseBackend::~DatabaseBackend()
>  {
> -    ASSERT(!m_opened);
> +    if (opened())
> +        closeDatabase();
>  }

In WebCore we try to avoid doing much work in destructors.  The ASSERT here was to help assure the heavyweight work had already been completed.

This change moves the heavyweight work into the destructor, which might be problematic.

Why is this necessary now when it wasn't before?

> Source/WebCore/Modules/webdatabase/DatabaseContext.cpp:162
> +PassRefPtr<DatabaseBackendContext> DatabaseContext::backend()
> +{
> +    ref();
> +    DatabaseBackendContext* backend;
> +    backend = reinterpret_cast<DatabaseBackendContext*>(this);
> +    RefPtr<DatabaseBackendContext> backendRef = adoptRef(backend);
> +    return backendRef.release();
> +}

The ref() is super mysterious here.  Why is it necessary?  Can lifetime not be managed another way?

Any time we do manual ref()/deref()s deserves and explanation.

> Source/WebCore/Modules/webdatabase/DatabaseSync.h:79
> +    DatabaseSync(ScriptExecutionContext*, PassRefPtr<DatabaseBackendContext>, const String& name,
> +        const String& expectedVersion, const String& displayName, unsigned long estimatedSize);
> +    PassRefPtr<DatabaseBackendSync> backend();

Same comment about duplicating DatabaseBackendContext and ScriptExecutionContext

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:111
> -        return;
> +        return; // Already open. Nothing more to do.

We don't normally put comments on the same line as code.

It is perfectly acceptable - and expected - to add a {...} block to conditionals like this to contain the comment on its own line.

That said, I don't think this comment is necessary at all, as it's more of a "WHAT this code does" comment instead of a "WHY this code does it" comment.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:124
> +    // Open the database unconditionally. If one does not exists already, open
> +    // a new one.

Not really convinced this comment needs to be here.  I think the paragraphing was enough to make this code easily readable.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:132
> +
> +    // Ensure that the 'Origins' table exists. Create it if it doesn't exist yet.

same here.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:140
> +
> +    // Ensure that the 'Databases' table exists. Create it if it doesn't exist yet.

same here.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:214
>      if (!m_database.isOpen())
> -        return false;
> +        return false; // No "tracker database". Hence, no entry for the database of interest.

This comment feels useful to me.  But it needs to be on its own line, and therefore the if() needs a {...} block to contain the two lines.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:375
> -    openTrackerDatabase(false);
> +    openTrackerDatabase(false); // Don't create one if it doesn't already exist.

If this comment is necessary, it needs to be on its own line.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:380
> +    // Setup an SQL query to fetch the databases for the specified origin:
>      SQLiteStatement statement(m_database, "SELECT name FROM Databases where origin=?;");

Don't think this one is needed.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:390
> +    // Execute the query and fetch each relevant db into the result:
>      int result;
>      while ((result = statement.step()) == SQLResultRow)
>          resultVector.append(statement.getColumnText(0));

Ditto.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:634
>      if (!originQuotaManager().tracksOrigin(origin))
> -        return 0;
> +        return 0; // Still no db's for this origin. Hence, total usage is 0.

Not convinced this one is needed.  If it is, it needs its own line.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:637
> +    // Compute the total disk usage of all db's from the specified origin:
>      return originQuotaManager().diskUsage(origin);

If this is just saying what diskUsage() does, it's not necessary.

If the intention is to point out that diskUsage involves computation and is not simply an accessor, I think it needs to more explicitly state that.

[Mark Lam]

Comment on attachment 186268 [details]
Revised backend classes that are friendlier for chromium.

View in context: https://bugs.webkit.org/attachment.cgi?id=186268&action=review

>> Source/WebCore/Modules/webdatabase/Database.cpp:72
>> +    , DatabaseBackendAsync(databaseContext, name, expectedVersion, displayName, estimatedSize)
> 
> Presumably the DatabaseBackendContext here refers to the same ScriptExecutionContext being passed in.  If that's the case, no need for both arguments.

Yes, currently, they refer to the same ScriptExecutionContext.  I expressed it this way to hint at forward looking changes where Database is instantiated separately from its corresponding DatabaseBackend.  But since those changes are not materialized yet, I'm fine with reducing this to just pass the DatabaseBackendContext and make use of the existing temporary hack to fetch the ScriptExecutionContext from that.

>> Source/WebCore/Modules/webdatabase/Database.cpp:-77
>> -    ScriptController::initializeThreading();
> 
> This is the type of mysterious change I'd hope to see an explanation of in the ChangeLog.  Why was it needed before, and why is it not needed now?

Previously, all we cared about is to make sure that we call ScriptController::initializeThreading() on first use of Database code.  Since the JS interface is set up that we need a Database object before we can do database work, we just did this thread initialization in the Database object even though it has nothing to do with a Database object specifically.  Now, we have added a DatabaseManager.  I moved this thread initialization call there to DatabaseManager::openDatabase() (which is the same Database instantiation code path).  My thinking is: let the manager initialize the system (e.g. like this thread initialization), and let the object that it manages (i.e. the Database) assume that its manager has setup the environment properly for it to operate in.

I will add a comment in the ChangeLog to explain this.

>> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:246
>>  }
> 
> In WebCore we try to avoid doing much work in destructors.  The ASSERT here was to help assure the heavyweight work had already been completed.
> 
> This change moves the heavyweight work into the destructor, which might be problematic.
> 
> Why is this necessary now when it wasn't before?

This code is defensive.  Plus DatabaseSync was calling closeDatabase() in its destructor anyway.  I simply consolidated it here so that we get common behavior instead of special casing it for DatabaseSync alone, and we get the code being more defensive against leaks as a bonus.

>> Source/WebCore/Modules/webdatabase/DatabaseContext.cpp:162
>> +}
> 
> The ref() is super mysterious here.  Why is it necessary?  Can lifetime not be managed another way?
> 
> Any time we do manual ref()/deref()s deserves and explanation.

I could have expressed it better (now that I'm a little more familiar with how refs work).  How about it I re-write it as this?

PassRefPtr<DatabaseBackendContext> DatabaseContext::backend()
{
    DatabaseBackendContext* backend = static_cast<DatabaseBackendContext*>(this);
    return backend;
}

>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:111
>> +        return; // Already open. Nothing more to do.
> 
> We don't normally put comments on the same line as code.
> 
> It is perfectly acceptable - and expected - to add a {...} block to conditionals like this to contain the comment on its own line.
> 
> That said, I don't think this comment is necessary at all, as it's more of a "WHAT this code does" comment instead of a "WHY this code does it" comment.

I'm fine with dropping this comment.

>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:124
>> +    // a new one.
> 
> Not really convinced this comment needs to be here.  I think the paragraphing was enough to make this code easily readable.

I think of it as re-enforcing the comment on the previous comment block above.  Having it here allows me to start reading from this point on and understanding how things work without having to parse and grok the complicated behavior of SQLiteFileSystem::ensureDatabaseFileExists() and further inferring the fall out of that from a variable createIfDoesNotExist flag.  Yes, one can figure things out without this comment, but it requires a bit more bandwidth.  If you strongly object to the presence of this comment, I can delete it.

>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:132
>> +    // Ensure that the 'Origins' table exists. Create it if it doesn't exist yet.
> 
> same here.

I like section headers that tells me what the code is trying to achieve so that I can quickly tell what is happening without groking the code.  It's less bandwidth to get the intent from "Create it if it doesn't exist yet" then from "m_database.executeCommand("CREATE TABLE Origins (origin TEXT UNIQUE ON CONFLICT REPLACE, quota INTEGER NOT NULL ON CONFLICT FAIL);")".  Note that there is also an if statement involve to handle the case where the "creation origin" fails.  The comment allows me to bypass that detail quickly while I need to grok that from the code (though I agree that it will be only a minor effort).

Again, it's a minor convenience for the reader but I can delete it if you strongly object.

>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:140
>> +    // Ensure that the 'Databases' table exists. Create it if it doesn't exist yet.
> 
> same here.

Ditto.  Same reason as above.

>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:214
>> +        return false; // No "tracker database". Hence, no entry for the database of interest.
> 
> This comment feels useful to me.  But it needs to be on its own line, and therefore the if() needs a {...} block to contain the two lines.

Will fix.

>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:375
>> +    openTrackerDatabase(false); // Don't create one if it doesn't already exist.
> 
> If this comment is necessary, it needs to be on its own line.

It's necessary because "false" here doesn't tell you much.  Will fix the line issue.

>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:637
>>      return originQuotaManager().diskUsage(origin);
> 
> If this is just saying what diskUsage() does, it's not necessary.
> 
> If the intention is to point out that diskUsage involves computation and is not simply an accessor, I think it needs to more explicitly state that.

It's to point out that this code may be heavy weight and will be scanning the disk and not just fetching a cached value.  I agree that I should be more verbose and state that explicitly.

[Michael Nordman]

Comment on attachment 186268 [details]
Revised backend classes that are friendlier for chromium.

View in context: https://bugs.webkit.org/attachment.cgi?id=186268&action=review

>>> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:246
>>>  }
>> 
>> In WebCore we try to avoid doing much work in destructors.  The ASSERT here was to help assure the heavyweight work had already been completed.
>> 
>> This change moves the heavyweight work into the destructor, which might be problematic.
>> 
>> Why is this necessary now when it wasn't before?
> 
> This code is defensive.  Plus DatabaseSync was calling closeDatabase() in its destructor anyway.  I simply consolidated it here so that we get common behavior instead of special casing it for DatabaseSync alone, and we get the code being more defensive against leaks as a bonus.

With DatabaseSync, the class is effectively single threaded. But with Database, refs are held and released on multiple threads which means the final release could happen from an unexpected thread. I think the ASSERT here was to ensure, in the Database case, that the database file is actually closed on the background thread instead of potentially the main ui thread.

[Brady Eidson]

(In reply to comment #17)

> >> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:375
> >> +    openTrackerDatabase(false); // Don't create one if it doesn't already exist.
> > 
> > If this comment is necessary, it needs to be on its own line.
> 
> It's necessary because "false" here doesn't tell you much.  Will fix the line issue.

The "false" is leftover from days where we will let people get away with adding bool arguments for random things.  I'm probably personally responsible for this one originally.

These days, we aren't afraid to add a two-state enum that is descriptive for what's going on.  That makes it impossible to accidentally misuse, makes the code self-documenting, and removes the need for comments like these.

[Mark Lam]

Comment on attachment 186268 [details]
Revised backend classes that are friendlier for chromium.

View in context: https://bugs.webkit.org/attachment.cgi?id=186268&action=review

>>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:375
>>> +    openTrackerDatabase(false); // Don't create one if it doesn't already exist.
>> 
>> If this comment is necessary, it needs to be on its own line.
> 
> It's necessary because "false" here doesn't tell you much.  Will fix the line issue.

Added a TrackerCreationAction { DontCreateIfDoesNotExist, CreateIfDoesNotExist } enum to use here and replaced all calls to openTrackerDatabase() to use it.

[Mark Lam]

Created attachment 186531 [details]
Addressed Brady's comments.

[Mark Lam]

(In reply to comment #21)
> Created an attachment (id=186531) [details]
> Addressed Brady

Fudge fingers.  Hit the enter key accidentally.   Meant to say "Addressed Brady's comments."

[Brady Eidson]

(In reply to comment #17)

> >> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:246
> >>  }
> > 
> > In WebCore we try to avoid doing much work in destructors.  The ASSERT here was to help assure the heavyweight work had already been completed.
> > 
> > This change moves the heavyweight work into the destructor, which might be problematic.
> > 
> > Why is this necessary now when it wasn't before?
> 
> This code is defensive.  Plus DatabaseSync was calling closeDatabase() in its destructor anyway.  I simply consolidated it here so that we get common behavior instead of special casing it for DatabaseSync alone, and we get the code being more defensive against leaks as a bonus.
> 

Except there's a very real difference between Async and Synchronous databases that calls for this behavior to *not* be consolidated...

(In reply to comment #18)
> With DatabaseSync, the class is effectively single threaded. But with Database, refs are held and released on multiple threads which means the final release could happen from an unexpected thread. I think the ASSERT here was to ensure, in the Database case, that the database file is actually closed on the background thread instead of potentially the main ui thread.

By ditching the ASSERT in ~DatabaseBackend we're losing the ability to see a threading error like this creep in.

[Brady Eidson]

> (In reply to comment #18)
> By ditching the ASSERT in ~DatabaseBackend we're losing the ability to see a threading error like this creep in.

Since you're new to working in this area, I'll elaborate on why this is important.

SQLite is "multi-thread safe", but each database handle can only be used on a single thread at a time.

The decision a long time ago in WebCore was to make sure we open a SQLite database handle on the background thread where it is going to be used, then close it on that same background thread once we're done with it.

It is important we continue to be able to make sure we don't close it on the main thread, as it still may be used on a background thread.  That's why closing it is an explicit background task.

[Mark Lam]

(In reply to comment #24)
> > (In reply to comment #18)
> > By ditching the ASSERT in ~DatabaseBackend we're losing the ability to see a threading error like this creep in.
> 
> Since you're new to working in this area, I'll elaborate on why this is important.
> 
> SQLite is "multi-thread safe", but each database handle can only be used on a single thread at a time.
> 
> The decision a long time ago in WebCore was to make sure we open a SQLite database handle on the background thread where it is going to be used, then close it on that same background thread once we're done with it.
> 
> It is important we continue to be able to make sure we don't close it on the main thread, as it still may be used on a background thread.  That's why closing it is an explicit background task.

Thanks.  I'll "revert" the change by putting the assert back into DatabaseBackend, and moving the DatabaseSync call to close to DatabaseBackendSync.

[Mark Lam]

Created attachment 186659 [details]
One more time with ~DatabaseBackend() assert put back in, and ~DatabaseBackendSync() closing its own database.

[Brady Eidson]

Comment on attachment 186659 [details]
One more time with ~DatabaseBackend() assert put back in, and ~DatabaseBackendSync() closing its own database.

View in context: https://bugs.webkit.org/attachment.cgi?id=186659&action=review

> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:251
> +    // SQLite is "multi-thread safe", but each database handle can only be used
> +    // on a single thread at a time.
> +    //
> +    // For DatabaseBackendAsync, we open the SQLite database on the DatabaseThread,
> +    // and hence we should also close it on that same thread. This means that the
> +    // SQLite database need to be closed by another mechanism (see
> +    // DatabaseContext::stopDatabases()). By the time we get here, the SQLite
> +    // database should have already been closed.

Good comment, thanks!

[Mark Lam]

Thanks for the review.  Landed in r141928: <http://trac.webkit.org/changeset/141928>.