Bug 34619 – Add a way to cancel/ignore transactions on a database

• Component: New Bugs
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

When the user wants to clear all browsing data, Chromium needs to be able to delete all DB files. In order to do that, we need to have a way to cancel/ignore transactions on a database, and close the handle to that DB file.

This could lead to canceled/ignored transactions and undefined app behavior, but we believe the user's request to clear all browsing data is more important.

[Dumitru Daniliuc]

Created attachment 48269 [details]
patch

[Jeremy Orlow]

Comment on attachment 48269 [details]
patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 54448)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,20 @@
> [phone]  Dumitru Daniliuc  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Adding a way to get the set of all open database handles pointing
> +        to a given database.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=34619
> +

Maybe add a few more details?


> ===================================================================
> --- WebCore/storage/DatabaseTracker.h	(revision 54448)
> +++ WebCore/storage/DatabaseTracker.h	(working copy)
> @@ -69,12 +70,20 @@ public:
>  
>      void addOpenDatabase(Database*);
>      void removeOpenDatabase(Database*);
> +    void getOpenDatabases(const String& originIdentifier, const String& name, HashSet<Database*>* databases);

This probably should be a hash set of ref pointers.


> Index: WebCore/storage/chromium/DatabaseTrackerChromium.cpp
> ===================================================================
> --- WebCore/storage/chromium/DatabaseTrackerChromium.cpp	(revision 54448)
> +++ WebCore/storage/chromium/DatabaseTrackerChromium.cpp	(working copy)
> @@ -38,6 +38,7 @@
>  #include "QuotaTracker.h"
>  #include "ScriptExecutionContext.h"
>  #include "SecurityOrigin.h"
> +#include "SecurityOriginHash.h"
>  #include "SQLiteFileSystem.h"
>  #include <wtf/HashSet.h>
>  #include <wtf/MainThread.h>
> @@ -76,6 +77,29 @@ String DatabaseTracker::fullPathForDatab
>  void DatabaseTracker::addOpenDatabase(Database* database)
>  {

Don't we need this code in the non-Chromium version too?

If so, maybe there's even some way to share more code?  For example have a common parent class?  Just a thought.


> @@ -102,14 +126,55 @@ private:
>  
>  void DatabaseTracker::removeOpenDatabase(Database* database)
>  {
> +    if (!database)
> +        return;
> +
>      if (!database->scriptExecutionContext()->isContextThread()) {
>          database->scriptExecutionContext()->postTask(TrackerRemoveOpenDatabaseTask::create(database));
>          return;
>      }
>  
> +    MutexLocker openDatabaseMapLock(m_openDatabaseMapGuard);
> +    ASSERT(m_openDatabaseMap);
> +    DatabaseNameMap* nameMap = m_openDatabaseMap->get(database->securityOrigin());
> +    ASSERT(nameMap);
> +    String name(database->stringIdentifier());
> +    DatabaseSet* databaseSet = nameMap->get(name);
> +    ASSERT(databaseSet);
> +    databaseSet->remove(database);
> +
> +    if (databaseSet->isEmpty()) {
> +        nameMap->remove(name);
> +        delete databaseSet;
> +        if (nameMap->isEmpty()) {
> +            m_openDatabaseMap->remove(database->securityOrigin());
> +            delete nameMap;

I wish there was some way to avoid this, but I can't think of any that wouldn't be more complex.

> +        }
> +    }
> +
>      DatabaseObserver::databaseClosed(database);
>  }
>  
> +
> +void DatabaseTracker::getOpenDatabases(const String& originIdentifier, const String& name, HashSet<Database*>* databases)

This method should probably take in a SecurityOrigin pointer.

> +{

Maybe you should clear the databases hash set or at least assert it's empty?


> Index: WebKit/chromium/public/WebDatabase.h
> ===================================================================
> --- WebKit/chromium/public/WebDatabase.h	(revision 54448)
> +++ WebKit/chromium/public/WebDatabase.h	(working copy)
> @@ -72,6 +72,8 @@ public:
>      WEBKIT_API static void updateDatabaseSize(
>          const WebString& originIdentifier, const WebString& databaseName,
>          unsigned long long databaseSize, unsigned long long spaceAvailable);
> +    WEBKIT_API static void closeDatabaseImmediately(
> +        const WebString& originIdentifier, const WebString& databaseName);

Instead of an origin identifier, it should probably take in a "const WebSecurityOrigin&".

>  
>  #if WEBKIT_IMPLEMENTATION
>      WebDatabase(const WTF::PassRefPtr<WebCore::Database>&);
> Index: WebKit/chromium/src/WebDatabase.cpp
> ===================================================================
> --- WebKit/chromium/src/WebDatabase.cpp	(revision 54448)
> +++ WebKit/chromium/src/WebDatabase.cpp	(working copy)
> @@ -32,7 +32,9 @@
>  #include "WebDatabase.h"
>  
>  #include "Database.h"
> +#include "DatabaseTask.h"
>  #include "DatabaseThread.h"
> +#include "DatabaseTracker.h"
>  #include "Document.h"
>  #include "KURL.h"
>  #include "QuotaTracker.h"
> @@ -106,6 +108,21 @@ void WebDatabase::updateDatabaseSize(
>          originIdentifier, databaseName, databaseSize, spaceAvailable);
>  }
>  
> +void WebDatabase::closeDatabaseImmediately(
> +    const WebString& originIdentifier, const WebString& databaseName)

Seems like those lines could be combined.

[Jeremy Orlow]

Oops, lost the context for this one:

> > +{
> 
> Maybe you should clear the databases hash set or at least assert it's empty?
>


It was supposed to be this:

> +void DatabaseTracker::getOpenDatabases(const String& originIdentifier, const String& name, HashSet<Database*>* databases)
> +{

Maybe you should clear the databases hash set or at least assert it's empty?

> +    MutexLocker openDatabaseMapLock(m_openDatabaseMapGuard);
> +    if (!m_openDatabaseMap)
> +        return;

[Eric U.]

> +++ WebCore/storage/chromium/DatabaseTrackerChromium.cpp	(working copy)
> @@ -38,6 +38,7 @@
>  #include "QuotaTracker.h"
>  #include "ScriptExecutionContext.h"
>  #include "SecurityOrigin.h"
> +#include "SecurityOriginHash.h"
>  #include "SQLiteFileSystem.h"
>  #include <wtf/HashSet.h>
>  #include <wtf/MainThread.h>
> @@ -76,6 +77,29 @@ String DatabaseTracker::fullPathForDatab
>  void DatabaseTracker::addOpenDatabase(Database* database)
>  {
>      ASSERT(database->scriptExecutionContext()->isContextThread());

This should never be called with null database; note the line just above.

> +    if (!database)
> +      return;





>  void DatabaseTracker::removeOpenDatabase(Database* database)
>  {

Can this ever be called with null database?

> +    if (!database)
> +        return;
> +

[Dumitru Daniliuc]

Created attachment 48278 [details]
patch

(In reply to comment #2)
> (From update of attachment 48269 [details])
> > Index: WebCore/ChangeLog
> > ===================================================================
> > --- WebCore/ChangeLog	(revision 54448)
> > +++ WebCore/ChangeLog	(working copy)
> > @@ -1,3 +1,20 @@
> > [phone]  Dumitru Daniliuc  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Adding a way to get the set of all open database handles pointing
> > +        to a given database.
> > +
> > +        https://bugs.webkit.org/show_bug.cgi?id=34619
> > +
> 
> Maybe add a few more details?

done.

> > ===================================================================
> > --- WebCore/storage/DatabaseTracker.h	(revision 54448)
> > +++ WebCore/storage/DatabaseTracker.h	(working copy)
> > @@ -69,12 +70,20 @@ public:
> >  
> >      void addOpenDatabase(Database*);
> >      void removeOpenDatabase(Database*);
> > +    void getOpenDatabases(const String& originIdentifier, const String& name, HashSet<Database*>* databases);
> 
> This probably should be a hash set of ref pointers.

done.

> > Index: WebCore/storage/chromium/DatabaseTrackerChromium.cpp
> > ===================================================================
> > --- WebCore/storage/chromium/DatabaseTrackerChromium.cpp	(revision 54448)
> > +++ WebCore/storage/chromium/DatabaseTrackerChromium.cpp	(working copy)
> > @@ -38,6 +38,7 @@
> >  #include "QuotaTracker.h"
> >  #include "ScriptExecutionContext.h"
> >  #include "SecurityOrigin.h"
> > +#include "SecurityOriginHash.h"
> >  #include "SQLiteFileSystem.h"
> >  #include <wtf/HashSet.h>
> >  #include <wtf/MainThread.h>
> > @@ -76,6 +77,29 @@ String DatabaseTracker::fullPathForDatab
> >  void DatabaseTracker::addOpenDatabase(Database* database)
> >  {
> 
> Don't we need this code in the non-Chromium version too?
> 
> If so, maybe there's even some way to share more code?  For example have a
> common parent class?  Just a thought.

i need to refactor DatabaseTracker.{h|cpp} to remove the #ifdefs. i can try to share some of this code then. for nwo though, i don't think there's a much better way to do this.

> > +        }
> > +    }
> > +
> >      DatabaseObserver::databaseClosed(database);
> >  }
> >  
> > +
> > +void DatabaseTracker::getOpenDatabases(const String& originIdentifier, const String& name, HashSet<Database*>* databases)
> 
> This method should probably take in a SecurityOrigin pointer.

done.

> > +{
> 
> Maybe you should clear the databases hash set or at least assert it's empty?

i don't think we need to do this. for example, the caller might want to get all handles for DBs A, B and C. i don't think we should force it to use 3 different hash sets.

> > Index: WebKit/chromium/public/WebDatabase.h
> > ===================================================================
> > --- WebKit/chromium/public/WebDatabase.h	(revision 54448)
> > +++ WebKit/chromium/public/WebDatabase.h	(working copy)
> > @@ -72,6 +72,8 @@ public:
> >      WEBKIT_API static void updateDatabaseSize(
> >          const WebString& originIdentifier, const WebString& databaseName,
> >          unsigned long long databaseSize, unsigned long long spaceAvailable);
> > +    WEBKIT_API static void closeDatabaseImmediately(
> > +        const WebString& originIdentifier, const WebString& databaseName);
> 
> Instead of an origin identifier, it should probably take in a "const
> WebSecurityOrigin&".

the caller operates on strings and is unaware of WebSecurityOrigins. i think it might be better to leave it this way, especially since updateDatabaseSize() uses strings too.

> >  #if WEBKIT_IMPLEMENTATION
> >      WebDatabase(const WTF::PassRefPtr<WebCore::Database>&);
> > Index: WebKit/chromium/src/WebDatabase.cpp
> > ===================================================================
> > --- WebKit/chromium/src/WebDatabase.cpp	(revision 54448)
> > +++ WebKit/chromium/src/WebDatabase.cpp	(working copy)
> > @@ -32,7 +32,9 @@
> >  #include "WebDatabase.h"
> >  
> >  #include "Database.h"
> > +#include "DatabaseTask.h"
> >  #include "DatabaseThread.h"
> > +#include "DatabaseTracker.h"
> >  #include "Document.h"
> >  #include "KURL.h"
> >  #include "QuotaTracker.h"
> > @@ -106,6 +108,21 @@ void WebDatabase::updateDatabaseSize(
> >          originIdentifier, databaseName, databaseSize, spaceAvailable);
> >  }
> >  
> > +void WebDatabase::closeDatabaseImmediately(
> > +    const WebString& originIdentifier, const WebString& databaseName)
> 
> Seems like those lines could be combined.

done.


(In reply to comment #4)
> > +++ WebCore/storage/chromium/DatabaseTrackerChromium.cpp	(working copy)
> > @@ -38,6 +38,7 @@
> >  #include "QuotaTracker.h"
> >  #include "ScriptExecutionContext.h"
> >  #include "SecurityOrigin.h"
> > +#include "SecurityOriginHash.h"
> >  #include "SQLiteFileSystem.h"
> >  #include <wtf/HashSet.h>
> >  #include <wtf/MainThread.h>
> > @@ -76,6 +77,29 @@ String DatabaseTracker::fullPathForDatab
> >  void DatabaseTracker::addOpenDatabase(Database* database)
> >  {
> >      ASSERT(database->scriptExecutionContext()->isContextThread());
> 
> This should never be called with null database; note the line just above.
> 
> > +    if (!database)
> > +      return;

removed.

> >  void DatabaseTracker::removeOpenDatabase(Database* database)
> >  {
> 
> Can this ever be called with null database?
> 
> > +    if (!database)
> > +        return;

i don't think so. removed.

[WebKit Review Bot]

Attachment 48278 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/238526

[Jeremy Orlow]

Comment on attachment 48278 [details]
patch

All your comments seem reasonable.


> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 54456)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,22 @@
> [phone]  Dumitru Daniliuc  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Adding a way to get the set of all open database handles pointing
> +        to a given database. Sometimes we need to be able to close all
> +        handles to a database as soon as possible (to delete the DB file,
> +        for example).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=34619

Please use the change log style of

Headline.
http://bug.link

Details
and
more
details.

[Dumitru Daniliuc]

Changed the format of the comments in ChangeLogs. Also, fixed the chromium build problem.

Landed as r54506.