WebSQL: User cannot grant quota increase if the JS provides an expected usage value...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Sep 2018 19:59:40 +0000 (19:59 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Sep 2018 19:59:40 +0000 (19:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189801
<rdar://problem/43592498>

Reviewed by Youenn Fablet.

Source/WebCore:

User was unable to grant a quota increase for WebSQL if the JS provided an expected usage value that
is too low. This is because WebKit was passing this provided expectedUsage value to the client for
the purpose of quota increase, even when this expectedUsage value does not make any sense (i.e. it
is lower than the current database size). As a result, the client would grant a quota that is equal
to the previous quota and the JS would not be able to insert any data.

In order to address the issue, when the current quota is exceeded and Database::didExceedQuota()
is called, we now make sure that the expectedUsage value is greater than the current quota. If it
is not, we provide `current quota + 5MB` as expected usage to the client. This way, the client will
grant a quota that is actually increased (provided that the user accepts).

Test: storage/websql/transaction-database-expand-quota.html

* Modules/webdatabase/Database.cpp:
(WebCore::Database::setEstimatedSize):
(WebCore::Database::didExceedQuota):
* Modules/webdatabase/Database.h:

LayoutTests:

Add layout test coverage.

* storage/websql/transaction-database-expand-quota-expected.txt: Added.
* storage/websql/transaction-database-expand-quota.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@236348 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/storage/websql/transaction-database-expand-quota-expected.txt [new file with mode: 0644]
LayoutTests/storage/websql/transaction-database-expand-quota.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/webdatabase/Database.cpp
Source/WebCore/Modules/webdatabase/Database.h
Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp
Source/WebCore/Modules/webdatabase/DatabaseTracker.h

index 33c9af9..74e9db7 100644 (file)
@@ -1,3 +1,16 @@
+2018-09-21  Chris Dumez  <cdumez@apple.com>
+
+        WebSQL: User cannot grant quota increase if the JS provides an expected usage value that is too low
+        https://bugs.webkit.org/show_bug.cgi?id=189801
+        <rdar://problem/43592498>
+
+        Reviewed by Youenn Fablet.
+
+        Add layout test coverage.
+
+        * storage/websql/transaction-database-expand-quota-expected.txt: Added.
+        * storage/websql/transaction-database-expand-quota.html: Added.
+
 2018-09-21  Youenn Fablet  <youenn@apple.com>
 
         Add RTCCodecStats support
diff --git a/LayoutTests/storage/websql/transaction-database-expand-quota-expected.txt b/LayoutTests/storage/websql/transaction-database-expand-quota-expected.txt
new file mode 100644 (file)
index 0000000..aec4b01
--- /dev/null
@@ -0,0 +1,14 @@
+UI DELEGATE DATABASE CALLBACK: exceededDatabaseQuotaForSecurityOrigin:{file, , 0} database:ExpandedQuotaTransaction
+UI DELEGATE DATABASE CALLBACK: increased quota to 6291456
+UI DELEGATE DATABASE CALLBACK: exceededDatabaseQuotaForSecurityOrigin:{file, , 0} database:ExpandedQuotaTransaction
+UI DELEGATE DATABASE CALLBACK: increased quota to 11534336
+Check that a quota increase is granted, even if the provided expected size is too low.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Successfully wrote data
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/storage/websql/transaction-database-expand-quota.html b/LayoutTests/storage/websql/transaction-database-expand-quota.html
new file mode 100644 (file)
index 0000000..9c63b0e
--- /dev/null
@@ -0,0 +1,68 @@
+<html>
+<head>
+<script src="../../resources/js-test.js"></script>
+</head>
+<body onload="runTest()">
+<script>
+description("Check that a quota increase is granted, even if the provided expected size is too low.");
+jsTestIsAsync = true;
+
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.clearAllDatabases();
+    testRunner.dumpDatabaseCallbacks(); 
+    testRunner.databaseDefaultQuota = 5 * 1024 * 1024;
+    testRunner.databaseMaxQuota = 50 * 1024 * 1024;
+}
+
+let db;
+
+const dataSize = 8 * 1024 * 1024; // 8 MB.
+let largeData = "";
+for (let i = 0; i < dataSize; i++)
+    largeData += "x";
+
+let isFirstAttempt = true;
+
+function writeData()
+{
+    db.transaction((tx) => {
+        let id = 1;
+        tx.executeSql('INSERT INTO foo (id, text) VALUES (?, ?)', [id, largeData]);
+    }, (error) => {
+        if (isFirstAttempt && error.code == SQLError.QUOTA_ERR) {
+            isFirstAttempt = false;
+            setTimeout(writeData, 0);
+            return;
+        }
+        testFailed("Failed to write data: " + error.code + ": " + error.message);
+        finishJSTest();
+    }, () => {
+        testPassed("Successfully wrote data");
+        finishJSTest();
+    });
+}
+
+function runTest() {
+    try {
+        db = openDatabase('ExpandedQuotaTransaction', '', '', 6 * 1024 * 1024);
+    } catch (err) {
+        testFailed("Failed to open the database");
+        finishJSTest();
+        return;
+    }
+
+    db.transaction((tx) => {
+        tx.executeSql('CREATE TABLE foo (id unique, text)');
+    }, (error) => {
+        testFailed("Failed to create table: " + error.code + ": " + error.message);
+        finishJSTest();
+    }, () => {
+       writeData();
+    });
+}
+
+onload = runTest;
+</script>
+</body>
+</html>
index 198f444..f7aab9d 100644 (file)
@@ -1,3 +1,29 @@
+2018-09-21  Chris Dumez  <cdumez@apple.com>
+
+        WebSQL: User cannot grant quota increase if the JS provides an expected usage value that is too low
+        https://bugs.webkit.org/show_bug.cgi?id=189801
+        <rdar://problem/43592498>
+
+        Reviewed by Youenn Fablet.
+
+        User was unable to grant a quota increase for WebSQL if the JS provided an expected usage value that
+        is too low. This is because WebKit was passing this provided expectedUsage value to the client for
+        the purpose of quota increase, even when this expectedUsage value does not make any sense (i.e. it
+        is lower than the current database size). As a result, the client would grant a quota that is equal
+        to the previous quota and the JS would not be able to insert any data.
+
+        In order to address the issue, when the current quota is exceeded and Database::didExceedQuota()
+        is called, we now make sure that the expectedUsage value is greater than the current quota. If it
+        is not, we provide `current quota + 5MB` as expected usage to the client. This way, the client will
+        grant a quota that is actually increased (provided that the user accepts).
+
+        Test: storage/websql/transaction-database-expand-quota.html
+
+        * Modules/webdatabase/Database.cpp:
+        (WebCore::Database::setEstimatedSize):
+        (WebCore::Database::didExceedQuota):
+        * Modules/webdatabase/Database.h:
+
 2018-09-21  Youenn Fablet  <youenn@apple.com>
 
         Use biplanar CVPixelBuffer for black frames sent to libwebrtc
index 4915fcc..63333c0 100644 (file)
@@ -90,6 +90,7 @@ namespace WebCore {
 
 static const char versionKey[] = "WebKitDatabaseVersionKey";
 static const char unqualifiedInfoTableName[] = "__WebKitDatabaseInfoTable__";
+const unsigned long long quotaIncreaseSize = 5 * 1024 * 1024;
 
 static const char* fullyQualifiedInfoTableName()
 {
@@ -191,7 +192,7 @@ static inline DatabaseGUID guidForOriginAndName(const String& origin, const Stri
     }).iterator->value;
 }
 
-Database::Database(DatabaseContext& context, const String& name, const String& expectedVersion, const String& displayName, unsigned estimatedSize)
+Database::Database(DatabaseContext& context, const String& name, const String& expectedVersion, const String& displayName, unsigned long long estimatedSize)
     : m_scriptExecutionContext(*context.scriptExecutionContext())
     , m_contextThreadSecurityOrigin(m_scriptExecutionContext->securityOrigin()->isolatedCopy())
     , m_databaseThreadSecurityOrigin(m_scriptExecutionContext->securityOrigin()->isolatedCopy())
@@ -617,11 +618,17 @@ String Database::displayName() const
     return m_displayName.isolatedCopy();
 }
 
