Bug 110600 – webdatabase: Need a method for reliable multi-process quota management.

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

[Mark Lam]

The current WebSQL origin quota management system is vulnerable to multiple processes contending for the same quota of storage resource concurrently.

For example, let's say we have 3 processes, P1, P2, and P3, opening different databases, DB1, DB2, and DB3 respectively, from the same origin.  Let's also say that there was previously no databases created for that origin.  Let's say P1, P2,  and P3 started to open and write to their databases at about the same time.  In the beginning, all processes sees that the entire quota is available.  Let's say the quota is 5M.  So, P1 starts writing to DB1 and P2 starts writing to DB2.  Meanwhile P3 managed to open its database, but got busy doing something else.  

Since both P1 and P2 think they each have 5M, DB1 and DB2 will both end up growing to around 5M each as long as P1 and P2 does not have reason to scan the disk and check the actual disk usage again.  Let's say the diskUsage ends up riding to around 9M (just shy of the 10M which would result from both DB1 and DB2 filling out 5M each).

Meanwhile, P3 becomes ready to start writing to DB3.  P3 computes its max database size as follows:

    quota = 5M;  // Fetched from a browser setting.
    currentDiskUsage = 9M; // Computed by scanning the actual disk usage for this origin.
    currentSizeofDB3 = 0M; // or near 0 because P3 hasn't written much to it yet.

    // Hence, ...
    availableRoomForGrowth = quota - currentDiskUsage; // i.e. 5M - 9M = -4M.
    maxDatabaseSize = availableRoomForGrowth + currentSizeOfDB3; // i.e. -4M + 0M = 4M.

And maxDatabaseSize is returned as an unsigned long long.  The -4M now gets interpreted as a huge positive number, which gets set as DB3's max database size.  This allows P3 to write an unreasonable large amount of records to DB3 and exceed the bounds of the 5M quota.

There are 2 issues here:

1. The computation of maxDatabaseSize should have enforced its contract to not exceed the quota.  This will be handled in https://bugs.webkit.org/show_bug.cgi?id=110557.

2. We need a method for enforcing the origin quota that works across multiple processes so that each process does not mistakenly think it has storage availability without accounting for the consumption of other processes.  This will handled in this bug.

[Adam Barth]

There's some code relating to quota in http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/quota

[Mark Lam]

<rdar://problem/13260686>

[Mark Lam]

Created attachment 190762 [details]
the fix.

https://bugs.webkit.org/show_bug.cgi?id=110805 changed the disk usage calculation to stat the disk instead of relying on cached values.  What remains is to implement some mutual exclusion so that multiple processes aren’t updating the directory at the same time.  This patch introduces the FileLock to provide that inter-process mutual exclusion.

Note: the FileLock mechanism only provides mutual exclusion against other processes.  It does not provide mutual exclusion between threads.  This means that multiple database threads (within the same process) writing at the same time to the same database origin directory can still result in an inaccurate calculation of the disk usage.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.gypi', u'Source/WTF/WTF.pro', u'Source/WTF/WTF.vcproj/WTF.vcproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj.filters', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wscript', u'Source/WTF/wtf/FileLock.h', u'Source/WTF/wtf/FileLockDarwin.cpp', u'Source/WTF/wtf/FileLockNone.cpp', u'Source/WTF/wtf/PlatformBlackBerry.cmake', u'Source/WTF/wtf/PlatformEfl.cmake', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.h', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp']" exit_code: 1
Source/WTF/wtf/FileLock.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 18 files


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

[Early Warning System Bot]

Comment on attachment 190762 [details]
the fix.

Attachment 190762 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16847216

[Early Warning System Bot]

Comment on attachment 190762 [details]
the fix.

Attachment 190762 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16792093

[EFL EWS Bot]

Comment on attachment 190762 [details]
the fix.

Attachment 190762 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/16666414

[Mark Lam]

