Bug 108431 – webdatabase: Clean up calls to DatabaseTracker::add/removeOpenDatabase()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mark Lam]

In the existing code, DatabaseTracker::add/removeOpenDatabase() are called from many different places.  We can clean this up so that addOpenDatabase() is only called when we successfully open a database, and removeOpenDatabase() when we close the database.

Previously, the code would call DatabaseTracker::recordCreatingDatabase() in DatabaseTracker::canEstablishDatabase() (called from DatabaseManager::openDatabase()) and call the corresponding DatabaseTracker::doneCreatingDatabase() in DatabaseTracker::addOpenDatabase() (called from the DatabaseBackend constructor).  After the DatabaseBackend constructor is called, we may find that we fail to open the backing SQL database.  In that case, there was nothing more to do for cleaning up the "CreatingDatabase" state, but to call removeOpenDatabase() to undo the effects of addOpenDatabase(). 

Now that we only call addOpenDatabase() if successfully open the database (not just create its interface object in memory), we may fail to open the database and addOpenDatabase() will not be called.  As a result, doneCreatingDatabase() will also not be called though we had called recordCreatingDatabase() earlier.  The clean up the "CreatingDatabase" state, we need call doneCreatingdatabase() in  DatabaseBackend::performOpenAndVerify() if it should fail to open the backing SQL database.

Note: the old code can call DatabaseTracker::removeOpenDatabase() directly from DatabaseManager::openDatabase().  We don't want that, nor do we want to call DatabaseTracker::doneCreatingDatabase() from there though it seems like a convenient place to do so.  The reason for this is because DatabaseTracker is a back-end data structure which eventually belongs in the DatabaseProcess while the DatabaseManager is a client-side / front-end data structure which belongs in the WebProcess.

Hence, this patch removes one instance of the front-end code from touching the back-end DatabaseTracker.

[Mark Lam]

Created attachment 185695 [details]
The fix.

[David Levin]

Just happened to look, you don't need to fix this at all, but doesn't it strike you as unfortunate that nearly the same code is repeated before every return false in DatabaseBackend::performOpenAndVerify?

[Mark Lam]