-unsigned Database::estimatedSize() const
+unsigned long long Database::estimatedSize() const
 {
     return m_estimatedSize;
 }
 
+void Database::setEstimatedSize(unsigned long long estimatedSize)
+{
+    m_estimatedSize = estimatedSize;
+    DatabaseTracker::singleton().setDatabaseDetails(securityOrigin(), m_name, m_displayName, m_estimatedSize);
+}
+
 String Database::fileName() const
 {
     // Return a deep copy for ref counting thread safety
@@ -788,6 +795,11 @@ bool Database::didExceedQuota()
     ASSERT(databaseContext().scriptExecutionContext()->isContextThread());
     auto& tracker = DatabaseTracker::singleton();
     auto oldQuota = tracker.quota(securityOrigin());
+    if (estimatedSize() <= oldQuota) {
+        // The expected usage provided by the page is now smaller than the actual database size so we bump the expected usage to
+        // oldQuota + 5MB so that the client actually increases the quota.
+        setEstimatedSize(oldQuota + quotaIncreaseSize);
+    }
     databaseContext().databaseExceededQuota(stringIdentifier(), details());
     return tracker.quota(securityOrigin()) > oldQuota;
 }
index 348d6ae..1f75740 100644 (file)
@@ -88,7 +88,7 @@ public:
     // Internal engine support
     String stringIdentifier() const;
     String displayName() const;
-    unsigned estimatedSize() const;
+    unsigned long long estimatedSize() const;
     String fileName() const;
     DatabaseDetails details() const;
     SQLiteDatabase& sqliteDatabase() { return m_sqliteDatabase; }
@@ -126,7 +126,7 @@ public:
     void performClose();
 
 private:
-    Database(DatabaseContext&, const String& name, const String& expectedVersion, const String& displayName, unsigned estimatedSize);
+    Database(DatabaseContext&, const String& name, const String& expectedVersion, const String& displayName, unsigned long long estimatedSize);
 
     void closeDatabase();
 