Created attachment 190770 [details]
the fix 2.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.gypi', u'Source/WTF/WTF.pro', u'Source/WTF/WTF.vcproj/WTF.vcproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj.filters', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wscript', u'Source/WTF/wtf/FileLock.h', u'Source/WTF/wtf/FileLockDarwin.cpp', u'Source/WTF/wtf/FileLockNone.cpp', u'Source/WTF/wtf/PlatformBlackBerry.cmake', u'Source/WTF/wtf/PlatformEfl.cmake', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.h', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp']" exit_code: 1
Source/WTF/wtf/FileLockNone.cpp:46:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WTF/wtf/FileLock.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 18 files


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

[Mark Lam]

Comment on attachment 190770 [details]
the fix 2.

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

>> Source/WTF/wtf/FileLockNone.cpp:46
>> +	return true;
> 
> Tab found; better to use spaces  [whitespace/tab] [1]

Will fix this tab before committing.

[Mark Lam]

Comment on attachment 190770 [details]
the fix 2.

It's ready for a review.

[Geoffrey Garen]

Comment on attachment 190770 [details]
the fix 2.

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

Please resolve the design issues here before landing.

Let's do a follow-up patch to add an in-process Mutex to FileLock, so that it provides exclusion between threads. (Otherwise, there's no real reason to have a table of FileLocks.)

> Source/WTF/ChangeLog:24
> +        * wtf/FileLockDarwin.cpp: Added.

Let's call this FileLockUnix.cpp, since the flock API is available on most flavors of Unix and Linux. (See http://linux.die.net/man/2/flock.) This is our convention for other classes like this.

> Source/WTF/wtf/FileLockDarwin.cpp:47
> +    m_lock.filename = filename;
> +    int length = filename.length();
> +
> +    m_lock.filenameCStr = new char[length + 1];
> +    strncpy(m_lock.filenameCStr, filename.latin1().data(), length);
> +    m_lock.filenameCStr[length] = '\0';

Let's not reinvent String=>CString conversion here. Instead, let's store just a CString as a data member, and initialize it with filename.latin1().

> Source/WTF/wtf/FileLockDarwin.cpp:49
> +    m_lock.fd = open(m_lock.filenameCStr, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);

It would be nice, in a follow-up patch, to hold the file open only as long as it is supposed to be locked.

> Source/WTF/wtf/FileLockDarwin.cpp:58
> +    if (m_deleteFileFunc)
> +        (*m_deleteFileFunc)(m_lock.filename.latin1().data());

Let's not use a deletion callback here. FileLock has its own fd for this file, which it knows how to manage, regardless of what's happening in the user-visible file system. Only our client knows when it's the best time to remove the .lock file from the user-visible file system. That should happen independently of releasing or deleting the file lock.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:679
> +    FileLock* lock = m_fileLockMap.take(origin);
> +    if (lock)
> +        delete lock;

Please use OwnPtr instead of manual pointer management.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:681
> +    else
> +        deleteFile(pathByAppendingComponent(originPath(origin), String(".lock")));

This shouldn't be conditional.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:848
> +        deleteFileLockFor(origin);

This function should remove the lock from the table and also unlink the .lock file.

[Mark Lam]

Created attachment 191187 [details]
work in progress to try out on the ews bots (not ready for review yet).

While prepping the last patch for a commit, I found some race conditions that can still result in runaway growth of the database files.  Instead of committing it, I decided to do some more testing and address the remainder of Geoff's feedback to also implement FileLock mutual exclusion between threads.  So, here's what's new in this patch:

