- Revision
- 226617
- Author
- [email protected]
- Date
- 2018-01-09 00:34:34 -0800 (Tue, 09 Jan 2018)
Log Message
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.
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (226616 => 226617)
--- trunk/LayoutTests/ChangeLog 2018-01-09 07:17:06 UTC (rev 226616)
+++ trunk/LayoutTests/ChangeLog 2018-01-09 08:34:34 UTC (rev 226617)
@@ -1,3 +1,20 @@
+2018-01-09 Ryosuke Niwa <[email protected]>
+
+ 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 <[email protected]>
::first-letter incorrectly selects grapheme pairs
Added: trunk/LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash-expected.txt (0 => 226617)
--- trunk/LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash-expected.txt 2018-01-09 08:34:34 UTC (rev 226617)
@@ -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
+
Added: trunk/LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash.html (0 => 226617)
--- trunk/LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash.html (rev 0)
+++ trunk/LayoutTests/http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash.html 2018-01-09 08:34:34 UTC (rev 226617)
@@ -0,0 +1,92 @@
+<!DOCTYPE html>
+<html>
+<body>
+<style>
+#container div { width: 10px; height: 10px; }
+</style>
+<script src=""
+<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 = '' + 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=""
+</body>
+</html>
Modified: trunk/LayoutTests/http/wpt/resource-timing/rt-performance-extensions.js (226616 => 226617)
--- trunk/LayoutTests/http/wpt/resource-timing/rt-performance-extensions.js 2018-01-09 07:17:06 UTC (rev 226616)
+++ trunk/LayoutTests/http/wpt/resource-timing/rt-performance-extensions.js 2018-01-09 08:34:34 UTC (rev 226617)
@@ -66,7 +66,7 @@
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 @@
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");
Modified: trunk/Source/WebCore/ChangeLog (226616 => 226617)
--- trunk/Source/WebCore/ChangeLog 2018-01-09 07:17:06 UTC (rev 226616)
+++ trunk/Source/WebCore/ChangeLog 2018-01-09 08:34:34 UTC (rev 226617)
@@ -1,3 +1,38 @@
+2018-01-09 Ryosuke Niwa <[email protected]>
+
+ 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 <[email protected]>
::first-letter incorrectly selects grapheme pairs
Modified: trunk/Source/WebCore/page/Performance.cpp (226616 => 226617)
--- trunk/Source/WebCore/page/Performance.cpp 2018-01-09 07:17:06 UTC (rev 226616)
+++ trunk/Source/WebCore/page/Performance.cpp 2018-01-09 08:34:34 UTC (rev 226617)
@@ -52,6 +52,7 @@
Performance::Performance(ScriptExecutionContext& context, MonotonicTime timeOrigin)
: ContextDestructionObserver(&context)
+ , m_resourceTimingBufferFullTimer(*this, &Performance::resourceTimingBufferFullTimerFired)
, m_timeOrigin(timeOrigin)
, m_performanceTimelineTaskQueue(context)
{
@@ -166,26 +167,41 @@
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 @@
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)
Modified: trunk/Source/WebCore/page/Performance.h (226616 => 226617)
--- trunk/Source/WebCore/page/Performance.h 2018-01-09 07:17:06 UTC (rev 226616)
+++ trunk/Source/WebCore/page/Performance.h 2018-01-09 08:34:34 UTC (rev 226617)
@@ -101,6 +101,7 @@
void derefEventTarget() final { deref(); }
bool isResourceTimingBufferFull() const;
+ void resourceTimingBufferFullTimerFired();
void queueEntry(PerformanceEntry&);
@@ -111,6 +112,13 @@
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;
