Title: [287689] trunk/Source/WebCore
- Revision
- 287689
- Author
- cdu...@apple.com
- Date
- 2022-01-06 09:00:10 -0800 (Thu, 06 Jan 2022)
Log Message
Potential high CPU usage on macrumors.com
https://bugs.webkit.org/show_bug.cgi?id=234867
<rdar://85382450>
Reviewed by Geoffrey Garen.
The issue would occur when a suspended EventLoopTaskGroup had pending tasks and was getting marked
as ready to stop. The "ready to stop" concept was introduced in r269227 so that we would only stop
event loop groups for a given origin once all groups in that origin are ready to stop. This was so
that promises from detached iframes could resolve when scripted from another frame. The bug here
was that an EventLoopTaskGroup could move from "Suspended" state (a mode in which its tasks are not
executed) and into a "ReadyToStop" state (a mode in which its tasks are executed until all groups
in its origin are ready to stop). As a result, we would take a EventLoopGroup out of suspended
state unintentionally. This was causing a rescheduling loop in DeferredPromise::callFunction()
which would cause high CPU usage. Because activeDOMObjectsAreSuspended() returns true,
callFunction() would not call the JS function and would instead schedule a task on the event loop
to do so. The exception here is that the event loop group is suspended too and thus the scheduled
task will not run until the ActiveDOMObjects (and the event loop group) get resumed (i.e. page is
restored from the back/forward cache). However, due to the bug above, the event loop group was
unexpected NOT suspended and would run the scheduled task on the next runloop iteration and
callFunction() would just keep rescheduling itself in a loop.
To address the issue, we now cancel all tasks in the group and stop it permanently when its state
moves from "suspended" to "ready to stop". The state moves from "suspended" to "ready to stop"
when a CachedPage gets destroyed. As this point, the tasks in this group will never run since
we're suspended and we'll never get out of suspension (ready to stop).
* bindings/js/JSDOMPromiseDeferred.cpp:
(WebCore::DeferredPromise::callFunction):
* dom/EventLoop.cpp:
(WebCore::EventLoop::resumeGroup):
(WebCore::EventLoop::run):
(WebCore::EventLoop::clearAllTasks):
* dom/EventLoop.h:
(WebCore::EventLoopTaskGroup::markAsReadyToStop):
(WebCore::EventLoopTaskGroup::isStoppedPermanently const):
(WebCore::EventLoopTaskGroup::isSuspended const):
(WebCore::EventLoopTaskGroup::isStoppedPermanently): Deleted.
(WebCore::EventLoopTaskGroup::isSuspended): Deleted.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (287688 => 287689)
--- trunk/Source/WebCore/ChangeLog 2022-01-06 16:40:39 UTC (rev 287688)
+++ trunk/Source/WebCore/ChangeLog 2022-01-06 17:00:10 UTC (rev 287689)
@@ -1,3 +1,45 @@
+2022-01-06 Chris Dumez <cdu...@apple.com>
+
+ Potential high CPU usage on macrumors.com
+ https://bugs.webkit.org/show_bug.cgi?id=234867
+ <rdar://85382450>
+
+ Reviewed by Geoffrey Garen.
+
+ The issue would occur when a suspended EventLoopTaskGroup had pending tasks and was getting marked
+ as ready to stop. The "ready to stop" concept was introduced in r269227 so that we would only stop
+ event loop groups for a given origin once all groups in that origin are ready to stop. This was so
+ that promises from detached iframes could resolve when scripted from another frame. The bug here
+ was that an EventLoopTaskGroup could move from "Suspended" state (a mode in which its tasks are not
+ executed) and into a "ReadyToStop" state (a mode in which its tasks are executed until all groups
+ in its origin are ready to stop). As a result, we would take a EventLoopGroup out of suspended
+ state unintentionally. This was causing a rescheduling loop in DeferredPromise::callFunction()
+ which would cause high CPU usage. Because activeDOMObjectsAreSuspended() returns true,
+ callFunction() would not call the JS function and would instead schedule a task on the event loop
+ to do so. The exception here is that the event loop group is suspended too and thus the scheduled
+ task will not run until the ActiveDOMObjects (and the event loop group) get resumed (i.e. page is
+ restored from the back/forward cache). However, due to the bug above, the event loop group was
+ unexpected NOT suspended and would run the scheduled task on the next runloop iteration and
+ callFunction() would just keep rescheduling itself in a loop.
+
+ To address the issue, we now cancel all tasks in the group and stop it permanently when its state
+ moves from "suspended" to "ready to stop". The state moves from "suspended" to "ready to stop"
+ when a CachedPage gets destroyed. As this point, the tasks in this group will never run since
+ we're suspended and we'll never get out of suspension (ready to stop).
+
+ * bindings/js/JSDOMPromiseDeferred.cpp:
+ (WebCore::DeferredPromise::callFunction):
+ * dom/EventLoop.cpp:
+ (WebCore::EventLoop::resumeGroup):
+ (WebCore::EventLoop::run):
+ (WebCore::EventLoop::clearAllTasks):
+ * dom/EventLoop.h:
+ (WebCore::EventLoopTaskGroup::markAsReadyToStop):
+ (WebCore::EventLoopTaskGroup::isStoppedPermanently const):
+ (WebCore::EventLoopTaskGroup::isSuspended const):
+ (WebCore::EventLoopTaskGroup::isStoppedPermanently): Deleted.
+ (WebCore::EventLoopTaskGroup::isSuspended): Deleted.
+
2022-01-06 Jean-Yves Avenard <j...@apple.com>
Content Filtering should be using SharedBuffer instead of copying its data
Modified: trunk/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp (287688 => 287689)
--- trunk/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp 2022-01-06 16:40:39 UTC (rev 287688)
+++ trunk/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp 2022-01-06 17:00:10 UTC (rev 287689)
@@ -55,6 +55,7 @@
if (activeDOMObjectsAreSuspended()) {
JSC::Strong<JSC::Unknown, ShouldStrongDestructorGrabLock::Yes> strongResolution(lexicalGlobalObject.vm(), resolution);
+ ASSERT(scriptExecutionContext()->eventLoop().isSuspended());
scriptExecutionContext()->eventLoop().queueTask(TaskSource::Networking, [this, protectedThis = Ref { *this }, mode, strongResolution = WTFMove(strongResolution)]() mutable {
if (shouldIgnoreRequestToFulfill())
return;
Modified: trunk/Source/WebCore/dom/EventLoop.cpp (287688 => 287689)
--- trunk/Source/WebCore/dom/EventLoop.cpp 2022-01-06 16:40:39 UTC (rev 287688)
+++ trunk/Source/WebCore/dom/EventLoop.cpp 2022-01-06 17:00:10 UTC (rev 287689)
@@ -54,7 +54,7 @@
void EventLoop::resumeGroup(EventLoopTaskGroup& group)
{
ASSERT(isContextThread());
- if (!m_groupsWithSuspenedTasks.contains(group))
+ if (!m_groupsWithSuspendedTasks.contains(group))
return;
scheduleToRunIfNeeded();
}
@@ -107,7 +107,7 @@
if (!m_tasks.isEmpty()) {
auto tasks = std::exchange(m_tasks, { });
- m_groupsWithSuspenedTasks.clear();
+ m_groupsWithSuspendedTasks.clear();
Vector<std::unique_ptr<EventLoopTask>> remainingTasks;
for (auto& task : tasks) {
auto* group = task->group();
@@ -115,7 +115,7 @@
continue;
if (group->isSuspended()) {
- m_groupsWithSuspenedTasks.add(*group);
+ m_groupsWithSuspendedTasks.add(*group);
remainingTasks.append(WTFMove(task));
continue;
}
@@ -137,7 +137,7 @@
void EventLoop::clearAllTasks()
{
m_tasks.clear();
- m_groupsWithSuspenedTasks.clear();
+ m_groupsWithSuspendedTasks.clear();
}
void EventLoopTaskGroup::queueTask(std::unique_ptr<EventLoopTask>&& task)
Modified: trunk/Source/WebCore/dom/EventLoop.h (287688 => 287689)
--- trunk/Source/WebCore/dom/EventLoop.h 2022-01-06 16:40:39 UTC (rev 287688)
+++ trunk/Source/WebCore/dom/EventLoop.h 2022-01-06 17:00:10 UTC (rev 287689)
@@ -96,7 +96,7 @@
// Use a global queue instead of multiple task queues since HTML5 spec allows UA to pick arbitrary queue.
Vector<std::unique_ptr<EventLoopTask>> m_tasks;
WeakHashSet<EventLoopTaskGroup> m_associatedGroups;
- WeakHashSet<EventLoopTaskGroup> m_groupsWithSuspenedTasks;
+ WeakHashSet<EventLoopTaskGroup> m_groupsWithSuspendedTasks;
bool m_isScheduledToRun { false };
};
@@ -136,9 +136,17 @@
if (isReadyToStop() || isStoppedPermanently())
return;
+ bool wasSuspended = isSuspended();
m_state = State::ReadyToStop;
if (auto* eventLoop = m_eventLoop.get())
eventLoop->stopAssociatedGroupsIfNecessary();
+
+ if (wasSuspended && !isStoppedPermanently()) {
+ // We we get marked as ready to stop while suspended (happens when a CachedPage gets destroyed) then the
+ // queued tasks will never be able to run (since tasks don't run while suspended and we will never resume).
+ // As a result, we can simply discard our tasks and stop permanently.
+ stopAndDiscardAllTasks();
+ }
}
// This gets called by the event loop when all groups in the EventLoop as ready to stop.
@@ -168,8 +176,8 @@
eventLoop->resumeGroup(*this);
}
- bool isStoppedPermanently() { return m_state == State::Stopped; }
- bool isSuspended() { return m_state == State::Suspended; }
+ bool isStoppedPermanently() const { return m_state == State::Stopped; }
+ bool isSuspended() const { return m_state == State::Suspended; }
bool isReadyToStop() const { return m_state == State::ReadyToStop; }
void queueTask(std::unique_ptr<EventLoopTask>&&);
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