1. Added a mutex to FileLock to provide mutual exclusion between threads.
2. The actual lock file on disk is opened only when we need to lock it, and is closed after we unlock it.  We do not hold on to the file descriptor beyond that.
3. Introduced an OriginLock that wraps the FileLock.  This is necessary because the database origin directory may be deleted by user request at any time while the one or more database threads may still be using the FileLock.  Hence,
    a. OriginLock is ThreadSafeRefCounted so that it (and the FileLock it owns) will only be deleted when it is no longer needed by any thread.
    b. Only the DatabaseThread (via SQLTransactionBackend) will lock and unlock it.  This ensures that the FileLock gets unlocked properly (either in the course of a normal transaction, or in interruption clean up code) before it gets destructed.
    c. The OriginLock destructor will delete/unlink the lock file for the FileLock after it has destructed / un-owned it.
    d. DatabaseTracker also needs to delete/unlink the lock file if it tries to delete an origin but found no OriginLock for it.  Otherwise, it leave the lock file deletion to the OriginLock destructor.

4. SQLTransactionBackend now needs to have a RefPtr to the OriginLock to keep it alive long enough to be unlocked in case the user tries to delete the origin directory.  If the SQLTransactionBackend hasn't unlocked the OriginLock yet, then the origin directory will not get deleted.

5. The DatabaseTracker::m_originLockMap should use a String as its key instead of the SecurityOrigin.  The String to use in this case is the databaseIdentifier that is uniquely identifies the database origin subdirectory.  Using the SecurityOrigin as the key has bad side effects because of the special treatment of "files://".  Anyway, the originLockMap is intended to map the subdirectory to it lock, not to map the SecurityOrigin to the lock.  Hence, using the databaseIdentifier String as the key is the right thing to do. 

6.  DatabaseTracker::getMaxSizeForDatabase() needs to explicitly check if the disk usage exceeds the quota, and return the existing databaseSize if so instead of calculating a max DB size.  This is because to not do so may result in a very small max DB size that allow the DB to grow without bounds.  Here are 2 conditions that can result in that:
     a. due to multi-process inconsistencies in reading the tracker DB (Database.db), we can get a quota of 0 under some circumstances.  This is a race condition that was discovered in some of my test runs.
     b. if we have reached a disk usage which is near the quota but does not reach it yet, and we try to open a database with a size of 1 byte (or any amount within the remaining quota budget), the openDatabase() call will succeed.  However, this results in a diskUsage that now exceeds the quota because creating that DB file actually takes more than it claims.
     c. contrary to expectations, rolling back a transaction doesn't seem to revert the size of the database.  My 1st simple attempt at forcing a vacuuming of the database immediately after the rollback didn't seem to help either.  Further testing is needed to see if something can be done there to revert the disk usage after a rollback.
     So far, the only avenue for preventing over-usage that I've found to work is the "PRAGMA max_page_count=N" that was set on the sqlite DB before we start a transaction.  That prevents the DB from growing beyond its given max size.  The rollback doesn't seem to be effective in preventing excess disk usage.

     Regardless, by adding an explicit check to handle case when the disk usage exceeds the quota, we prevent runaway growth of the DB (and this was confirmed by testing).

7. Enhanced FileLock to handle the scenario where the lock file in use can be deleted by the user from under it.  In that case, we expect the database origin directory and the databases to also be deleted.  We just need to handle the arising failure conditions gracefully.

8. Fixed a bug in SQLStatement where it should check if an errorCallback exists before invoking it.

Again, this patch is not ready for a review yet.  The ChangeLogs and some comments need to be updated.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.gypi', u'Source/WTF/WTF.pro', u'Source/WTF/WTF.vcproj/WTF.vcproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj.filters', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wscript', u'Source/WTF/wtf/FileLock.h', u'Source/WTF/wtf/FileLockNone.cpp', u'Source/WTF/wtf/FileLockUnix.cpp', u'Source/WTF/wtf/PlatformBlackBerry.cmake', u'Source/WTF/wtf/PlatformEfl.cmake', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.h', u'Source/WebCore/Modules/webdatabase/OriginLock.cpp', u'Source/WebCore/Modules/webdatabase/OriginLock.h', u'Source/WebCore/Modules/webdatabase/SQLStatement.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.h', u'Source/WebCore/Modules/webdatabase/chromium/DatabaseTrackerChromium.cpp', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gyp/WebCore.gyp', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj']" exit_code: 1
Source/WTF/wtf/FileLock.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 32 files


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

