Release assert in addResourceTiming when a cache resource is requested during style...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jan 2018 08:34:34 +0000 (08:34 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jan 2018 08:34:34 +0000 (08:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181137
<rdar://problem/35666574>

Reviewed by Simon Fraser.

Source/WebCore:

Make the dispatching of resourcetimingbufferfull event asynchronous to avoid dispatching it
synchronously during a style resolution when CachedResourceLoader::requestImage requests
a previously loaded image.

We now schedule a timer when the resource timing buffer becomes full, and dispatch the event
when the timer fires. Meanwhile, we have a backup buffer to which additional resource timing
entries would be added. Once the event is dispatched, we refill the buffer exposed to author
scripts. When refilling the buffer results in it becoming full again, we keep repeating the
process of firing resourcetimingbufferfull and re-filling the buffer until either we stop
making progress (i.e. the script didn't increase the number of empty entires in the buffer)
or the backup buffer (at the time we started this process) becomes empty.

Also fixed a bug that we were firing resourcetimingbufferfull event when the last entry that
fits within the buffer size was added instead of when an entry is being added to an already
full buffer. To make this work, the patch introduces m_resourceTimingBufferFullFlag,
representing the concept "resource timing buffer full" flag in the resource timing specification.

Test: http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash.html

* page/Performance.cpp:
(WebCore::Performance::Performance):
(WebCore::Performance::clearResourceTimings):
(WebCore::Performance::setResourceTimingBufferSize):
(WebCore::Performance::addResourceTiming):
(WebCore::Performance::resourceTimingBufferFullTimerFired):
* page/Performance.h:

LayoutTests:

Added a regression test for the crash.

Also fixed test cases in rt-performance-extensions.js which were incorrectly asserting and assuming that
resourcetimingbufferfull event will be fired when there are exactly the same number of entries as the buffer size.

* http/tests/performance/performance-resource-timing-resourcetimingbufferfull -crash-expected.txt: Added.
* http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash.html: Added.
* http/wpt/resource-timing/rt-performance-extensions.js: Fixed the test cases.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash.html [new file with mode: 0644]
LayoutTests/http/wpt/resource-timing/rt-performance-extensions.js
Source/WebCore/ChangeLog
Source/WebCore/page/Performance.cpp
Source/WebCore/page/Performance.h

index 66533a7..4684551 100644 (file)
@@ -1,3 +1,20 @@
+2018-01-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Release assert in addResourceTiming when a cache resource is requested during style recalc
+        https://bugs.webkit.org/show_bug.cgi?id=181137
+        <rdar://problem/35666574>
+
+        Reviewed by Simon Fraser.
+
+        Added a regression test for the crash.
+
+        Also fixed test cases in rt-performance-extensions.js which were incorrectly asserting and assuming that
+        resourcetimingbufferfull event will be fired when there are exactly the same number of entries as the buffer size.
+
+        * http/tests/performance/performance-resource-timing-resourcetimingbufferfull -crash-expected.txt: Added.
+        * http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash.html: Added.
+        * http/wpt/resource-timing/rt-performance-extensions.js: Fixed the test cases.
+
 2018-01-08  Chris Nardi  <csnardi1@gmail.com>
 
         ::first-letter incorrectly selects grapheme pairs
diff --git a/LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash-expected.txt b/LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash-expected.txt
new file mode 100644 (file)
index 0000000..63ab058
--- /dev/null
@@ -0,0 +1,36 @@
+This tests causing resource timing buffer to be full during a style recalc.
+WebKit should not hit a release assertion by dispatching an event synchronously.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+loadImagesInAnotherFrame(7)
+container.innerHTML = ""; resourcetimingbufferfullFired = false
+performance.setResourceTimingBufferSize(3); performance.clearResourceTimings()
+PASS performance.getEntriesByType("resource").length is 0
+divsWithBackgroundImages(7); container.getBoundingClientRect()
+PASS resources = performance.getEntriesByType("resource"); resources.length is 3
+PASS resources[0].initiatorType is "css"
+PASS new URL(resources[0].name).search is "?resource=0"
+PASS new URL(resources[1].name).search is "?resource=1"
+PASS new URL(resources[2].name).search is "?resource=2"
+PASS resourcetimingbufferfullEventCount is 0
+
+Inside resourcetimingbufferfull 1
+PASS performance.clearResourceTimings(); performance.getEntriesByType("resource").length is 0
+
+Inside resourcetimingbufferfull 2
+PASS resources = performance.getEntriesByType("resource"); resources.length is 3
+PASS new URL(resources[0].name).search is "?resource=3"
+PASS new URL(resources[1].name).search is "?resource=4"
+PASS new URL(resources[2].name).search is "?resource=5"
+PASS performance.clearResourceTimings(); performance.getEntriesByType("resource").length is 0
+
+After resourcetimingbufferfull
+PASS resources = performance.getEntriesByType("resource"); resources.length is 1
+PASS resources[0].initiatorType is "css"
+PASS new URL(resources[0].name).search is "?resource=6"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash.html b/LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash.html
new file mode 100644 (file)
index 0000000..dea81df
--- /dev/null
@@ -0,0 +1,92 @@
+<!DOCTYPE html>
+<html>
+<body>
+<style>
+#container div { width: 10px; height: 10px; }
+</style>
+<script src="/resources/js-test-pre.js"></script>
+<div id="container">
+</div>
+<script>
+
+description('This tests causing resource timing buffer to be full during a style recalc.<br>'
+    + 'WebKit should not hit a release assertion by dispatching an event synchronously.');
+
+function loadImagesInAnotherFrame(count)
+{
+    const iframe = document.createElement('iframe');
+    document.body.appendChild(iframe);
+    const doc = iframe.contentDocument;
+    let loadCount = 0;
+    for (let i = 0; i < count; i++) {
+        const image = doc.createElement('img');
+        image.src = '../../resources/square100.png?resource=' + i;
+        doc.body.appendChild(image);
+        image.onload = () => {
+            loadCount++;
+            if (loadCount == count)
+                startTest();
+        }
+    }
+}
+
+function startTest()
+{
+    evalAndLog('container.innerHTML = ""; resourcetimingbufferfullFired = false');
+    evalAndLog('performance.setResourceTimingBufferSize(3); performance.clearResourceTimings()');
+    shouldBe('performance.getEntriesByType("resource").length', '0');
+    evalAndLog('divsWithBackgroundImages(7); container.getBoundingClientRect()');
+    shouldBe('resources = performance.getEntriesByType("resource"); resources.length', '3');
+    shouldBeEqualToString('resources[0].initiatorType', 'css');
+    shouldBeEqualToString('new URL(resources[0].name).search', '?resource=0');
+    shouldBeEqualToString('new URL(resources[1].name).search', '?resource=1');
+    shouldBeEqualToString('new URL(resources[2].name).search', '?resource=2');
+    shouldBe('resourcetimingbufferfullEventCount', '0');
+}
+
+function divsWithBackgroundImages(count)
+{
+    const container = document.getElementById('container');
+    for (let i = 0; i < count; i++) {
+        const div = document.createElement('div');
+        div.style.backgroundImage = `url('../../resources/square100.png?resource=${i}')`;
+        container.appendChild(div);
+    }
+}
+
+setTimeout(finishJSTest, 3000);
+
+let resourcetimingbufferfullEventCount = 0;
+performance.onresourcetimingbufferfull = () => {
+    resourcetimingbufferfullEventCount++;
+    debug('');
+    debug('Inside resourcetimingbufferfull ' + resourcetimingbufferfullEventCount);
+    if (resourcetimingbufferfullEventCount == 2) {
+        shouldBe('resources = performance.getEntriesByType("resource"); resources.length', '3');
+        shouldBeEqualToString('new URL(resources[0].name).search', '?resource=3');
+        shouldBeEqualToString('new URL(resources[1].name).search', '?resource=4');
+        shouldBeEqualToString('new URL(resources[2].name).search', '?resource=5');
+    }
+    shouldBe('performance.clearResourceTimings(); performance.getEntriesByType("resource").length', '0');
+
+    if (resourcetimingbufferfullEventCount < 2)
+        return;
+
+    setTimeout(() => {
+        debug('');
+        debug('After resourcetimingbufferfull');
+        shouldBe('resources = performance.getEntriesByType("resource"); resources.length', '1');
+        shouldBeEqualToString('resources[0].initiatorType', 'css');
+        shouldBeEqualToString('new URL(resources[0].name).search', '?resource=6');
+        finishJSTest();
+    }, 0);
+}
+
+evalAndLog('loadImagesInAnotherFrame(7)');
+
+var jsTestIsAsync = true;
+
+</script>
+<script src="/resources/js-test-post.js"></script>
+</body>
+</html>
index 075a9a0..0ba50aa 100644 (file)
@@ -66,7 +66,7 @@ promise_test(function(t) {
     return loadResources(3).then(function(entries) {
         assert_equals(entries.length, 3, "context should have observed 3 resources");
         assert_equals(performance.getEntriesByType("resource").length, 3, "context global buffer should be full at 3 resources");
-        assert_true(bufferFullEventDispatched, "context should not have dispatched buffer full event");
+        assert_false(bufferFullEventDispatched, "context should not have dispatched buffer full event");
     });
 }, "resourcetimingbufferfull event should not trigger if exactly the BufferSizeLimit number of resources are added to the buffer", {timeout: 3000});
 
@@ -98,8 +98,8 @@ promise_test(function(t) {
         bufferFullEvent = event;
     };
 
-    return loadResources(1).then(function(entries) {
-        assert_equals(entries.length, 1, "context should have observed 1 resource");
+    return loadResources(2).then(function(entries) {
+        assert_equals(entries.length, 2, "context should have observed 1 resource");
         assert_equals(performance.getEntriesByType("resource").length, 1, "context global buffer should be full at 1 resource");
         assert_equals(bufferFullEvent.target, performance, "event should dispatch at the performance object");
         assert_true(bufferFullEvent.bubbles, "event should bubble");
index 8543556..f7d0815 100644 (file)
@@ -1,3 +1,38 @@
+2018-01-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Release assert in addResourceTiming when a cache resource is requested during style recalc
+        https://bugs.webkit.org/show_bug.cgi?id=181137
+        <rdar://problem/35666574>
+
+        Reviewed by Simon Fraser.
+
+        Make the dispatching of resourcetimingbufferfull event asynchronous to avoid dispatching it
+        synchronously during a style resolution when CachedResourceLoader::requestImage requests
+        a previously loaded image.
+
+        We now schedule a timer when the resource timing buffer becomes full, and dispatch the event
+        when the timer fires. Meanwhile, we have a backup buffer to which additional resource timing
+        entries would be added. Once the event is dispatched, we refill the buffer exposed to author
+        scripts. When refilling the buffer results in it becoming full again, we keep repeating the
+        process of firing resourcetimingbufferfull and re-filling the buffer until either we stop
+        making progress (i.e. the script didn't increase the number of empty entires in the buffer)
+        or the backup buffer (at the time we started this process) becomes empty.
+
+        Also fixed a bug that we were firing resourcetimingbufferfull event when the last entry that
+        fits within the buffer size was added instead of when an entry is being added to an already
+        full buffer. To make this work, the patch introduces m_resourceTimingBufferFullFlag,
+        representing the concept "resource timing buffer full" flag in the resource timing specification.
+
+        Test: http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash.html
+
+        * page/Performance.cpp:
+        (WebCore::Performance::Performance):
+        (WebCore::Performance::clearResourceTimings):
+        (WebCore::Performance::setResourceTimingBufferSize):
+        (WebCore::Performance::addResourceTiming):
+        (WebCore::Performance::resourceTimingBufferFullTimerFired):
+        * page/Performance.h:
+
 2018-01-08  Chris Nardi  <csnardi1@gmail.com>
 
         ::first-letter incorrectly selects grapheme pairs
index 8166551..e745cd6 100644 (file)
@@ -52,6 +52,7 @@ namespace WebCore {
 
 Performance::Performance(ScriptExecutionContext& context, MonotonicTime timeOrigin)
     : ContextDestructionObserver(&context)
+    , m_resourceTimingBufferFullTimer(*this, &Performance::resourceTimingBufferFullTimerFired)
     , m_timeOrigin(timeOrigin)
     , m_performanceTimelineTaskQueue(context)
 {
@@ -166,26 +167,41 @@ Vector<RefPtr<PerformanceEntry>> Performance::getEntriesByName(const String& nam
 void Performance::clearResourceTimings()
 {
     m_resourceTimingBuffer.clear();
+    m_resourceTimingBufferFullFlag = false;
 }
 
 void Performance::setResourceTimingBufferSize(unsigned size)
 {
     m_resourceTimingBufferSize = size;
+    m_resourceTimingBufferFullFlag = false;
 }
 
 void Performance::addResourceTiming(ResourceTiming&& resourceTiming)
 {
-    RefPtr<PerformanceResourceTiming> entry = PerformanceResourceTiming::create(m_timeOrigin, WTFMove(resourceTiming));
+    auto entry = PerformanceResourceTiming::create(m_timeOrigin, WTFMove(resourceTiming));
 
-    queueEntry(*entry);
+    if (m_waitingForBackupBufferToBeProcessed) {
+        m_backupResourceTimingBuffer.append(WTFMove(entry));
+        return;
+    }
 
-    if (isResourceTimingBufferFull())
+    if (m_resourceTimingBufferFullFlag) {
+        // We fired resourcetimingbufferfull evnet but the author script didn't clear the buffer.
+        // Notify performance observers but don't add it to the buffer.
+        queueEntry(entry.get());
         return;
+    }
 
-    m_resourceTimingBuffer.append(entry);
+    if (isResourceTimingBufferFull()) {
+        ASSERT(!m_resourceTimingBufferFullTimer.isActive());
+        m_backupResourceTimingBuffer.append(WTFMove(entry));
+        m_waitingForBackupBufferToBeProcessed = true;
+        m_resourceTimingBufferFullTimer.startOneShot(0_s);
+        return;
+    }
 
-    if (isResourceTimingBufferFull())
-        dispatchEvent(Event::create(eventNames().resourcetimingbufferfullEvent, true, false));
+    queueEntry(entry.get());
+    m_resourceTimingBuffer.append(WTFMove(entry));
 }
 
 bool Performance::isResourceTimingBufferFull() const
@@ -193,6 +209,39 @@ bool Performance::isResourceTimingBufferFull() const
     return m_resourceTimingBuffer.size() >= m_resourceTimingBufferSize;
 }
 
+void Performance::resourceTimingBufferFullTimerFired()
+{
+    while (!m_backupResourceTimingBuffer.isEmpty()) {
+        auto backupBuffer = WTFMove(m_backupResourceTimingBuffer);
+
+        m_resourceTimingBufferFullFlag = true;
+        dispatchEvent(Event::create(eventNames().resourcetimingbufferfullEvent, true, false));
+
+        RELEASE_ASSERT(m_resourceTimingBufferSize >= m_resourceTimingBuffer.size());
+        unsigned remainingBufferSize = m_resourceTimingBufferSize - m_resourceTimingBuffer.size();
+        bool bufferIsStillFullAfterDispatchingEvent = !remainingBufferSize;
+        if (bufferIsStillFullAfterDispatchingEvent) {
+            for (auto& entry : backupBuffer)
+                queueEntry(*entry);
+            // Dispatching resourcetimingbufferfull event may have inserted more entries.
+            for (auto& entry : m_backupResourceTimingBuffer)
+                queueEntry(*entry);
+            break;
+        }
+
+        unsigned i = 0;
+        for (auto& entry : backupBuffer) {
+            if (i < remainingBufferSize) {
+                m_resourceTimingBuffer.append(entry.copyRef());
+                queueEntry(*entry);
+            } else
+                m_backupResourceTimingBuffer.append(entry.copyRef());
+            i++;
+        }
+    }
+    m_waitingForBackupBufferToBeProcessed = false;
+}
+
 ExceptionOr<void> Performance::mark(const String& markName)
 {
     if (!m_userTiming)
index 438a001..24ab94b 100644 (file)
@@ -101,6 +101,7 @@ private:
     void derefEventTarget() final { deref(); }
 
     bool isResourceTimingBufferFull() const;
+    void resourceTimingBufferFullTimerFired();
 
     void queueEntry(PerformanceEntry&);
 
@@ -111,6 +112,13 @@ private:
     Vector<RefPtr<PerformanceEntry>> m_resourceTimingBuffer;
     unsigned m_resourceTimingBufferSize { 150 };
 
+    Timer m_resourceTimingBufferFullTimer;
+    Vector<RefPtr<PerformanceEntry>> m_backupResourceTimingBuffer;
+
+    // https://w3c.github.io/resource-timing/#dfn-resource-timing-buffer-full-flag
+    bool m_resourceTimingBufferFullFlag { false };
+    bool m_waitingForBackupBufferToBeProcessed { false };
+
     MonotonicTime m_timeOrigin;
 
     std::unique_ptr<UserTiming> m_userTiming;