Attachment 348018 Details for Bug 188728 – Patch

[patch] Patch

bug-188728-20180824095235.patch (text/plain), 16.92 KB, created by youenn fablet on 2018-08-24 09:52:36 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2018-08-24 09:52:36 PDT

Size: 16.92 KB

patch

obsolete

>Subversion Revision: 235230
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 0de3a382ebd95646aa5f3b738796e8394ee14371..4c9d5727defccd373aab46400fa785d911e14304 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,35 @@
>+2018-08-24  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. 
>+        Enforce that these objects return false to hasPendingActivity once being stopped.
>+        This ensures that they can be garbage collected once their document is preparing for destruction.
>+
>+        Add an internals API to access to existing documents of a process.
>+        This allows verifying that documents do get destroyed when stopping IDB requests in the middle of a pending activity.\ 
>+
>+        Test: http/tests/IndexedDB/value.https.html
>+
>+        * Modules/indexeddb/IDBCursor.cpp:
>+        (WebCore::IDBCursor::hasPendingActivity const):
>+        * Modules/indexeddb/IDBCursor.h:
>+        * 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):
>+        * testing/Internals.cpp:
>+        (WebCore::Internals::documentURLs):
>+        * testing/Internals.h:
>+        * testing/Internals.idl:
>+
> 2018-08-23  Youenn Fablet  <youenn@apple.com>
> 
>         Update libwebrtc up to 984f1a80c0
>diff --git a/Source/WebCore/Modules/indexeddb/IDBCursor.cpp b/Source/WebCore/Modules/indexeddb/IDBCursor.cpp
>index 05db83417e5c01c9567a52c02b29dc2760a370c0..71303a1e8cbbf2f3ac987e4f24a9af5acf5718ba 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBCursor.cpp
>+++ b/Source/WebCore/Modules/indexeddb/IDBCursor.cpp
>@@ -369,7 +369,7 @@ bool IDBCursor::canSuspendForDocumentSuspension() const
> 
> bool IDBCursor::hasPendingActivity() const
> {
>-    return m_outstandingRequestCount;
>+    return !m_contextStopped && m_outstandingRequestCount;
> }
> 
> void IDBCursor::decrementOutstandingRequestCount()
>diff --git a/Source/WebCore/Modules/indexeddb/IDBCursor.h b/Source/WebCore/Modules/indexeddb/IDBCursor.h
>index 49030569911a77979eaddd934ae0480c42841886..d7e65e47cd53f347afe49ed53e01db38148eae79 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBCursor.h
>+++ b/Source/WebCore/Modules/indexeddb/IDBCursor.h
>@@ -85,6 +85,7 @@ protected:
> private:
>     const char* activeDOMObjectName() const final;
>     bool canSuspendForDocumentSuspension() const final;
>+    void stop() final { m_contextStopped = true; }
> 
>     bool sourcesDeleted() const;
>     IDBObjectStore& effectiveObjectStore() const;
>@@ -101,6 +102,7 @@ private:
>     IDBRequest* m_request { nullptr };
> 
>     bool m_gotValue { false };
>+    bool m_contextStopped { false };
> 
>     IDBKeyData m_currentKeyData;
>     IDBKeyData m_currentPrimaryKeyData;
>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 2c27dfd5f8060455ef1771b1dc98d933a14b9a61..031ebc1edb235615981af5abce8e84852ffea0f5 100644
>--- a/Source/WebCore/Modules/indexeddb/IDBRequest.cpp
>+++ b/Source/WebCore/Modules/indexeddb/IDBRequest.cpp
>@@ -266,7 +266,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()
>@@ -289,7 +289,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/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
>index 9ba1d274a2add63ad5b623d2abdc0381e289dee7..42532ae50cef9defa7e7c1b12c209a94da204669 100644
>--- a/Source/WebCore/testing/Internals.cpp
>+++ b/Source/WebCore/testing/Internals.cpp
>@@ -4706,6 +4706,14 @@ size_t Internals::pluginCount()
>     return contextDocument()->page()->pluginData().webVisiblePlugins().size();
> }
> 
>+Vector<String> Internals::documentURLs()
>+{
>+    Vector<String> urls;
>+    for (const auto* document : Document::allDocuments())
>+        urls.append(document->url().string());
>+    return urls;
>+}
>+
> void Internals::notifyResourceLoadObserver()
> {
>     ResourceLoadObserver::shared().notifyObserver();
>diff --git a/Source/WebCore/testing/Internals.h b/Source/WebCore/testing/Internals.h
>index 8199ea4c67d24e4456cd7d3d2c5b4b0f732c20d9..efdc8dec451ef1e98a9bd1411ba4bcc3adb21a98 100644
>--- a/Source/WebCore/testing/Internals.h
>+++ b/Source/WebCore/testing/Internals.h
>@@ -730,6 +730,7 @@ public:
>     void setUseSystemAppearance(bool);
> 
>     size_t pluginCount();
>+    Vector<String> documentURLs();
> 
>     void notifyResourceLoadObserver();
> 
>diff --git a/Source/WebCore/testing/Internals.idl b/Source/WebCore/testing/Internals.idl
>index 04f77ac7fc9c67c6b3279a3c2441e08ae40b041b..8e53d755ddd7e89239caf9eec6e78fb68bdc2d06 100644
>--- a/Source/WebCore/testing/Internals.idl
>+++ b/Source/WebCore/testing/Internals.idl
>@@ -665,6 +665,7 @@ enum CompositingPolicy {
>     void setUseSystemAppearance(boolean value);
> 
>     unsigned long pluginCount();
>+    sequence<DOMString> documentURLs();
> 
>     void notifyResourceLoadObserver();
> };
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index ae73e3124ab525bbeb35ae60adb6ab3333810b61..6ddcba9d2087ca7efcc2fee517c7ff3ead452edc 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-08-24  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/resources/myidbframe.htm: Added.
>+        * http/tests/IndexedDB/resources/support.js: Added.
>+        * http/tests/IndexedDB/value.https-expected.txt: Added.
>+        * http/tests/IndexedDB/value.https.html: Added.
>+
> 2018-08-23  Youenn Fablet  <youenn@apple.com>
> 
>         Update libwebrtc up to 984f1a80c0
>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;
>+  };
>+}
>diff --git a/LayoutTests/http/tests/IndexedDB/value.https-expected.txt b/LayoutTests/http/tests/IndexedDB/value.https-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..ec03587d2c3e6934ee7bc90fc965d1c7df44fdfa
>--- /dev/null
>+++ b/LayoutTests/http/tests/IndexedDB/value.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/value.https.html b/LayoutTests/http/tests/IndexedDB/value.https.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..ad4008dcd256c3f214ea116f3695eab657fd3f2b
>--- /dev/null
>+++ b/LayoutTests/http/tests/IndexedDB/value.https.html
>@@ -0,0 +1,47 @@
>+<!DOCTYPE html>
>+<meta charset="utf-8">
>+<title>Values</title>
>+<link rel="author" href="mailto:odinho@opera.com" title="Odin HÃ¸rthe Omdal">
>+<script src="../resources/gc.js"></script>
>+<script src="../resources/testharness.js"></script>
>+<script src="../resources/testharnessreport.js"></script>
>+<script>
>+var resolveCallback, rejectCallback;
>+var promise = new Promise((resolve, reject) => {
>+    resolveCallback = resolve;
>+    rejectCallback = reject;
>+});
>+
>+function waitFor(duration)
>+{
>+    return new Promise((resolve) => setTimeout(resolve, duration));
>+}
>+
>+async function done()
>+{
>+    try {
>+        iframe.src = "non-existent-frame";
>+        let counter = 0;
>+        while (++counter < 10) {
>+            if (window.internals.documentURLs().join(' ').indexOf('myidbframe.htm') === -1) {
>+                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>

Actions: View | Formatted Diff | Diff

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