[Early Warning System Bot]

Comment on attachment 191187 [details]
work in progress to try out on the ews bots (not ready for review yet).

Attachment 191187 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16914353

[Mark Lam]

Created attachment 191188 [details]
quota-test.html used in the testing of the patch.

[Early Warning System Bot]

Comment on attachment 191187 [details]
work in progress to try out on the ews bots (not ready for review yet).

Attachment 191187 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16899805

[Mark Lam]

Created attachment 191189 [details]
work in progress 2: fixed rot in FileLockNone.

[WebKit Review Bot]

Comment on attachment 191189 [details]
work in progress 2: fixed rot in FileLockNone.

Attachment 191189 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16898804

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.gypi', u'Source/WTF/WTF.pro', u'Source/WTF/WTF.vcproj/WTF.vcproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj.filters', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wscript', u'Source/WTF/wtf/FileLock.h', u'Source/WTF/wtf/FileLockNone.cpp', u'Source/WTF/wtf/FileLockUnix.cpp', u'Source/WTF/wtf/PlatformBlackBerry.cmake', u'Source/WTF/wtf/PlatformEfl.cmake', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.h', u'Source/WebCore/Modules/webdatabase/OriginLock.cpp', u'Source/WebCore/Modules/webdatabase/OriginLock.h', u'Source/WebCore/Modules/webdatabase/SQLStatement.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.h', u'Source/WebCore/Modules/webdatabase/chromium/DatabaseTrackerChromium.cpp', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gyp/WebCore.gyp', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj']" exit_code: 1
Source/WTF/wtf/FileLock.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 32 files


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

[Mark Lam]

Created attachment 191192 [details]
work in progress 3: fixed my sloppy code in the chromium port.

[WebKit Review Bot]

Comment on attachment 191192 [details]
work in progress 3: fixed my sloppy code in the chromium port.

Attachment 191192 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16901692

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.gypi', u'Source/WTF/WTF.pro', u'Source/WTF/WTF.vcproj/WTF.vcproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj.filters', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wscript', u'Source/WTF/wtf/FileLock.h', u'Source/WTF/wtf/FileLockNone.cpp', u'Source/WTF/wtf/FileLockUnix.cpp', u'Source/WTF/wtf/PlatformBlackBerry.cmake', u'Source/WTF/wtf/PlatformEfl.cmake', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.h', u'Source/WebCore/Modules/webdatabase/OriginLock.cpp', u'Source/WebCore/Modules/webdatabase/OriginLock.h', u'Source/WebCore/Modules/webdatabase/SQLStatement.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.h', u'Source/WebCore/Modules/webdatabase/chromium/DatabaseTrackerChromium.cpp', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gyp/WebCore.gyp', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj']" exit_code: 1
Source/WTF/wtf/FileLock.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 32 files


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

[Mark Lam]

Comment on attachment 191192 [details]
work in progress 3: fixed my sloppy code in the chromium port.

Sigh ... should not code when I should be sleeping.  Will look at this in the morning.

[Mark Lam]

Created attachment 191194 [details]
work in progress 4: one more time to test the ews bots.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.gypi', u'Source/WTF/WTF.pro', u'Source/WTF/WTF.vcproj/WTF.vcproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj.filters', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wscript', u'Source/WTF/wtf/FileLock.h', u'Source/WTF/wtf/FileLockNone.cpp', u'Source/WTF/wtf/FileLockUnix.cpp', u'Source/WTF/wtf/PlatformBlackBerry.cmake', u'Source/WTF/wtf/PlatformEfl.cmake', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.h', u'Source/WebCore/Modules/webdatabase/OriginLock.cpp', u'Source/WebCore/Modules/webdatabase/OriginLock.h', u'Source/WebCore/Modules/webdatabase/SQLStatement.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransactionBackend.h', u'Source/WebCore/Modules/webdatabase/chromium/DatabaseTrackerChromium.cpp', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gyp/WebCore.gyp', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj']" exit_code: 1
Source/WTF/wtf/FileLock.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 32 files


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

