Title: [252504] trunk
- Revision
- 252504
- Author
- rn...@webkit.org
- Date
- 2019-11-15 15:05:51 -0800 (Fri, 15 Nov 2019)
Log Message
JS wrappers of scroll event targets can get prematurely collected by GC
https://bugs.webkit.org/show_bug.cgi?id=204219
Reviewed by Keith Miller.
Source/WebCore:
This patch addresses the bug that the JS wrappers of the pending scroll event targets can be
collected by GC before scroll events are fired. This bug has always existed but it's worse after
r252205 because there is more of a time delay between an element is scrolled in RenderLayer sense
and when the corresponding scroll event is fired on the element.
Fixed the bug by using GCReachableRef to store the pending scroll event targets introduced in r252205
to keep the JS wrappers of those elements & document alive.
Test: fast/scrolling/scrolling-event-target-gc.html
* dom/Document.cpp:
(WebCore::Document::PendingScrollEventTargetList): Added. Wraps Vector<GCReachableRef<ContainerNode>>
to avoid including GCReachableRef.h in Document.h.
(WebCore::Document::commonTeardown): Clear m_pendingScrollEventTargetList to avoid document leaks.
(WebCore::Document::addPendingScrollEventTarget):
(WebCore::Document::runScrollSteps):
* dom/Document.h:
LayoutTests:
Added a regression test for adding elements as pending scroll event targets,
and removing them from the document before scroll events are fired.
* fast/scrolling/scrolling-event-target-gc-expected.txt: Added.
* fast/scrolling/scrolling-event-target-gc.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (252503 => 252504)
--- trunk/LayoutTests/ChangeLog 2019-11-15 23:00:19 UTC (rev 252503)
+++ trunk/LayoutTests/ChangeLog 2019-11-15 23:05:51 UTC (rev 252504)
@@ -1,3 +1,16 @@
+2019-11-14 Ryosuke Niwa <rn...@webkit.org>
+
+ JS wrappers of scroll event targets can get prematurely collected by GC
+ https://bugs.webkit.org/show_bug.cgi?id=204219
+
+ Reviewed by Keith Miller.
+
+ Added a regression test for adding elements as pending scroll event targets,
+ and removing them from the document before scroll events are fired.
+
+ * fast/scrolling/scrolling-event-target-gc-expected.txt: Added.
+ * fast/scrolling/scrolling-event-target-gc.html: Added.
+
2019-11-15 Pablo Saavedra <psaave...@igalia.com>
[WPE][GTK] Gardening after r252487
Added: trunk/LayoutTests/fast/scrolling/scrolling-event-target-gc-expected.txt (0 => 252504)
--- trunk/LayoutTests/fast/scrolling/scrolling-event-target-gc-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/scrolling/scrolling-event-target-gc-expected.txt 2019-11-15 23:05:51 UTC (rev 252504)
@@ -0,0 +1,5 @@
+This tests scheduling a scroll event on an element then removing the element.
+WebKit should not collect its JS wrappers.
+
+PASS
+
Added: trunk/LayoutTests/fast/scrolling/scrolling-event-target-gc.html (0 => 252504)
--- trunk/LayoutTests/fast/scrolling/scrolling-event-target-gc.html (rev 0)
+++ trunk/LayoutTests/fast/scrolling/scrolling-event-target-gc.html 2019-11-15 23:05:51 UTC (rev 252504)
@@ -0,0 +1,69 @@
+<!DOCTYPE html><!-- webkit-test-runner [ internal:AsyncOverflowScrollingEnabled=true ] -->
+<html>
+<body>
+<style>
+.container > div { width: 10px; height: 10px; overflow: scroll; }
+</style>
+<p>This tests scheduling a scroll event on an element then removing the element.<br>WebKit should not collect its JS wrappers.</p>
+<pre id="result"></pre>
+<div class="container"></div>
+<script src=""
+<script>
+
+if (window.testRunner) {
+ testRunner.waitUntilDone();
+ testRunner.dumpAsText();
+}
+
+const logs = [];
+let currentNumber = 0;
+function scheduleScrollEvents(count)
+{
+ let container = document.querySelector('.container');
+ for (let i = 0; i < count; i++) {
+ const div = document.createElement('div');
+ div.innerHTML = '<p>hello, world<br>this is a test</p>';
+ container.appendChild(div);
+ div.liveNumber = currentNumber;
+ div.addEventListener('scroll', log);
+ currentNumber++;
+ }
+
+ for (const div of container.children)
+ div.scrollTo(10, 10);
+
+ container.textContent = '';
+}
+
+function log()
+{
+ logs.push(this.liveNumber);
+}
+
+function checkLogs()
+{
+ let failed = false;
+ for (let i = 0; i < logs.length; ++i) {
+ if (logs[i] != i) {
+ result.textContent += `FAIL at ${i}: ${logs[i]}\n`;
+ failed = true;
+ }
+ }
+ if (!failed)
+ result.textContent = 'PASS';
+}
+
+for (let i = 0; i < 5; ++i) {
+ scheduleScrollEvents(20);
+ gc();
+}
+
+requestAnimationFrame(() => {
+ checkLogs();
+ if (window.testRunner)
+ testRunner.notifyDone();
+});
+
+</script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (252503 => 252504)
--- trunk/Source/WebCore/ChangeLog 2019-11-15 23:00:19 UTC (rev 252503)
+++ trunk/Source/WebCore/ChangeLog 2019-11-15 23:05:51 UTC (rev 252504)
@@ -1,3 +1,28 @@
+2019-11-14 Ryosuke Niwa <rn...@webkit.org>
+
+ JS wrappers of scroll event targets can get prematurely collected by GC
+ https://bugs.webkit.org/show_bug.cgi?id=204219
+
+ Reviewed by Keith Miller.
+
+ This patch addresses the bug that the JS wrappers of the pending scroll event targets can be
+ collected by GC before scroll events are fired. This bug has always existed but it's worse after
+ r252205 because there is more of a time delay between an element is scrolled in RenderLayer sense
+ and when the corresponding scroll event is fired on the element.
+
+ Fixed the bug by using GCReachableRef to store the pending scroll event targets introduced in r252205
+ to keep the JS wrappers of those elements & document alive.
+
+ Test: fast/scrolling/scrolling-event-target-gc.html
+
+ * dom/Document.cpp:
+ (WebCore::Document::PendingScrollEventTargetList): Added. Wraps Vector<GCReachableRef<ContainerNode>>
+ to avoid including GCReachableRef.h in Document.h.
+ (WebCore::Document::commonTeardown): Clear m_pendingScrollEventTargetList to avoid document leaks.
+ (WebCore::Document::addPendingScrollEventTarget):
+ (WebCore::Document::runScrollSteps):
+ * dom/Document.h:
+
2019-11-15 Jer Noble <jer.no...@apple.com>
Unreviewed Mojave build fix after r252501; wrap calls to AVContentKeyRequest.options in
Modified: trunk/Source/WebCore/dom/Document.cpp (252503 => 252504)
--- trunk/Source/WebCore/dom/Document.cpp 2019-11-15 23:00:19 UTC (rev 252503)
+++ trunk/Source/WebCore/dom/Document.cpp 2019-11-15 23:05:51 UTC (rev 252504)
@@ -78,6 +78,7 @@
#include "FrameLoaderClient.h"
#include "FrameView.h"
#include "FullscreenManager.h"
+#include "GCReachableRef.h"
#include "GenericCachedHTMLCollection.h"
#include "HTMLAllCollection.h"
#include "HTMLAnchorElement.h"
@@ -363,6 +364,14 @@
bool m_disallowLayout { false };
};
+// Defined here to avoid including GCReachableRef.h in Document.h
+struct Document::PendingScrollEventTargetList {
+ WTF_MAKE_FAST_ALLOCATED;
+
+public:
+ Vector<GCReachableRef<ContainerNode>> targets;
+};
+
#if ENABLE(INTERSECTION_OBSERVER)
static const Seconds intersectionObserversInitialUpdateDelay { 2000_ms };
#endif
@@ -759,6 +768,8 @@
accessSVGExtensions().pauseAnimations();
clearScriptedAnimationController();
+
+ m_pendingScrollEventTargetList = nullptr;
}
Element* Document::elementForAccessKey(const String& key)
@@ -4003,13 +4014,17 @@
void Document::addPendingScrollEventTarget(ContainerNode& target)
{
- if (m_pendingScrollEventTargets.contains(&target))
+ if (!m_pendingScrollEventTargetList)
+ m_pendingScrollEventTargetList = makeUnique<PendingScrollEventTargetList>();
+
+ auto& targets = m_pendingScrollEventTargetList->targets;
+ if (targets.findMatching([&] (auto& entry) { return entry.ptr() == ⌖ }) != notFound)
return;
- if (m_pendingScrollEventTargets.isEmpty())
+ if (targets.isEmpty())
scheduleTimedRenderingUpdate();
- m_pendingScrollEventTargets.append(makeWeakPtr(target));
+ targets.append(target);
}
void Document::setNeedsVisualViewportScrollEvent()
@@ -4023,16 +4038,12 @@
void Document::runScrollSteps()
{
// FIXME: The order of dispatching is not specified: https://github.com/WICG/visual-viewport/issues/66.
- if (!m_pendingScrollEventTargets.isEmpty()) {
+ if (m_pendingScrollEventTargetList && !m_pendingScrollEventTargetList->targets.isEmpty()) {
LOG_WITH_STREAM(Events, stream << "Document" << this << "sending scroll events to pending scroll event targets");
- auto currentTargets = WTFMove(m_pendingScrollEventTargets);
- for (auto target : currentTargets) {
- auto protectedTarget = makeRefPtr(target.get());
- ASSERT(protectedTarget);
- if (!protectedTarget)
- continue;
- auto bubbles = protectedTarget->isDocumentNode() ? Event::CanBubble::Yes : Event::CanBubble::No;
- protectedTarget->dispatchEvent(Event::create(eventNames().scrollEvent, bubbles, Event::IsCancelable::No));
+ auto currentTargets = WTFMove(m_pendingScrollEventTargetList->targets);
+ for (auto& target : currentTargets) {
+ auto bubbles = target->isDocumentNode() ? Event::CanBubble::Yes : Event::CanBubble::No;
+ target->dispatchEvent(Event::create(eventNames().scrollEvent, bubbles, Event::IsCancelable::No));
}
}
if (m_needsVisualViewportScrollEvent) {
Modified: trunk/Source/WebCore/dom/Document.h (252503 => 252504)
--- trunk/Source/WebCore/dom/Document.h 2019-11-15 23:00:19 UTC (rev 252503)
+++ trunk/Source/WebCore/dom/Document.h 2019-11-15 23:05:51 UTC (rev 252504)
@@ -2041,7 +2041,8 @@
bool m_isTelephoneNumberParsingAllowed { true };
#endif
- Vector<WeakPtr<ContainerNode>> m_pendingScrollEventTargets;
+ struct PendingScrollEventTargetList;
+ std::unique_ptr<PendingScrollEventTargetList> m_pendingScrollEventTargetList;
#if ENABLE(MEDIA_STREAM)
HashSet<HTMLMediaElement*> m_mediaStreamStateChangeElements;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
