Bug 153084 – Modern IDB: Support opening and deleting SQLite databases on disk

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Modern IDB: Support opening and deleting SQLite databases on disk

[Brady Eidson]

Created attachment 268923 [details]
Patch v1

Touches filesystem - Let's get 3 reviewers up in here.

[WebKit Commit Bot]

Attachment 268923 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/Storage/WebDatabaseProvider.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 268923 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:74
> +    bool ensureValidRecordsTable();
> +    std::unique_ptr<IDBDatabaseInfo> extractExistingDatabaseInfo();
> +    std::unique_ptr<IDBDatabaseInfo> createAndPopulateInitialDatabaseInfo();

These are unimplemented in this patch, but it doesn't hurt to add them to a header.

> Source/WebKit/mac/Storage/WebDatabaseProvider.mm:36
> +        databasesDirectory = @"~/Library/WebKit/Databases/___IndexedDB";

This is equivalent to the old way we get a directory for the databases, but doesn't this allow an application to have access to the IDB databases of another application that uses WebKit?

[Brady Eidson]

(In reply to comment #3)
> Comment on attachment 268923 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268923&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:74
> > +    bool ensureValidRecordsTable();
> > +    std::unique_ptr<IDBDatabaseInfo> extractExistingDatabaseInfo();
> > +    std::unique_ptr<IDBDatabaseInfo> createAndPopulateInitialDatabaseInfo();
> 
> These are unimplemented in this patch, but it doesn't hurt to add them to a
> header.

Yah, whoops - I'll remove them.

This patch was originally much bigger but I decided the review on "create" and "delete" by themselves would be much nicer.

> > Source/WebKit/mac/Storage/WebDatabaseProvider.mm:36
> > +        databasesDirectory = @"~/Library/WebKit/Databases/___IndexedDB";
> 
> This is equivalent to the old way we get a directory for the databases, but
> doesn't this allow an application to have access to the IDB databases of
> another application that uses WebKit?

Yes. Which - for WebSQL databases - was kind of okay, but is definitely not for IDB.

Good catch. I'll throw in the app's bundle id.

[Brady Eidson]

Created attachment 268934 [details]
Patch v2

[WebKit Commit Bot]

Attachment 268934 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/Storage/WebDatabaseProvider.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 268934 [details]
Patch v2

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

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:65
> +    if (!m_sqliteDB->open(dbFilename)) {

This calls SQLiteFileSystem::openDatabase which calls sqlite3_open which creates the file if it doesn't exist.  I think there should be a corresponding if (m_sqliteDB) { m_sqliteDB->close(); m_sqliteDB = nullptr; } in ~SQLiteIDBBackingStore that closes it without deleting it.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:75
> +    if (!m_sqliteDB)
> +        return *m_databaseInfo;
> +
>      return *m_databaseInfo;

This is strange.  It doesn't matter if m_sqliteDB is null.  And right now m_sqliteDB is never null here.  Will this be a meaningful check once the fixme is fixed?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:180
> +    if (m_sqliteDB)
> +        m_sqliteDB->close();

Shouldn't m_sqliteDB be set to nullptr after this?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:183
> +    deleteFile(dbFilename);
> +    deleteEmptyDirectory(m_absoluteDatabaseDirectory);

Since this corresponds the SQLiteFileSystem::openDatabase, I think these should be SQLiteFileSystem::deleteDatabaseFile and SQLiteFileSystem::deleteEmptyDatabaseDirectory to match stylistically, even though those functions do nothing but call these functions.

[Alex Christensen]

Comment on attachment 268934 [details]
Patch v2

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

> Source/WebKit/mac/Storage/WebDatabaseProvider.mm:38
> +        databasesDirectory = WebCore::pathByAppendingComponent(databasesDirectory, ASCIILiteral("___IndexedDB"));

No bundle identifier if WebDatabaseDirectoryDefaultsKey is in the standardUserDefaults?

[Brady Eidson]

(In reply to comment #8)
> Comment on attachment 268934 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268934&action=review
> 
> > Source/WebKit/mac/Storage/WebDatabaseProvider.mm:38
> > +        databasesDirectory = WebCore::pathByAppendingComponent(databasesDirectory, ASCIILiteral("___IndexedDB"));
> 
> No bundle identifier if WebDatabaseDirectoryDefaultsKey is in the
> standardUserDefaults?

Correct.

The bundle id is only used to segregate folks using the default.

If authors are savvy enough to specify the path, they're also savvy enough to specify their bundle id, leading to the problem we're trying to avoid.

If they're that savvy, we can't stop them from getting into this type of trouble.

[Brady Eidson]

(In reply to comment #7)
> Comment on attachment 268934 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268934&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:65
> > +    if (!m_sqliteDB->open(dbFilename)) {
> 
> This calls SQLiteFileSystem::openDatabase which calls sqlite3_open which
> creates the file if it doesn't exist.  I think there should be a
> corresponding if (m_sqliteDB) { m_sqliteDB->close(); m_sqliteDB = nullptr; }
> in ~SQLiteIDBBackingStore that closes it without deleting it.
> 
> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:75
> > +    if (!m_sqliteDB)
> > +        return *m_databaseInfo;
> > +
> >      return *m_databaseInfo;
> 
> This is strange.  It doesn't matter if m_sqliteDB is null.  And right now
> m_sqliteDB is never null here.  Will this be a meaningful check once the
> fixme is fixed?
> 
> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:180
> > +    if (m_sqliteDB)
> > +        m_sqliteDB->close();
> 
> Shouldn't m_sqliteDB be set to nullptr after this?
> 
> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:183
> > +    deleteFile(dbFilename);
> > +    deleteEmptyDirectory(m_absoluteDatabaseDirectory);
> 
> Since this corresponds the SQLiteFileSystem::openDatabase, I think these
> should be SQLiteFileSystem::deleteDatabaseFile and
> SQLiteFileSystem::deleteEmptyDatabaseDirectory to match stylistically, even
> though those functions do nothing but call these functions.

Will make these changes in v3 of the patch when I get a chance to work on v3.

[Brady Eidson]

Created attachment 268975 [details]
Patch v3

[WebKit Commit Bot]

Attachment 268975 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/Storage/WebDatabaseProvider.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 268975 [details]
Patch v3

r=me
Get two more

[Sam Weinig]

Comment on attachment 268975 [details]
Patch v3

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

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:128
> +/*
> +    if (m_databaseDirectoryPath.isEmpty())
> +        return MemoryIDBBackingStore::create(identifier);
> +
> +    return std::make_unique<SQLiteIDBBackingStore>(identifier, m_databaseDirectoryPath);
> +*/

Please don't check-in commented out code.

[Andy Estes]

Comment on attachment 268975 [details]
Patch v3

r=me

[Brady Eidson]

Created attachment 269030 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 269030 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/Storage/WebDatabaseProvider.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 269030 [details]
Patch for landing

Clearing flags on attachment: 269030

Committed r195090: <http://trac.webkit.org/changeset/195090>