[WebKit Review Bot]

Comment on attachment 191194 [details]
work in progress 4: one more time to test the ews bots.

Attachment 191194 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16905631

[WebKit Review Bot]

Comment on attachment 191194 [details]
work in progress 4: one more time to test the ews bots.

Attachment 191194 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16898851

[Build Bot]

Comment on attachment 191194 [details]
work in progress 4: one more time to test the ews bots.

Attachment 191194 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16913422

New failing tests:
fast/workers/storage/read-and-write-transactions-dont-run-together.html
storage/websql/multiple-databases-garbage-collection.html
fast/workers/storage/multiple-transactions-on-different-handles.html
storage/websql/statement-error-callback-isolated-world.html
fast/workers/storage/multiple-databases-garbage-collection.html
storage/websql/transaction-success-callback-isolated-world.html
fast/workers/storage/open-database-while-transaction-in-progress.html
storage/websql/transaction-error-callback-isolated-world.html
storage/websql/transaction-callback-isolated-world.html
storage/websql/open-database-creation-callback.html
storage/websql/statement-success-callback-isolated-world.html
storage/websql/multiple-transactions-on-different-handles.html
storage/websql/multiple-transactions.html
fast/workers/storage/test-authorizer.html

[Build Bot]

Comment on attachment 191194 [details]
work in progress 4: one more time to test the ews bots.

Attachment 191194 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16910665

New failing tests:
storage/websql/transaction-success-callback-isolated-world.html
storage/websql/open-database-creation-callback.html

[Mark Lam]

