- Revision
- 241512
- Author
- bshaf...@apple.com
- Date
- 2019-02-14 00:34:58 -0800 (Thu, 14 Feb 2019)
Log Message
Cherry-pick r241499. rdar://problem/48065634
Crash in DOMTimer::fired
https://bugs.webkit.org/show_bug.cgi?id=194638
Reviewed by Brent Fulgham.
Source/WebCore:
This patch continues the saga of hunting down timer related crashes after r239814, r225985, r227934.
The crash was caused by the bug that we don't remove a DOMTimer from NestedTimersMap if a DOMTimer
is created & installed inside another DOMTimer's callback (via execute call in DOMTimer::fired).
Fixed the crash by using a Ref in NestedTimersMap. This will keep the timer alive until we exit
from DOMTimer::fired. Because DOMTimer::fired always calls stopTracking() which clears the map
we would not leak these DOM timers.
We could, alternatively, use WeakPtr in NestedTimersMap but that would unnecessarily increase the
size of DOMTimer for a very marginal benefit of DOMTimer objcets being deleted slightly earlier.
Deleting itself in DOMTimer's destructor involves more logic & house keeping in the timer code,
and is no longer the preferred approach when dealing with these classes of bugs in WebKit.
Test: fast/dom/timer-destruction-during-firing.html
* page/DOMTimer.cpp:
(WebCore::NestedTimersMap::add):
(WebCore::DOMTimer::install):
(WebCore::DOMTimer::fired):
LayoutTests:
Added a regression test. It needs debug assertions without the fix.
* fast/dom/timer-destruction-during-firing-expected.txt: Added.
* fast/dom/timer-destruction-during-firing.html: Added.
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@241499 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Added Paths
Diff
Modified: branches/safari-607-branch/LayoutTests/ChangeLog (241511 => 241512)
--- branches/safari-607-branch/LayoutTests/ChangeLog 2019-02-14 08:34:54 UTC (rev 241511)
+++ branches/safari-607-branch/LayoutTests/ChangeLog 2019-02-14 08:34:58 UTC (rev 241512)
@@ -1,5 +1,59 @@
2019-02-13 Babak Shafiei <bshaf...@apple.com>
+ Cherry-pick r241499. rdar://problem/48065634
+
+ Crash in DOMTimer::fired
+ https://bugs.webkit.org/show_bug.cgi?id=194638
+
+ Reviewed by Brent Fulgham.
+
+ Source/WebCore:
+
+ This patch continues the saga of hunting down timer related crashes after r239814, r225985, r227934.
+
+ The crash was caused by the bug that we don't remove a DOMTimer from NestedTimersMap if a DOMTimer
+ is created & installed inside another DOMTimer's callback (via execute call in DOMTimer::fired).
+
+ Fixed the crash by using a Ref in NestedTimersMap. This will keep the timer alive until we exit
+ from DOMTimer::fired. Because DOMTimer::fired always calls stopTracking() which clears the map
+ we would not leak these DOM timers.
+
+ We could, alternatively, use WeakPtr in NestedTimersMap but that would unnecessarily increase the
+ size of DOMTimer for a very marginal benefit of DOMTimer objcets being deleted slightly earlier.
+ Deleting itself in DOMTimer's destructor involves more logic & house keeping in the timer code,
+ and is no longer the preferred approach when dealing with these classes of bugs in WebKit.
+
+ Test: fast/dom/timer-destruction-during-firing.html
+
+ * page/DOMTimer.cpp:
+ (WebCore::NestedTimersMap::add):
+ (WebCore::DOMTimer::install):
+ (WebCore::DOMTimer::fired):
+
+ LayoutTests:
+
+ Added a regression test. It needs debug assertions without the fix.
+
+ * fast/dom/timer-destruction-during-firing-expected.txt: Added.
+ * fast/dom/timer-destruction-during-firing.html: Added.
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@241499 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-02-13 Ryosuke Niwa <rn...@webkit.org>
+
+ Crash in DOMTimer::fired
+ https://bugs.webkit.org/show_bug.cgi?id=194638
+
+ Reviewed by Brent Fulgham.
+
+ Added a regression test. It needs debug assertions without the fix.
+
+ * fast/dom/timer-destruction-during-firing-expected.txt: Added.
+ * fast/dom/timer-destruction-during-firing.html: Added.
+
+2019-02-13 Babak Shafiei <bshaf...@apple.com>
+
Cherry-pick r241484. rdar://problem/48065620
Entering fullscreen inside a shadow root will not set fullscreen pseudoclasses outside of root
Added: branches/safari-607-branch/LayoutTests/fast/dom/timer-destruction-during-firing-expected.txt (0 => 241512)
--- branches/safari-607-branch/LayoutTests/fast/dom/timer-destruction-during-firing-expected.txt (rev 0)
+++ branches/safari-607-branch/LayoutTests/fast/dom/timer-destruction-during-firing-expected.txt 2019-02-14 08:34:58 UTC (rev 241512)
@@ -0,0 +1,3 @@
+This tests deleting DOMTimer inside another DOMTimer. WebKit should not hit any debug assertions.
+
+PASS
Added: branches/safari-607-branch/LayoutTests/fast/dom/timer-destruction-during-firing.html (0 => 241512)
--- branches/safari-607-branch/LayoutTests/fast/dom/timer-destruction-during-firing.html (rev 0)
+++ branches/safari-607-branch/LayoutTests/fast/dom/timer-destruction-during-firing.html 2019-02-14 08:34:58 UTC (rev 241512)
@@ -0,0 +1,44 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests deleting DOMTimer inside another DOMTimer. WebKit should not hit any debug assertions.</p>
+<p id="result"></p>
+<script src=""
+<script>
+
+if (!window.testRunner)
+ document.getElementById('result').textContent = 'This test requires testRunner';
+else {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+
+ setTimeout(() => {
+ for (let k = 0; k < 50; k++) {
+ const frames = [];
+ for (let i = 0; i < 1; i++)
+ frames[i] = createTimerInNewFrame();
+ for (const frame of frames)
+ frame.remove();
+ frames.length = 0;
+ gc();
+ }
+ self.postMessage('end', '*');
+ }, 0);
+
+ window._onmessage_ = () => {
+ document.getElementById('result').textContent = 'PASS';
+ testRunner.notifyDone();
+ }
+}
+
+function createTimerInNewFrame()
+{
+ const frame = document.createElement('iframe');
+ document.body.appendChild(frame);
+ frame.contentWindow.setTimeout(() => {}, 0);
+ return frame;
+}
+
+</script>
+</body>
+</html>
Modified: branches/safari-607-branch/Source/WebCore/ChangeLog (241511 => 241512)
--- branches/safari-607-branch/Source/WebCore/ChangeLog 2019-02-14 08:34:54 UTC (rev 241511)
+++ branches/safari-607-branch/Source/WebCore/ChangeLog 2019-02-14 08:34:58 UTC (rev 241512)
@@ -1,5 +1,75 @@
2019-02-13 Babak Shafiei <bshaf...@apple.com>
+ Cherry-pick r241499. rdar://problem/48065634
+
+ Crash in DOMTimer::fired
+ https://bugs.webkit.org/show_bug.cgi?id=194638
+
+ Reviewed by Brent Fulgham.
+
+ Source/WebCore:
+
+ This patch continues the saga of hunting down timer related crashes after r239814, r225985, r227934.
+
+ The crash was caused by the bug that we don't remove a DOMTimer from NestedTimersMap if a DOMTimer
+ is created & installed inside another DOMTimer's callback (via execute call in DOMTimer::fired).
+
+ Fixed the crash by using a Ref in NestedTimersMap. This will keep the timer alive until we exit
+ from DOMTimer::fired. Because DOMTimer::fired always calls stopTracking() which clears the map
+ we would not leak these DOM timers.
+
+ We could, alternatively, use WeakPtr in NestedTimersMap but that would unnecessarily increase the
+ size of DOMTimer for a very marginal benefit of DOMTimer objcets being deleted slightly earlier.
+ Deleting itself in DOMTimer's destructor involves more logic & house keeping in the timer code,
+ and is no longer the preferred approach when dealing with these classes of bugs in WebKit.
+
+ Test: fast/dom/timer-destruction-during-firing.html
+
+ * page/DOMTimer.cpp:
+ (WebCore::NestedTimersMap::add):
+ (WebCore::DOMTimer::install):
+ (WebCore::DOMTimer::fired):
+
+ LayoutTests:
+
+ Added a regression test. It needs debug assertions without the fix.
+
+ * fast/dom/timer-destruction-during-firing-expected.txt: Added.
+ * fast/dom/timer-destruction-during-firing.html: Added.
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@241499 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-02-13 Ryosuke Niwa <rn...@webkit.org>
+
+ Crash in DOMTimer::fired
+ https://bugs.webkit.org/show_bug.cgi?id=194638
+
+ Reviewed by Brent Fulgham.
+
+ This patch continues the saga of hunting down timer related crashes after r239814, r225985, r227934.
+
+ The crash was caused by the bug that we don't remove a DOMTimer from NestedTimersMap if a DOMTimer
+ is created & installed inside another DOMTimer's callback (via execute call in DOMTimer::fired).
+
+ Fixed the crash by using a Ref in NestedTimersMap. This will keep the timer alive until we exit
+ from DOMTimer::fired. Because DOMTimer::fired always calls stopTracking() which clears the map
+ we would not leak these DOM timers.
+
+ We could, alternatively, use WeakPtr in NestedTimersMap but that would unnecessarily increase the
+ size of DOMTimer for a very marginal benefit of DOMTimer objcets being deleted slightly earlier.
+ Deleting itself in DOMTimer's destructor involves more logic & house keeping in the timer code,
+ and is no longer the preferred approach when dealing with these classes of bugs in WebKit.
+
+ Test: fast/dom/timer-destruction-during-firing.html
+
+ * page/DOMTimer.cpp:
+ (WebCore::NestedTimersMap::add):
+ (WebCore::DOMTimer::install):
+ (WebCore::DOMTimer::fired):
+
+2019-02-13 Babak Shafiei <bshaf...@apple.com>
+
Cherry-pick r241494. rdar://problem/48065624
AX: Crash in handleMenuOpen
Modified: branches/safari-607-branch/Source/WebCore/page/DOMTimer.cpp (241511 => 241512)
--- branches/safari-607-branch/Source/WebCore/page/DOMTimer.cpp 2019-02-14 08:34:54 UTC (rev 241511)
+++ branches/safari-607-branch/Source/WebCore/page/DOMTimer.cpp 2019-02-14 08:34:58 UTC (rev 241512)
@@ -113,7 +113,7 @@
DOMTimerFireState* DOMTimerFireState::current = nullptr;
struct NestedTimersMap {
- typedef HashMap<int, DOMTimer*>::const_iterator const_iterator;
+ typedef HashMap<int, Ref<DOMTimer>>::const_iterator const_iterator;
static NestedTimersMap* instanceForContext(ScriptExecutionContext& context)
{
@@ -139,10 +139,10 @@
nestedTimers.clear();
}
- void add(int timeoutId, DOMTimer* timer)
+ void add(int timeoutId, Ref<DOMTimer>&& timer)
{
if (isTrackingNestedTimers)
- nestedTimers.add(timeoutId, timer);
+ nestedTimers.add(timeoutId, WTFMove(timer));
}
void remove(int timeoutId)
@@ -162,7 +162,7 @@
}
static bool isTrackingNestedTimers;
- HashMap<int /* timeoutId */, DOMTimer*> nestedTimers;
+ HashMap<int /* timeoutId */, Ref<DOMTimer>> nestedTimers;
};
bool NestedTimersMap::isTrackingNestedTimers = false;
@@ -235,7 +235,7 @@
// Keep track of nested timer installs.
if (NestedTimersMap* nestedTimers = NestedTimersMap::instanceForContext(context))
- nestedTimers->add(timer->m_timeoutId, timer);
+ nestedTimers->add(timer->m_timeoutId, *timer);
return timer->m_timeoutId;
}
@@ -390,8 +390,8 @@
// Check if we should throttle nested single-shot timers.
if (nestedTimers) {
- for (auto& keyValue : *nestedTimers) {
- auto* timer = keyValue.value;
+ for (auto& idAndTimer : *nestedTimers) {
+ auto& timer = idAndTimer.value;
if (timer->isActive() && !timer->repeatInterval())
timer->updateThrottlingStateIfNecessary(fireState);
}
