Indexed DB: Ensure large explicit keys consistently max out generator

inexorabletash · Commit bot · commit c4c73fb74bfe · 2017-03-07T22:31:34.000Z
Background: Stores can have a key generator which generates successive numeric keys. Storing a record with an explicit numeric key adjusts they key generator to produce values above the explicit key. Once the generator hits 2^53 it stops generating new keys (since that's the maximum integer uniquely representable as a JS number). Chrome's logic for certain values above this limit was "wonky". Values above 2^53 would max out the generator. Values above 2^63 and Infinity would be ignored and not adjust the generator, due to relying on undefined double->int64_t casting behavior. Fix to always max out the generator for large values. Also adds web-platform-tests - other implementations are wonky too. :( Also adds some missing test coverage for key injection cases. Spec discussion: w3c/IndexedDB#147 BUG=691754 Review-Url: https://codereview.chromium.org/2735213002 Cr-Commit-Position: refs/heads/master@{#455256}
diff --git a/content/browser/indexed_db/indexed_db_database.cc b/content/browser/indexed_db/indexed_db_database.cc
@@ -6,6 +6,7 @@
 
 #include <math.h>
 
+#include <algorithm>
 #include <limits>
 #include <set>
 
@@ -1183,13 +1184,11 @@ static std::unique_ptr<IndexedDBKey> GenerateKey(
     IndexedDBTransaction* transaction,
     int64_t database_id,
     int64_t object_store_id) {
-  const int64_t max_generator_value =
-      9007199254740992LL;  // Maximum integer storable as ECMAScript number.
+  // Maximum integer uniquely representable as ECMAScript number.
+  const int64_t max_generator_value = 9007199254740992LL;
   int64_t current_number;
   leveldb::Status s = backing_store->GetKeyGeneratorCurrentNumber(
-      transaction->BackingStoreTransaction(),
-      database_id,
-      object_store_id,
+      transaction->BackingStoreTransaction(), database_id, object_store_id,
       &current_number);
   if (!s.ok()) {
     LOG(ERROR) << "Failed to GetKeyGeneratorCurrentNumber";
@@ -1201,16 +1200,24 @@ static std::unique_ptr<IndexedDBKey> GenerateKey(
   return base::MakeUnique<IndexedDBKey>(current_number, WebIDBKeyTypeNumber);
 }
 
+// Called at the end of a "put" operation. The key is a number that was either
+// generated by the generator which now needs to be incremented (so
+// |check_current| is false) or was user-supplied so we only conditionally use
+// (and |check_current| is true).
 static leveldb::Status UpdateKeyGenerator(IndexedDBBackingStore* backing_store,
                                           IndexedDBTransaction* transaction,
                                           int64_t database_id,
                                           int64_t object_store_id,
                                           const IndexedDBKey& key,
                                           bool check_current) {
   DCHECK_EQ(WebIDBKeyTypeNumber, key.type());
+  // Maximum integer uniquely representable as ECMAScript number.
+  const double max_generator_value = 9007199254740992.0;
+  int64_t value = base::saturated_cast<int64_t>(
+      floor(std::min(key.number(), max_generator_value)));
   return backing_store->MaybeUpdateKeyGeneratorCurrentNumber(
       transaction->BackingStoreTransaction(), database_id, object_store_id,
-      static_cast<int64_t>(floor(key.number())) + 1, check_current);
+      value + 1, check_current);
 }
 
 struct IndexedDBDatabase::PutOperationParams {
diff --git a/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-explicit.html b/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-explicit.html
@@ -0,0 +1,146 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Key Generator behavior with explicit keys generator overflow</title>
+<link rel=help href="https://w3c.github.io/IndexedDB/#key-generator-construct">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="support.js"></script>
+<script>
+
+function big_key_test(key, description) {
+  indexeddb_test(
+    (t, db) => {
+      assert_equals(indexedDB.cmp(key, key), 0, 'Key is valid');
+
+      db.createObjectStore('store', {autoIncrement: true});
+    },
+    (t, db) => {
+      const tx = db.transaction('store', 'readwrite');
+      const store = tx.objectStore('store');
+      const value = 0;
+      let request;
+
+      request = store.put(value);
+      request.onerror = t.unreached_func('put should succeed');
+      request.onsuccess = t.step_func(e => {
+        assert_equals(e.target.result, 1,
+                      'Key generator should initially be 1');
+      });
+
+      request = store.put(value);
+      request.onerror = t.unreached_func('put should succeed');
+      request.onsuccess = t.step_func(e => {
+        assert_equals(e.target.result, 2,
+                      'Key generator should increment');
+      });
+
+      request = store.put(value, 1000);
+      request.onerror = t.unreached_func('put should succeed');
+      request.onsuccess = t.step_func(e => {
+        assert_equals(e.target.result, 1000,
+                      'Explicit key should be used');
+      });
+
+      request = store.put(value);
+      request.onerror = t.unreached_func('put should succeed');
+      request.onsuccess = t.step_func(e => {
+        assert_equals(e.target.result, 1001,
+                      'Key generator should have updated');
+      });
+
+      request = store.put(value, key);
+      request.onerror = t.unreached_func('put should succeed');
+      request.onsuccess = t.step_func(e => {
+        assert_equals(e.target.result, key,
+                      'Explicit key should be used');
+      });
+
+      if (key >= 0) {
+        // Large positive values will max out the key generator, so it
+        // can no longer produce keys.
+        request = store.put(value);
+        request.onsuccess = t.unreached_func('put should fail');
+        request.onerror = t.step_func(e => {
+          e.preventDefault();
+          assert_equals(e.target.error.name, 'ConstraintError',
+                        'Key generator should have returned failure');
+        });
+      } else {
+        // Large negative values are always lower than the key generator's
+        // current number, so have no effect on the generator.
+        request = store.put(value);
+        request.onerror = t.unreached_func('put should succeed');
+        request.onsuccess = t.step_func(e => {
+          assert_equals(e.target.result, 1002,
+                        'Key generator should have updated');
+        });
+      }
+
+      request = store.put(value, 2000);
+      request.onerror = t.unreached_func('put should succeed');
+      request.onsuccess = t.step_func(e => {
+        assert_equals(e.target.result, 2000,
+                      'Explicit key should be used');
+      });
+
+      tx.onabort = t.step_func(() => {
+        assert_unreached(`Transaction aborted: ${tx.error.message}`);
+      });
+      tx.oncomplete = t.step_func(() => { t.done(); });
+    },
+    description);
+}
+
+[
+  {
+    key: Number.MAX_SAFE_INTEGER + 1,
+    description: '53 bits'
+  },
+  {
+    key: Math.pow(2, 60),
+    description: 'greater than 53 bits, less than 64 bits'
+  },
+  {
+    key: -Math.pow(2, 60),
+    description: 'greater than 53 bits, less than 64 bits (negative)'
+  },
+  {
+    key: Math.pow(2, 63),
+    description: '63 bits'
+  },
+  {
+    key: -Math.pow(2, 63),
+    description: '63 bits (negative)'
+  },
+  {
+    key: Math.pow(2, 64),
+    description: '64 bits'
+  },
+  {
+    key: -Math.pow(2, 64),
+    description: '64 bits (negative)'
+  },
+  {
+    key: Math.pow(2, 70),
+    description: 'greater than 64 bits, but still finite'
+  },
+  {
+    key: -Math.pow(2, 70),
+    description: 'greater than 64 bits, but still finite (negative)'
+  },
+  {
+    key: Infinity,
+    description: 'equal to Infinity'
+  },
+  {
+    key: -Infinity,
+    description: 'equal to -Infinity'
+  }
+].forEach(function(testCase) {
+  big_key_test(testCase.key,
+               `Key generator vs. explicit key ${testCase.description}`);
+});
+
+
+
+</script>
diff --git a/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-inject.html b/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-inject.html
@@ -0,0 +1,119 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Key Generator behavior with explicit keys and value injection</title>
+<link rel=help href="https://w3c.github.io/IndexedDB/#inject-key-into-value">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="support.js"></script>
+<script>
+
+indexeddb_test(
+  (t, db) => {
+    db.createObjectStore('store', {autoIncrement: true, keyPath: 'id'});
+  },
+  (t, db) => {
+    const tx = db.transaction('store', 'readwrite');
+    t.onabort = t.unreached_func('transaction should not abort');
+
+    const store = tx.objectStore('store');
+
+    store.put({name: 'n'}).onsuccess = t.step_func(e => {
+      const key = e.target.result;
+      assert_equals(key, 1, 'Key generator initial value should be 1');
+      store.get(key).onsuccess = t.step_func(e => {
+        const value = e.target.result;
+        assert_equals(typeof value, 'object', 'Result should be object');
+        assert_equals(value.name, 'n', 'Result should have name property');
+        assert_equals(value.id, key, 'Key should be injected');
+        t.done();
+      });
+    });
+  },
+  'Key is injected into value - single segment path');
+
+indexeddb_test(
+  (t, db) => {
+    db.createObjectStore('store', {autoIncrement: true, keyPath: 'a.b.id'});
+  },
+  (t, db) => {
+    const tx = db.transaction('store', 'readwrite');
+    t.onabort = t.unreached_func('transaction should not abort');
+
+    const store = tx.objectStore('store');
+
+    store.put({name: 'n'}).onsuccess = t.step_func(e => {
+      const key = e.target.result;
+      assert_equals(key, 1, 'Key generator initial value should be 1');
+      store.get(key).onsuccess = t.step_func(e => {
+        const value = e.target.result;
+        assert_equals(typeof value, 'object', 'Result should be object');
+        assert_equals(value.name, 'n', 'Result should have name property');
+        assert_equals(value.a.b.id, key, 'Key should be injected');
+        t.done();
+      });
+    });
+  },
+  'Key is injected into value - multi-segment path');
+
+indexeddb_test(
+  (t, db) => {
+    db.createObjectStore('store', {autoIncrement: true, keyPath: 'a.b.id'});
+  },
+  (t, db) => {
+    const tx = db.transaction('store', 'readwrite');
+    t.onabort = t.unreached_func('transaction should not abort');
+
+    const store = tx.objectStore('store');
+
+    store.put({name: 'n1', b: {name: 'n2'}}).onsuccess = t.step_func(e => {
+      const key = e.target.result;
+      assert_equals(key, 1, 'Key generator initial value should be 1');
+      store.get(key).onsuccess = t.step_func(e => {
+        const value = e.target.result;
+        assert_equals(typeof value, 'object', 'Result should be object');
+        assert_equals(value.name, 'n1', 'Result should have name property');
+        assert_equals(value.b.name, 'n2', 'Result should have name property');
+        assert_equals(value.a.b.id, key, 'Key should be injected');
+        t.done();
+      });
+    });
+  },
+  'Key is injected into value - multi-segment path, partially populated');
+
+indexeddb_test(
+  (t, db) => {
+    db.createObjectStore('store', {autoIncrement: true, keyPath: 'id'});
+  },
+  (t, db) => {
+    const tx = db.transaction('store', 'readwrite');
+    const store = tx.objectStore('store');
+
+    assert_throws('DataError', () => {
+      store.put(123);
+    }, 'Key path should be checked against value');
+
+    t.done();
+  },
+  'put() throws if key cannot be injected - single segment path');
+
+indexeddb_test(
+  (t, db) => {
+    db.createObjectStore('store', {autoIncrement: true, keyPath: 'a.b.id'});
+  },
+  (t, db) => {
+    const tx = db.transaction('store', 'readwrite');
+    const store = tx.objectStore('store');
+
+    assert_throws('DataError', () => {
+      store.put({a: 123});
+    }, 'Key path should be checked against value');
+
+    assert_throws('DataError', () => {
+      store.put({a: {b: 123} });
+    }, 'Key path should be checked against value');
+
+    t.done();
+  },
+  'put() throws if key cannot be injected - multi-segment path');
+
+</script>