(In reply to comment #2)
> Just happened to look, you don't need to fix this at all, but doesn't it strike you as unfortunate that nearly the same code is repeated before every return false in DatabaseBackend::performOpenAndVerify?

Yes, I don't like it.  I failed in my previous attempt to make it cleaner, but I'll see if I give it another look to se what I can do to fix that.

[WebKit Review Bot]

Comment on attachment 185695 [details]
The fix.

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

[Mark Lam]

Comment on attachment 185695 [details]
The fix.

Need to fix the chromium build failure.  Will upload another patch.

[David Levin]

(In reply to comment #3)
> (In reply to comment #2)
> > Just happened to look, you don't need to fix this at all, but doesn't it strike you as unfortunate that nearly the same code is repeated before every return false in DatabaseBackend::performOpenAndVerify?
> 
> Yes, I don't like it.  I failed in my previous attempt to make it cleaner, but I'll see if I give it another look to se what I can do to fix that.

Perhaps a common function would be better than repeating the same 5 lines?

[Mark Lam]

Created attachment 185719 [details]
a better patch.

Fixed chromium build failure.  Also used RAII pattern to call DatabaseTracker::doneCreatingDatabase() on exit.  To do this, I also need to remove the call to doneCreatingDatabase() from addOpenDatabase(), which actually makes it cleaner.  Please review.  Thanks.

[Geoffrey Garen]

Comment on attachment 185719 [details]
a better patch.

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

r=me

> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:60
> +// "opened" so that the client can call interrupt or delete every database

Typo: Should be "on every".

[Michael Nordman]

Geez... DatabaseManager is new to me, ditto DatbaseBackend. I have to look at some prior patches to understand the method to the madness:  107535 107784 108275 108278 108279 108431.

Looking at just this snippet and the build artifacts really doesn't tell me much. Do you have an umbrella bug that points at the related set of changes? I see this one (107475), buts its real new and i see a bunch of related changes that happened in december? I'll look at the svn revision history for the Modules/webdatabase directory to make sense out of it, but a quick overview from you might help.

[Mark Lam]

(In reply to comment #9)
> Geez... DatabaseManager is new to me, ditto DatbaseBackend. I have to look at some prior patches to understand the method to the madness:  107535 107784 108275 108278 108279 108431.
> 
> Looking at just this snippet and the build artifacts really doesn't tell me much. Do you have an umbrella bug that points at the related set of changes? I see this one (107475), buts its real new and i see a bunch of related changes that happened in december? I'll look at the svn revision history for the Modules/webdatabase directory to make sense out of it, but a quick overview from you might help.

Here are the changes so far in a nutshell:

1. DatabaseManager is a wrapper around DatabaseTracker (mostly).  It is meant to be front-end Manager handling script side database activity (e.g. callbacks) eventually, while the Tracker handles the tracking of SQL db instances like before in the back-end.

2. DatabaseBackend is a renamed AbstractDatabase (for now).

[Michael Nordman]

Sounds like this patch may change the order of somethings that matter for the chromium port. In chromium the interface between renderers and our main process looks like this (modulo some stuff around the edges about permissions and quotas).


  // WebDatabaseObserver implementation, these are called on the script
  // execution context thread on which the database is opened. This may be
  // the main thread or background WebWorker threads.
  virtual void databaseOpened(const WebKit::WebDatabase& database);
  virtual void databaseModified(const WebKit::WebDatabase& database);
  virtual void databaseClosed(const WebKit::WebDatabase& database);

  // SQLite VFS related methods, these are called on webcore's
  // background database threads via the WebKitPlatformSupport impl.
  base::PlatformFile OpenFile(const string16& vfs_file_name, int desired_flags);
  int DeleteFile(const string16& vfs_file_name, bool sync_dir);
  uint32 GetFileAttributes(const string16& vfs_file_name);
  int64 GetFileSize(const string16& vfs_file_name);
  int64 GetSpaceAvailable(const string16& origin_identifier);


The opened/closed methods in that first block relate to what WebCore::DatabaseTracker sees. The second block  are lower level hooks that we use to satisfy sqlite's need to poke at files.

I think in your changes the ordering of databaseOpened() vs OpenFile(vfs_file_name) may have been reversed? That would matter for chrome because until that databaseOpened() call has happened there is no way to compute an actual OS filepath (vfs_file_name is not a real path). We assign filepaths in databaseOpened() and store the value in our tracker's database at that point.

[Mark Lam]

Created attachment 185935 [details]
the patch plus fixing up the chromium port to work with this change.

Thanks to Michael for pointing out what the chromium port expects in terms of when it needs databaseOpened() and databaseClosed() to be called.  I introduced (for the Chromium port only) DatabaseTracker::prepareToOpenDatabase() and DatabaseTracker::failedToOpenDatabase() which takes care of calling databaseOpened() and databaseClosed() at the right time.  May I have a review from the Chromium folks on these port specific changes please?

[Mark Lam]

(In reply to comment #12)
> Thanks to Michael for pointing out what the chromium port expects in terms of when it needs databaseOpened() and databaseClosed() to be called.  I introduced (for the Chromium port only) DatabaseTracker::prepareToOpenDatabase() and DatabaseTracker::failedToOpenDatabase() which takes care of calling databaseOpened() and databaseClosed() at the right time.  May I have a review from the Chromium folks on these port specific changes please?

Just to give some additional details on what I did for the Chromium port:

1. Michael said that DatabaseObserver::databaseOpened() needs to be called before we attempt to open the actual SQL database.
    - To achieve this, I have Database::openAndVerifyVersion() call DatabaseTracker::tracker().prepareToOpenDatabase().
    - I also have DatabaseBackend::performOpenAndVerify() call DatabaseTracker::tracker().prepareToOpenDatabase().
    - DatabaseTracker::tracker().prepareToOpenDatabase() will check the m_isPreparedToOpenDatabase flag in the database.  If it is not set, prepareToOpenDatabase() will call DatabaseObserver::databaseOpened() and set the flag.  Else, it will consider the database "prepared" and do nothing.
    - This achieves the guarantee that DatabaseObserver::databaseOpened() is called exactly once before DatabaseBackend::performOpenAndVerify() actually opens the SQL database, but I don't hijack piggy back on DatabaseTracker::addOpenDatabase() (as before) to do it.

2. Previously, the client side code (i.e. DatabaseManager) will need to detect when we fail to open the SQL database, and call DatabaseTracker::removeOpenDatabase().  This is asymmetrical with who called addOpenDatabase().  One side-effect of calling removeOpenDatabase() here is that it will also call DatabaseObserver::databaseClosed().
    - In the new code, DatabaseBackend::performOpenAndVerify() who is responsible for opening the SQL database will know when it failed to do so.  If it failed, it will clean up after itself by using the DoneCreatingDatabaseOnExitCaller RAII object to call DatabaseTracker::failedToOpenDatabase().
    - DatabaseTracker::failedToOpenDatabase() will call DatabaseObserver::databaseClosed() if we're running on the script thread.  Else, it will post a task to run on the script thread that will call databaseClosed() later.

With that I have satisfied the expectations of the DatabaseObserver without compromising my goal of not having the front end / script side code call add/removeOpenDatabase().

FWIW, I've also run a subset of the layout tests (those relevant to webdatabase work) on a chromium build.  They used to fail before this change.  They now all pass.

[Mark Lam]

Created attachment 185984 [details]
test patch to test the baseline on the EWS.  This patch does nothing and is not for review.

[Michael Nordman]

Comment on attachment 185935 [details]
the patch plus fixing up the chromium port to work with this change.

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

> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:314
> +    DatabaseTracker::tracker().prepareToOpenDatabase(this);

Thnx for not breaking the chromium port!

Its a little unfortunate that the callsites to DatabaseTracker::prepareToOpenDatabase for the DatabaseSync vs Database classes are different and we have the somewhat gunky isPrepared flag to notice the difference?

Would it make sense to move this call to prepareToOpenDatabase() to an override in DatabaseSync.

bool DatabaseSync::performOpenAndVerify(...) {
#if CHROMIUM
  DatabaseTracker::tracker().prepareToOpenDatabase(this);
#endif
  return DatabseBackend::performOpenAndVerify(...);
}

... and to remove the isPrepared flag/getter/setter. So the call to prepareToOpenDatabase() for the Database case happens in Database.cpp and for DatabaseSync in DatabaseSync.cpp.

[Mark Lam]

(In reply to comment #15)
> Its a little unfortunate that the callsites to DatabaseTracker::prepareToOpenDatabase for the DatabaseSync vs Database classes are different and we have the somewhat gunky isPrepared flag to notice the difference?
> 
> Would it make sense to move this call to prepareToOpenDatabase() to an override in DatabaseSync.
> 
> bool DatabaseSync::performOpenAndVerify(...) {
> #if CHROMIUM
>   DatabaseTracker::tracker().prepareToOpenDatabase(this);
> #endif
>   return DatabseBackend::performOpenAndVerify(...);
> }
> 
> ... and to remove the isPrepared flag/getter/setter. So the call to prepareToOpenDatabase() for the Database case happens in Database.cpp and for DatabaseSync in DatabaseSync.cpp.

Thanks.  That is a good idea.  But instead of overriding performOpenAndVerify(), I'll add an openAndVerifyVersion() function.  That makes it more consistent with how Database handles opening database and hides what we do in the backend to make that happen i.e. call performOpenAndVerify().  Will apply this change, retest, and commit if I don't see new layout test failures.

[David Levin]

Comment on attachment 185935 [details]
the patch plus fixing up the chromium port to work with this change.

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

r=me 

esp with the attempt to address Michael's feedback that you mentioned.

> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:297
> +#endif

Maybe the if could go away here if ExceptionCode were passed in? (Or if that is a layering violation and it may be... then "m_ec == INVALID_STATE_ERR" converted to an enum and passed into doneCreatingDatabase which could call "DatabaseTracker::tracker().failedToOpenDatabase" when appropriate in the chromium impl.


Just an idea.

[Mark Lam]

Comment on attachment 185935 [details]
the patch plus fixing up the chromium port to work with this change.

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

>> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:297
>> +#endif
> 
> Maybe the if could go away here if ExceptionCode were passed in? (Or if that is a layering violation and it may be... then "m_ec == INVALID_STATE_ERR" converted to an enum and passed into doneCreatingDatabase which could call "DatabaseTracker::tracker().failedToOpenDatabase" when appropriate in the chromium impl.
> 
> 
> Just an idea.

Thanks, but I think it is cleaner as it is right now.  No need to pass the ec down to the tracker.  The tracker just does what we tell it, and is not conditional on a higher level situation like a failure to open a database.  This is another reason I like Michael's idea to not have to check for prepared-ness in the DatabaseTracker.

[Mark Lam]

Updated with Michael's suggestion.  There were no new layout test failures running on chromium.

Landed in r141649: <http://trac.webkit.org/changeset/141649>.