Attachment 348188 Details for Bug 188728 – Rebased

[patch] Rebased

bug-188728-20180827123641.patch (text/plain), 14.58 KB, created by youenn fablet on 2018-08-27 12:36:42 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2018-08-27 12:36:42 PDT

Size: 14.58 KB

patch

obsolete

>Subversion Revision: 235368
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index c781a535526bea95d38b185f459b3f1d0a812b59..376135ca411951da75a24ce51d4fcbf536844813 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,31 @@
>+2018-08-27  Youenn Fablet  <youenn@apple.com>
>+
>+        Various IndexDB tests abandon documents
>+        https://bugs.webkit.org/show_bug.cgi?id=188728
>+        <rdar://problem/43651095>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Some IDB objects implement hasPendingActivity but there are some possibilities that they continue returning true after being stopped.
>+        This is the case for requests that get stopped while still waiting for some pending activity.
>+        This is also the case for requests that emits upgradenneeded or blocked events.
>+
>+        Enforce that these objects return false to hasPendingActivity once being stopped.
>+        This ensures that they can be garbage collected once their context is preparing for destruction like in Document::prepareForDestruction.
>+
>+        Test: http/tests/IndexedDB/collect-IDB-objects.https.html
>+
>+        * Modules/indexeddb/IDBIndex.cpp:
>+        (WebCore::IDBIndex::hasPendingActivity const):
>+        * Modules/indexeddb/IDBObjectStore.cpp:
>+        (WebCore::IDBObjectStore::hasPendingActivity const):
>+        * Modules/indexeddb/IDBRequest.cpp:
>+        (WebCore::IDBRequest::hasPendingActivity const):
>+        (WebCore::IDBRequest::enqueueEvent):
>+        * Modules/indexeddb/IDBTransaction.cpp:
>+        (WebCore::IDBTransaction::notifyDidAbort):
>+        In case the context is stopped, IDBTransaction should not ask IDBRequest to fire an event.
>+
> 2018-08-27  Alex Christensen  <achristensen@webkit.org>
> 
>         Fix IOSMAC build
>diff --git a/Source/WebCore/Modules/indexeddb/IDBIndex.cpp b/Source/WebCore/Modules/indexeddb/IDBIndex.cpp
>index 1919e469418c05c8ae7723ed66111d9a3e45516b..9d1f83937737ae80110b020ea9983b68d803f67f 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBIndex.cpp
>+++ b/Source/WebCore/Modules/indexeddb/IDBIndex.cpp
>@@ -69,7 +69,7 @@ bool IDBIndex::canSuspendForDocumentSuspension() const
> 
> bool IDBIndex::hasPendingActivity() const
> {
>-    return !m_objectStore.transaction().isFinished();
>+    return m_objectStore.transaction().hasPendingActivity();
> }
> 
> const String& IDBIndex::name() const
>diff --git a/Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp b/Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp
>index 3c50ebcd69b7a3c3fb84a6a3a4d315ad62d43612..f08c9e52853f30b0878eeffd0db5d1702f231f10 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp
>+++ b/Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp
>@@ -82,7 +82,7 @@ bool IDBObjectStore::canSuspendForDocumentSuspension() const
> 
> bool IDBObjectStore::hasPendingActivity() const
> {
>-    return !m_transaction.isFinished();
>+    return m_transaction.hasPendingActivity();
> }
> 
> const String& IDBObjectStore::name() const
>diff --git a/Source/WebCore/Modules/indexeddb/IDBRequest.cpp b/Source/WebCore/Modules/indexeddb/IDBRequest.cpp
>index 35932860a57ef8d62afd15237708daf50d530662..6f5ce0e972653f6573ad5db76a4fccd49f22da9b 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBRequest.cpp
>+++ b/Source/WebCore/Modules/indexeddb/IDBRequest.cpp
>@@ -261,7 +261,7 @@ bool IDBRequest::canSuspendForDocumentSuspension() const
> bool IDBRequest::hasPendingActivity() const
> {
>     ASSERT(&originThread() == &Thread::current() || mayBeGCThread());
>-    return m_hasPendingActivity;
>+    return !m_contextStopped && m_hasPendingActivity;
> }
> 
> void IDBRequest::stop()
>@@ -284,7 +284,7 @@ void IDBRequest::cancelForStop()
> void IDBRequest::enqueueEvent(Ref<Event>&& event)
> {
>     ASSERT(&originThread() == &Thread::current());
>-    if (!scriptExecutionContext() || m_contextStopped)
>+    if (m_contextStopped)
>         return;
> 
>     event->setTarget(this);
>diff --git a/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp b/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp
>index 61215843a13265904b73d310bf4f3ffec9bba080..8cf062b514e1d743ac821fadc9600fb34ea1d8a7 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp
>+++ b/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp
>@@ -551,7 +551,7 @@ void IDBTransaction::notifyDidAbort(const IDBError& error)
>     m_idbError = error;
>     fireOnAbort();
> 
>-    if (isVersionChange()) {
>+    if (isVersionChange() && !m_contextStopped) {
>         ASSERT(m_openDBRequest);
>         m_openDBRequest->fireErrorAfterVersionChangeCompletion();
>     }
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index cd12c8d77fabcb9d392c865c33ce6e3cac11d54c..6ddef5c2332456025c5322fc90a9890916c714ee 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-08-27  Youenn Fablet  <youenn@apple.com>
>+
>+        Various IndexDB tests abandon documents
>+        https://bugs.webkit.org/show_bug.cgi?id=188728
>+        <rdar://problem/43651095>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * http/tests/IndexedDB/collect-IDB-objects.https-expected.txt: Added.
>+        * http/tests/IndexedDB/collect-IDB-objects.https.html: Added.
>+        * http/tests/IndexedDB/resources/myidbframe.htm: Added.
>+        * http/tests/IndexedDB/resources/support.js: Added.
>+
> 2018-08-27  Per Arne Vollan  <pvollan@apple.com>
> 
>         Layout Test fast/events/dblclick-event-getModifierState.html is failing
>diff --git a/LayoutTests/http/tests/IndexedDB/collect-IDB-objects.https-expected.txt b/LayoutTests/http/tests/IndexedDB/collect-IDB-objects.https-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..ec03587d2c3e6934ee7bc90fc965d1c7df44fdfa
>--- /dev/null
>+++ b/LayoutTests/http/tests/IndexedDB/collect-IDB-objects.https-expected.txt
>@@ -0,0 +1,4 @@
>+
>+
>+PASS Ensuring frame document gets collected after being stopped in the mmiddle of IDB operations 
>+
>diff --git a/LayoutTests/http/tests/IndexedDB/collect-IDB-objects.https.html b/LayoutTests/http/tests/IndexedDB/collect-IDB-objects.https.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..47dda8dee4cb53fc63e510d9f5570504f8d733a1
>--- /dev/null
>+++ b/LayoutTests/http/tests/IndexedDB/collect-IDB-objects.https.html
>@@ -0,0 +1,45 @@
>+<!DOCTYPE html>
>+<meta charset="utf-8">
>+<script src="/resources/testharness.js"></script>
>+<script src="/resources/testharnessreport.js"></script>
>+<script>
>+function waitFor(duration)
>+{
>+    return new Promise((resolve) => setTimeout(resolve, duration));
>+}
>+
>+var resolveCallback, rejectCallback;
>+var promise = new Promise((resolve, reject) => {
>+    resolveCallback = resolve;
>+    rejectCallback = reject;
>+});
>+
>+async function done()
>+{
>+    try {
>+        const frameIdentifier = internals.documentIdentifier(iframe.contentDocument);
>+        iframe.src = "non-existent-frame";
>+        let counter = 0;
>+        while (++counter < 10) {
>+            if (!internals.isDocumentAlive(frameIdentifier)) {
>+                resolveCallback();
>+                return;
>+            }
>+            if (window.GCController)
>+                GCController.collect();
>+
>+            await waitFor(50);
>+        }
>+    } finally {
>+        rejectCallback("Test failed");
>+    }
>+}
>+
>+promise_test((test) => {
>+    if (!window.internals)
>+        rejectCallback("Test require internals API");
>+    return promise;
>+}, "Ensuring frame document gets collected after being stopped in the mmiddle of IDB operations");
>+
>+</script>
>+<iframe src="resources/myidbframe.htm" id="iframe"></iframe>
>diff --git a/LayoutTests/http/tests/IndexedDB/resources/myidbframe.htm b/LayoutTests/http/tests/IndexedDB/resources/myidbframe.htm
>new file mode 100644
>index 0000000000000000000000000000000000000000..f907f7297af960f62aff218555911796f4bf01ea
>--- /dev/null
>+++ b/LayoutTests/http/tests/IndexedDB/resources/myidbframe.htm
>@@ -0,0 +1,26 @@
>+<!DOCTYPE html>
>+<script src="../../resources/testharness.js"></script>
>+<script src="support.js"></script>
>+
>+<script>
>+var t = async_test();
>+createdb(t).onupgradeneeded = function(e) {
>+    e.target.result
>+            .createObjectStore("store")
>+            .add(new Date(), 1);
>+
>+    e.target.onsuccess = t.step_func(function(e) {
>+        e.target.result
>+                .transaction("store")
>+                .objectStore("store")
>+                .get(1)
>+                .onsuccess = t.step_func(function(e)
>+        {
>+            t.done();
>+            parent.done();
>+        });
>+    });
>+};
>+</script>
>+
>+<div id="log"></div>
>diff --git a/LayoutTests/http/tests/IndexedDB/resources/support.js b/LayoutTests/http/tests/IndexedDB/resources/support.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..5edbdacbcc7d0cf27d0e3330198b55d50540345e
>--- /dev/null
>+++ b/LayoutTests/http/tests/IndexedDB/resources/support.js
>@@ -0,0 +1,194 @@
>+var databaseName = "database";
>+var databaseVersion = 1;
>+
>+/* Delete created databases
>+ *
>+ * Go through each finished test, see if it has an associated database. Close
>+ * that and delete the database. */
>+add_completion_callback(function(tests)
>+{
>+    for (var i in tests)
>+    {
>+        if(tests[i].db)
>+        {
>+            tests[i].db.close();
>+            self.indexedDB.deleteDatabase(tests[i].db.name);
>+        }
>+    }
>+});
>+
>+function fail(test, desc) {
>+    return test.step_func(function(e) {
>+        if (e && e.message && e.target.error)
>+            assert_unreached(desc + " (" + e.target.error.name + ": " + e.message + ")");
>+        else if (e && e.message)
>+            assert_unreached(desc + " (" + e.message + ")");
>+        else if (e && e.target.readyState === 'done' && e.target.error)
>+            assert_unreached(desc + " (" + e.target.error.name + ")");
>+        else
>+            assert_unreached(desc);
>+    });
>+}
>+
>+function createdb(test, dbname, version)
>+{
>+    var rq_open = createdb_for_multiple_tests(dbname, version);
>+    return rq_open.setTest(test);
>+}
>+
>+function createdb_for_multiple_tests(dbname, version) {
>+    var rq_open,
>+        fake_open = {},
>+        test = null,
>+        dbname = (dbname ? dbname : "testdb-" + new Date().getTime() + Math.random() );
>+
>+    if (version)
>+        rq_open = self.indexedDB.open(dbname, version);
>+    else
>+        rq_open = self.indexedDB.open(dbname);
>+
>+    function auto_fail(evt, current_test) {
>+        /* Fail handlers, if we haven't set on/whatever/, don't
>+         * expect to get event whatever. */
>+        rq_open.manually_handled = {};
>+
>+        rq_open.addEventListener(evt, function(e) {
>+            if (current_test !== test) {
>+                return;
>+            }
>+
>+            test.step(function() {
>+                if (!rq_open.manually_handled[evt]) {
>+                    assert_unreached("unexpected open." + evt + " event");
>+                }
>+
>+                if (e.target.result + '' == '[object IDBDatabase]' &&
>+                    !this.db) {
>+                  this.db = e.target.result;
>+
>+                  this.db.onerror = fail(test, 'unexpected db.error');
>+                  this.db.onabort = fail(test, 'unexpected db.abort');
>+                  this.db.onversionchange =
>+                      fail(test, 'unexpected db.versionchange');
>+                }
>+            });
>+        });
>+        rq_open.__defineSetter__("on" + evt, function(h) {
>+            rq_open.manually_handled[evt] = true;
>+            if (!h)
>+                rq_open.addEventListener(evt, function() {});
>+            else
>+                rq_open.addEventListener(evt, test.step_func(h));
>+        });
>+    }
>+
>+    // add a .setTest method to the IDBOpenDBRequest object
>+    Object.defineProperty(rq_open, 'setTest', {
>+        enumerable: false,
>+        value: function(t) {
>+            test = t;
>+
>+            auto_fail("upgradeneeded", test);
>+            auto_fail("success", test);
>+            auto_fail("blocked", test);
>+            auto_fail("error", test);
>+
>+            return this;
>+        }
>+    });
>+
>+    return rq_open;
>+}
>+
>+function assert_key_equals(actual, expected, description) {
>+  assert_equals(indexedDB.cmp(actual, expected), 0, description);
>+}
>+
>+function indexeddb_test(upgrade_func, open_func, description, options) {
>+  async_test(function(t) {
>+    options = Object.assign({upgrade_will_abort: false}, options);
>+    var dbname = location + '-' + t.name;
>+    var del = indexedDB.deleteDatabase(dbname);
>+    del.onerror = t.unreached_func('deleteDatabase should succeed');
>+    var open = indexedDB.open(dbname, 1);
>+    open.onupgradeneeded = t.step_func(function() {
>+      var db = open.result;
>+      t.add_cleanup(function() {
>+        // If open didn't succeed already, ignore the error.
>+        open.onerror = function(e) {
>+          e.preventDefault();
>+        };
>+        db.close();
>+        indexedDB.deleteDatabase(db.name);
>+      });
>+      var tx = open.transaction;
>+      upgrade_func(t, db, tx, open);
>+    });
>+    if (options.upgrade_will_abort) {
>+      open.onsuccess = t.unreached_func('open should not succeed');
>+    } else {
>+      open.onerror = t.unreached_func('open should succeed');
>+      open.onsuccess = t.step_func(function() {
>+        var db = open.result;
>+        if (open_func)
>+          open_func(t, db, open);
>+      });
>+    }
>+  }, description);
>+}
>+
>+// Call with a Test and an array of expected results in order. Returns
>+// a function; call the function when a result arrives and when the
>+// expected number appear the order will be asserted and test
>+// completed.
>+function expect(t, expected) {
>+  var results = [];
>+  return result => {
>+    results.push(result);
>+    if (results.length === expected.length) {
>+      assert_array_equals(results, expected);
>+      t.done();
>+    }
>+  };
>+}
>+
>+// Checks to see if the passed transaction is active (by making
>+// requests against the named store).
>+function is_transaction_active(tx, store_name) {
>+  try {
>+    const request = tx.objectStore(store_name).get(0);
>+    request.onerror = e => {
>+      e.preventDefault();
>+      e.stopPropagation();
>+    };
>+    return true;
>+  } catch (ex) {
>+    assert_equals(ex.name, 'TransactionInactiveError',
>+                  'Active check should either not throw anything, or throw ' +
>+                  'TransactionInactiveError');
>+    return false;
>+  }
>+}
>+
>+// Keep the passed transaction alive indefinitely (by making requests
>+// against the named store). Returns a function to to let the
>+// transaction finish, and asserts that the transaction is not yet
>+// finished.
>+function keep_alive(tx, store_name) {
>+  let completed = false;
>+  tx.addEventListener('complete', () => { completed = true; });
>+
>+  let pin = true;
>+
>+  function spin() {
>+    if (!pin)
>+      return;
>+    tx.objectStore(store_name).get(0).onsuccess = spin;
>+  }
>+  spin();
>+
>+  return () => {
>+    assert_false(completed, 'Transaction completed while kept alive');
>+    pin = false;
>+  };
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 188728: 348018 | 348023 | 348031 | 348188 | 348226