Cherry-pick 430e2dd31ad1. rdar://138799302
Cherry-pick c8d323b1851e. rdar://139301982
REGRESSION (283084@main): Document::visibilityStateChanged does not hold reference to callback clients
rdar://138799302
https://bugs.webkit.org/show_bug.cgi?id=282360
Reviewed by Tim Nguyen, Ryosuke Niwa, and Chris Dumez.
Document::visibilityStateChanged() invokes visibility state callback clients, but does not
hold a reference to them before invoking. The client could then accidentally free itself
and cause an UAF. One possible route that leads to an UAF is through ViewTransition,
which the test case demonstrates:
* The ViewTransition C++ objects are allocated by document.startViewTransition().
After the call, each object has a ref count of at least 2 (one in the JS wrapper
that wraps the C++ object, one in Document::m_activeViewTransition)
* The GC is invoked, which releases the JS wrappers and decreases the ref count to 1
* The document visibility state is changed. This invokes ViewTransition::visibilityStateChanged
on each object, which calls ::skipViewTransition, which calls ::clearViewTransition.
::clearViewTransition sets Document::m_activeViewTransition to null, so the object ref
count is 0 and it's deallocated. ::clearViewTransition then continues to modify the
(already deallocated) object, leading to an UAF.
Fix this by holding a reference to the callback clients before invoking it. This involves
making VisibilityChangeClient ref counted. Then Document::visibilityStateChanged()
would hold a reference to the client before invoking it. As WakeLockManager
(which inherits VisibilityChangeClient) wasn't ref counted, this patch also makes it
ref counted.
It's also observed that the JS wrapper should not be deallocated by the GC before the
view transition has completed. This commit fixes this by implementing
ViewTransition::virtualHasPendingActivity(), which the GC consults to determine whether
to deallocate the wrapper or not.
* LayoutTests/fast/dom/view-transition-lifetime-crash-expected.txt: Added.
* LayoutTests/fast/dom/view-transition-lifetime-crash.html: Added.
* Source/WebCore/Modules/screen-wake-lock/WakeLockManager.cpp:
(WebCore::WakeLockManager::ref const): Delegated ref() to the document.
(WebCore::WakeLockManager::deref const): Delegated deref() to the document.
* Source/WebCore/Modules/screen-wake-lock/WakeLockManager.h: Made WakeLockManager ref counted by declaring ref() and deref().
* Source/WebCore/Modules/screen-wake-lock/WakeLockSentinel.cpp:
(WebCore::WakeLockSentinel::release): Hold a reference to the document's WakeLockManager before using it.
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::visibilityStateChanged): Hold a reference to the visibility state callback client before calling it.
(WebCore::Document::wakeLockManager): Used makeUniqueWithoutRefCountedCheck to create new WakeLockManager.
* Source/WebCore/dom/ViewTransition.cpp:
(WebCore::ViewTransition::virtualHasPendingActivity const): Added implementation.
* Source/WebCore/dom/ViewTransition.h:
* Source/WebCore/dom/VisibilityChangeClient.h: Made VisibilityChangeClient ref counted.
Canonical link: https://commits.webkit.org/286136@main
Canonical link: https://commits.webkit.org/283286.444@safari-7620-branch
diff --git a/LayoutTests/fast/dom/view-transition-lifetime-crash-expected.txt b/LayoutTests/fast/dom/view-transition-lifetime-crash-expected.txt
new file mode 100644
index 0000000..654ddf7
--- /dev/null
+++ b/LayoutTests/fast/dom/view-transition-lifetime-crash-expected.txt
@@ -0,0 +1 @@
+This test passes if it does not crash.
diff --git a/LayoutTests/fast/dom/view-transition-lifetime-crash.html b/LayoutTests/fast/dom/view-transition-lifetime-crash.html
new file mode 100644
index 0000000..03beeaf
--- /dev/null
+++ b/LayoutTests/fast/dom/view-transition-lifetime-crash.html
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<html>
+
+<head>
+ <script src="../../resources/gc.js"></script>
+</head>
+
+<body>
+ <p>This test passes if it does not crash.</p>
+
+ <script>
+ // This allocates a bunch of ViewTransition objects, but nothing references them,
+ // so they should be garbage collected (below).
+ for (let i = 0; i < 100; i++) {
+ let vt = document.startViewTransition();
+
+ // Repeatedly starting new view transitions will cancel the previous view transition and
+ // reject the `ready` promise. If the document visibility is hidden, the view transition
+ // is skipped and the promise is also rejected too.
+ // In any case, we don't want the unhandled promise rejection to show up in the
+ // test output, so catch them but do nothing.
+ vt.ready.catch(e => { });
+ }
+
+ // Trigger GC to deallocate the ViewTransition objects (hopefully)
+ window.gc();
+
+ document.addEventListener("visibilitychange", () => {
+ testRunner.notifyDone();
+ });
+
+ window.internals.setPageVisibility(false);
+
+ testRunner.waitUntilDone();
+ testRunner.dumpAsText();
+ </script>
+</body>
+
+</html>
diff --git a/Source/WebCore/Modules/screen-wake-lock/WakeLockManager.cpp b/Source/WebCore/Modules/screen-wake-lock/WakeLockManager.cpp
index 4cc72a7..862fb1f 100644
--- a/Source/WebCore/Modules/screen-wake-lock/WakeLockManager.cpp
+++ b/Source/WebCore/Modules/screen-wake-lock/WakeLockManager.cpp
@@ -47,6 +47,16 @@
m_document.unregisterForVisibilityStateChangedCallbacks(*this);
}
+void WakeLockManager::ref() const
+{
+ m_document.ref();
+}
+
+void WakeLockManager::deref() const
+{
+ m_document.deref();
+}
+
void WakeLockManager::addWakeLock(Ref<WakeLockSentinel>&& lock, std::optional<PageIdentifier> pageID)
{
auto type = lock->type();
diff --git a/Source/WebCore/Modules/screen-wake-lock/WakeLockManager.h b/Source/WebCore/Modules/screen-wake-lock/WakeLockManager.h
index d49a126..04c396e 100644
--- a/Source/WebCore/Modules/screen-wake-lock/WakeLockManager.h
+++ b/Source/WebCore/Modules/screen-wake-lock/WakeLockManager.h
@@ -45,6 +45,9 @@
explicit WakeLockManager(Document&);
~WakeLockManager();
+ void ref() const final;
+ void deref() const final;
+
void addWakeLock(Ref<WakeLockSentinel>&&, std::optional<PageIdentifier>);
void removeWakeLock(WakeLockSentinel&);
diff --git a/Source/WebCore/Modules/screen-wake-lock/WakeLockSentinel.cpp b/Source/WebCore/Modules/screen-wake-lock/WakeLockSentinel.cpp
index b69f69b..35aaf6f 100644
--- a/Source/WebCore/Modules/screen-wake-lock/WakeLockSentinel.cpp
+++ b/Source/WebCore/Modules/screen-wake-lock/WakeLockSentinel.cpp
@@ -46,8 +46,10 @@
void WakeLockSentinel::release(Ref<DeferredPromise>&& promise)
{
if (!m_wasReleased) {
- if (RefPtr document = downcast<Document>(scriptExecutionContext()))
- Ref { *this }->release(document->wakeLockManager());
+ if (RefPtr document = downcast<Document>(scriptExecutionContext())) {
+ Ref wakeLockManagerRef { document->wakeLockManager() };
+ Ref { *this }->release(wakeLockManagerRef.get());
+ }
}
promise->resolve();
}
diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
index 8a30bdd..29a47dd 100644
--- a/Source/WebCore/dom/Document.cpp
+++ b/Source/WebCore/dom/Document.cpp
@@ -2344,7 +2344,7 @@
// https://w3c.github.io/page-visibility/#reacting-to-visibilitychange-changes
queueTaskToDispatchEvent(TaskSource::UserInteraction, Event::create(eventNames().visibilitychangeEvent, Event::CanBubble::Yes, Event::IsCancelable::No));
m_visibilityStateCallbackClients.forEach([](auto& client) {
- client.visibilityStateChanged();
+ Ref { client }->visibilityStateChanged();
});
#if ENABLE(MEDIA_STREAM) && PLATFORM(IOS_FAMILY)
@@ -2412,7 +2412,7 @@
WakeLockManager& Document::wakeLockManager()
{
if (!m_wakeLockManager)
- m_wakeLockManager = makeUnique<WakeLockManager>(*this);
+ m_wakeLockManager = makeUniqueWithoutRefCountedCheck<WakeLockManager>(*this);
return *m_wakeLockManager;
}
diff --git a/Source/WebCore/dom/ViewTransition.cpp b/Source/WebCore/dom/ViewTransition.cpp
index 1b1416b..9b57452 100644
--- a/Source/WebCore/dom/ViewTransition.cpp
+++ b/Source/WebCore/dom/ViewTransition.cpp
@@ -206,6 +206,11 @@
});
}
+bool ViewTransition::virtualHasPendingActivity() const
+{
+ return m_phase != ViewTransitionPhase::Done;
+}
+
// https://drafts.csswg.org/css-view-transitions/#ViewTransition-skipTransition
void ViewTransition::skipTransition()
{
diff --git a/Source/WebCore/dom/ViewTransition.h b/Source/WebCore/dom/ViewTransition.h
index 36d1263..5ea8e45 100644
--- a/Source/WebCore/dom/ViewTransition.h
+++ b/Source/WebCore/dom/ViewTransition.h
@@ -216,6 +216,7 @@
// ActiveDOMObject.
void stop() final;
+ bool virtualHasPendingActivity() const final;
bool isCrossDocument() { return m_isCrossDocument; }
diff --git a/Source/WebCore/dom/VisibilityChangeClient.h b/Source/WebCore/dom/VisibilityChangeClient.h
index 128ad94..a4c18f4 100644
--- a/Source/WebCore/dom/VisibilityChangeClient.h
+++ b/Source/WebCore/dom/VisibilityChangeClient.h
@@ -31,15 +31,13 @@
class VisibilityChangeClient;
}
-namespace WTF {
-template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
-template<> struct IsDeprecatedWeakRefSmartPointerException<WebCore::VisibilityChangeClient> : std::true_type { };
-}
-
namespace WebCore {
class VisibilityChangeClient : public CanMakeWeakPtr<VisibilityChangeClient> {
public:
+ virtual void ref() const = 0;
+ virtual void deref() const = 0;
+
virtual ~VisibilityChangeClient() = default;
virtual void visibilityStateChanged() = 0;