(In reply to comment #30)
> storage/websql/transaction-success-callback-isolated-world.html
> storage/websql/open-database-creation-callback.html

These are due to SQLTransactionCoordinator gating database writes based on the database identifier vs the origin identifier.  This results in a deadlock when the transaction code is locking the origin file lock based on the assumption that there can only be 1 write transaction at a time per origin. 

This is now fixed.  Looking into other layout test failures / flakiness next.

[Mark Lam]

(In reply to comment #31)
> Looking into other layout test failures / flakiness next.

The remaining 2 tests that failed are only with text diffs.  These tests are:
1. fast/workers/storage/multiple-databases-garbage-collection.html
2. storage/websql/multiple-databases-garbage-collection.html

Both of these tests creates a "Persistent" and "Forgotten" database.  The test executes a transaction on the persistent db first, followed by one on the forgotten db.  The transaction for the persistent db involves multiple statements and presumably would take longer to run.  The transaction on the forgotten db only creates an empty table which should take less time to run.

Previously, the SQLTransactionCoodinator allows writes to 2 different databases "concurrently".  Hence, the forgotten db would get to complete before the persistent db is done running its transactions.  However, currently, since we only allow 1 write transaction at a time per origin (and these 2 dbs are from the same origin), the persistent db is guaranteed to finish first before the forgotten db.

This difference is what resulted in the text difference in the test output i.e. that the persistent db completes its work before the forgotten db does.

The purpose of the test is to check that resources associated with both dbs are "freed gracefully".  There is no guarantee that one transaction should complete before the other.  Hence, there is no failure here.

I will figure out what to do about updating the test results to reflect this new state of things.

[Geoffrey Garen]

Comment on attachment 191194 [details]
work in progress 4: one more time to test the ews bots.

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

> Source/WTF/wscript:42
> +            wtf_excludes += ['FileLockDarwin.cpp', 'OSAllocatorPosix.cpp', 'ThreadingPthreads.cpp']

FileLockUnix.cpp, please.

> Source/WTF/wtf/FileLock.h:35
> +#include <wtf/Platform.h>
> +
> +#include <wtf/FastAllocBase.h>
> +#include <wtf/Locker.h>
> +#include <wtf/Noncopyable.h>
> +#include <wtf/text/CString.h>
> +#include <wtf/text/WTFString.h>

Sort alphabetically, please. Most editors have a feature for this. M-x sort-lines may work in emacs.

> Source/WTF/wtf/FileLock.h:71
> +    CString filenameCStr;

WebKit style avoids putting types into variable names in almost all cases. Let's call this "filename" instead.

> Source/WTF/wtf/FileLock.h:72
> +    CString filenameCStr;
> +    Mutex mutex;

It's a little weird to put cross-platform abstractions like Mutex and CString into a platform-specific data type. I think it would be clearer to make Mutex and filename direct data members of FileLock. fd is the only platform-specific data here.

> Source/WTF/wtf/FileLockUnix.cpp:79
> +        // Ditto. Lost the directory.
> +        ASSERT(errno == ENOENT);
> +        closeLockFile(m_lock.fd);

I don't understand this ASSERT+comment. Deleting a directory's reference to a file does not also close/delete open file descriptors for that file. Also, the flock() man page specifies that it never returns ENOENT.

> Source/WTF/wtf/FileLockUnix.cpp:109
> +bool FileLock::tryLock()
> +{
> +    bool success = m_lock.mutex.tryLock();
> +    if (!success)
> +        return false;
> +
> +    m_lock.fd = openLockFile(m_lock.filenameCStr.data());
> +    if (m_lock.fd == -1) {
> +        // The only way we can get here is if the directory containing the lock
> +        // has been deleted or we were given a path to a non-existant directory.
> +        // In that case, there's nothing we can do but return a failure.
> +        ASSERT(errno == ENOENT);
> +        m_lock.mutex.unlock();
> +        return false;
> +    }
> +
> +    int result = flock(m_lock.fd, LOCK_EX | LOCK_NB);
> +    if (result == -1) {
> +        closeLockFile(m_lock.fd);
> +        m_lock.mutex.unlock();
> +        return false;
> +    }
> +
> +    return true;

You can eliminate this duplicate code by having lock() and tryLock() called a shared helper tryLock() that takes the flock() operation flag as its argument and returns a bool result.

> Source/WTF/wtf/FileLockUnix.cpp:126
> +    // However, if the file descriptor was initialized, the directory and the
> +    // lock file could still have been deleted from under us. We can assert
> +    // that.
> +    int result = flock(m_lock.fd, LOCK_UN);
> +    if (result == -1)
> +        ASSERT(errno == ENOENT);

Again, I don't understand this ASSERT+comment.

> Source/WebCore/ChangeLog:10
> +        This change is needed so that disk usage measurements for storage quota
> +        enforcement can be more reliable in multi-process scenarios.

You should expand this comment to explain that this patch resolves multi-threading problems in addition to multi-processing problems.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:663
> +    RefPtr<OriginLock> lock = m_originLockMap.get(databaseIdentifier);
> +    if (!lock) {
> +        String path = originPath(origin);
> +        lock = adoptRef(new OriginLock(path));
> +        m_originLockMap.set(databaseIdentifier.isolatedCopy(), lock);

Our idiom for this, to avoid two hash lookups when adding an item for the first time, is to initially call add(), passing null, and then fix up the add result's value if and only if the add resulted in a new hash table entry.

What's the point of the call to isolatedCopy() here? Is it guaranteed that originLockFor() is only ever called on one thread?

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:681
> +    // deleteOriginLockFor() is called to delete the lock file for the origin.
> +    // We need to delete both the OriginLock instance in memory (if present),
> +    // as well as the lock file on disk. However, we don't know if there are
> +    // still some database threads who have not cleaned up their use of the

Do we actually allow origin delete operations to execute concurrently with database transactions?

I believe we should unconditionally delete the lock file.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:697
> +    RefPtr<OriginLock> lock = m_originLockMap.take(origin->databaseIdentifier());
> +    if (lock)
> +        lock.clear();
> +    else {
> +        String lockFileName = OriginLock::lockFileNameForPath(originPath(origin));
> +        deleteFile(lockFileName);
> +    }

C++ destructor semantics make this code unnecessary.

> Source/WebCore/Modules/webdatabase/OriginLock.cpp:44
> +    m_lock = adoptPtr(new FileLock(m_lockFilename));

No need to malloc the FileLock here. It can be a direct data member.

[Adam Barth]

Comment on attachment 191194 [details]
work in progress 4: one more time to test the ews bots.

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

I'm not very happy with this design.  In the Chromium port, WebKit never talks directly to the file system.  As a first step, we should put this file-system dependent code behind an ifdef that makes it clear that not all port support FileLock.

Rather than creating dummy "lock" classes that do nothing, we should guard the uses of the lock with the ifdef to make it clear to people reading the code that this kind of lock doesn't exist in every configuration.

Really, I think we should use a different design that doesn't involve WebKit talking directly to the file system, but that might be more than you want to bite off in this patch.

> Source/WTF/wtf/FileLockUnix.cpp:41
> +static int openLockFile(const char* fileName)
> +{
> +    return open(fileName, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
> +}

I'm not sure OS(DARWIN) is the right compile-guard for this code.  chromium-mac defines OS(DARWIN) on Mac OS X, but Chromium certainly doesn't want to touch the file system from WebKit.  Perhaps we should have a USE(FILE_LOCK) define that's set for PLATFORM(MAC)?

[Mark Lam]

Created attachment 191361 [details]
the updated fix.

[Adam Barth]

Comment on attachment 191361 [details]
the updated fix.

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

> Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp:850
> +void SQLTransactionBackend::acquireOriginLock()

Should we have a RAII object to ensure that we don't leak the lock?

[Early Warning System Bot]

Comment on attachment 191361 [details]
the updated fix.

Attachment 191361 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16906102

[Geoffrey Garen]

Comment on attachment 191361 [details]
the updated fix.

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

r=me

Please address comments before landing.

It looks like this patch addresses Adam's request for an #if USE(FILE_LOCK).

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:287
> +    if (diskUsage > quota)
> +        return databaseFileSize;

Would be nice to fix SQLiteDatabase::setMaximumSize() in a follow-up patch, to fix the bug you observed: that setting the size to less than the page size can be interpreted as an infinite size.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:659
> +    RefPtr<OriginLock> lock = m_originLockMap.get(databaseIdentifier);

add(), please.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:667
> +        // The originLockMap is accessed from multiple DatabaseThreads since
> +        // different script contexts can be writing to different databases from
> +        // the same origin. Hence, the databaseIdentifier needs to be an
> +        // isolated copy.

I would say, "An isolated copy gives us a value whose refcounting is thread-safe, since our copy is guarded by a mutex."

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:692
> +    lock.clear();

Instead of clear, just don't put the value in a variable. In fact, we should just call remove().

[Geoffrey Garen]

Please fix the build before landing, too.

[Early Warning System Bot]

Comment on attachment 191361 [details]
the updated fix.

Attachment 191361 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16930037

[Build Bot]

Comment on attachment 191361 [details]
the updated fix.

Attachment 191361 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16982009

[WebKit Review Bot]

Comment on attachment 191361 [details]
the updated fix.

Attachment 191361 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16994002

[Build Bot]

Comment on attachment 191361 [details]
the updated fix.

Attachment 191361 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16936054

[WebKit Review Bot]

Comment on attachment 191361 [details]
the updated fix.

Attachment 191361 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16996060

[Mark Lam]

Comment on attachment 191361 [details]
the updated fix.

Will upload a fixed patch for the release build and chromium build issues after I address Geoff's comments.

[Mark Lam]

Created attachment 191414 [details]
updated fix 2: addressed Geoff’s comments, and fixed release and chromium build issues.

[Mark Lam]

Will make sure the ews bots are green before landing.

[Mark Lam]

EWS bots are green.  Landed in r144760: <http://trac.webkit.org/changeset/144760>.