@@ -137,6 +137,7 @@ private:
     String getCachedVersion() const;
     void setCachedVersion(const String&);
     bool getActualVersionForTransaction(String& version);
+    void setEstimatedSize(unsigned long long);
 
     void scheduleTransaction();
 
@@ -157,7 +158,7 @@ private:
     String m_name;
     String m_expectedVersion;
     String m_displayName;
-    unsigned m_estimatedSize;
+    unsigned long long m_estimatedSize;
     String m_filename;
 
     DatabaseGUID m_guid;
index dde7cc6..bc28b3f 100644 (file)
@@ -136,7 +136,7 @@ void DatabaseTracker::openTrackerDatabase(TrackerCreationAction createAction)
     }
 }
 
-ExceptionOr<void> DatabaseTracker::hasAdequateQuotaForOrigin(const SecurityOriginData& origin, unsigned estimatedSize)
+ExceptionOr<void> DatabaseTracker::hasAdequateQuotaForOrigin(const SecurityOriginData& origin, unsigned long long estimatedSize)
 {
     ASSERT(!m_databaseGuard.tryLock());
     auto usage = this->usage(origin);
@@ -152,7 +152,7 @@ ExceptionOr<void> DatabaseTracker::hasAdequateQuotaForOrigin(const SecurityOrigi
     return { };
 }
 
-ExceptionOr<void> DatabaseTracker::canEstablishDatabase(DatabaseContext& context, const String& name, unsigned estimatedSize)
+ExceptionOr<void> DatabaseTracker::canEstablishDatabase(DatabaseContext& context, const String& name, unsigned long long estimatedSize)
 {
     LockHolder lockDatabase(m_databaseGuard);
 
@@ -200,7 +200,7 @@ ExceptionOr<void> DatabaseTracker::canEstablishDatabase(DatabaseContext& context
 // hasAdequateQuotaForOrigin() simple and correct (i.e. bug free), and just
 // re-use it. Also note that the path for opening a database involves IO, and
 // hence should not be a performance critical path anyway. 
-ExceptionOr<void> DatabaseTracker::retryCanEstablishDatabase(DatabaseContext& context, const String& name, unsigned estimatedSize)
+ExceptionOr<void> DatabaseTracker::retryCanEstablishDatabase(DatabaseContext& context, const String& name, unsigned long long estimatedSize)
 {
     LockHolder lockDatabase(m_databaseGuard);
 
@@ -476,7 +476,7 @@ DatabaseDetails DatabaseTracker::detailsForNameAndOrigin(const String& name, con
     return DatabaseDetails(name, displayName, expectedUsage, SQLiteFileSystem::getDatabaseFileSize(path), SQLiteFileSystem::databaseCreationTime(path), SQLiteFileSystem::databaseModificationTime(path));
 }
 
-void DatabaseTracker::setDatabaseDetails(const SecurityOriginData& origin, const String& name, const String& displayName, unsigned estimatedSize)
+void DatabaseTracker::setDatabaseDetails(const SecurityOriginData& origin, const String& name, const String& displayName, unsigned long long estimatedSize)
 {
     String originIdentifier = origin.databaseIdentifier();
     int64_t guid = 0;
index f7c07b6..71ab5eb 100644 (file)
@@ -66,10 +66,10 @@ public:
     // m_databaseGuard and m_openDatabaseMapGuard currently don't overlap.
     // notificationMutex() is currently independent of the other locks.
 
-    ExceptionOr<void> canEstablishDatabase(DatabaseContext&, const String& name, unsigned estimatedSize);
-    ExceptionOr<void> retryCanEstablishDatabase(DatabaseContext&, const String& name, unsigned estimatedSize);
+    ExceptionOr<void> canEstablishDatabase(DatabaseContext&, const String& name, unsigned long long estimatedSize);
+    ExceptionOr<void> retryCanEstablishDatabase(DatabaseContext&, const String& name, unsigned long long estimatedSize);
 
-    void setDatabaseDetails(const SecurityOriginData&, const String& name, const String& displayName, unsigned estimatedSize);
+    void setDatabaseDetails(const SecurityOriginData&, const String& name, const String& displayName, unsigned long long estimatedSize);
     WEBCORE_EXPORT String fullPathForDatabase(const SecurityOriginData&, const String& name, bool createIfDoesNotExist);
 
     void addOpenDatabase(Database&);
@@ -117,7 +117,7 @@ public:
 private:
     explicit DatabaseTracker(const String& databasePath);
 
-    ExceptionOr<void> hasAdequateQuotaForOrigin(const SecurityOriginData&, unsigned estimatedSize);
+    ExceptionOr<void> hasAdequateQuotaForOrigin(const SecurityOriginData&, unsigned long long estimatedSize);
 
     bool hasEntryForOriginNoLock(const SecurityOriginData&);
     String fullPathForDatabaseNoLock(const SecurityOriginData&, const String& name, bool createIfDoesNotExist);