Add very basic PageCache support for RTCPeerConnection
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Sep 2019 05:32:00 +0000 (05:32 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Sep 2019 05:32:00 +0000 (05:32 +0000)
https://bugs.webkit.org/show_bug.cgi?id=202244

Reviewed by Geoffrey Garen.

Source/WebCore:

RTCPeerConnection::canSuspendForDocumentSuspension() returns true if
RTCPeerConnection::hasPendingActivity() return false. Previously, hasPendingActivity()
would return true unless the JS has called close() on the RTCPeerConnection
object.

On weather.com, an ad would construct an RTCPeerConnection just to do some feature
checking and then let the variable go out of scope (without calling close()). Because
of the previous implementation of RTCPeerConnection::hasPendingActivity(), this wrapper
and its implementation object would stay alive forever AND prevent the page from
entering PageCache on navigation.

To improve this, the implementation of hadPendingActivity() has been updated so that
it keeps returning false if close() has been called, but will also return false if
there are no pending Promises to be resolved and no event listeners.

Test: fast/mediastream/RTCPeerConnection-page-cache.html

* Modules/mediastream/RTCPeerConnection.cpp:
WebCore::RTCPeerConnection::RTCPeerConnection:
Stop taking a pending activity in the constructor, this would be NOT keep the wrapper
alive since RTCPeerConnection::hasPendingActivity() was not checking
ActiveDOMObject::hasPendingActivity().

(WebCore::RTCPeerConnection::canSuspendForDocumentSuspension const):
(WebCore::RTCPeerConnection::hasPendingActivity const):

* Modules/mediastream/RTCPeerConnection.h:

* bindings/js/JSDOMPromiseDeferred.h:
(WebCore::DOMPromiseDeferredBase::whenSettled):

LayoutTests:

Add layout test coverage.

* fast/mediastream/RTCPeerConnection-page-cache-expected.txt: Added.
* fast/mediastream/RTCPeerConnection-page-cache.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250379 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/mediastream/RTCPeerConnection-page-cache-expected.txt [new file with mode: 0644]
LayoutTests/fast/mediastream/RTCPeerConnection-page-cache.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp
Source/WebCore/Modules/mediastream/RTCPeerConnection.h
Source/WebCore/bindings/js/JSDOMPromise.cpp
Source/WebCore/bindings/js/JSDOMPromise.h
Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp
Source/WebCore/bindings/js/JSDOMPromiseDeferred.h

index 7a045b0..fa69ae6 100644 (file)
@@ -1,5 +1,17 @@
 2019-09-25  Chris Dumez  <cdumez@apple.com>
 
+        Add very basic PageCache support for RTCPeerConnection
+        https://bugs.webkit.org/show_bug.cgi?id=202244
+
+        Reviewed by Geoffrey Garen.
+
+        Add layout test coverage.
+
+        * fast/mediastream/RTCPeerConnection-page-cache-expected.txt: Added.
+        * fast/mediastream/RTCPeerConnection-page-cache.html: Added.
+
+2019-09-25  Chris Dumez  <cdumez@apple.com>
+
         Improve Service worker support for Page Caching
         https://bugs.webkit.org/show_bug.cgi?id=202221
 
diff --git a/LayoutTests/fast/mediastream/RTCPeerConnection-page-cache-expected.txt b/LayoutTests/fast/mediastream/RTCPeerConnection-page-cache-expected.txt
new file mode 100644 (file)
index 0000000..07f6342
--- /dev/null
@@ -0,0 +1,13 @@
+Basic testing for RTCPeerConnection Page Caching.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+pageshow - not from page cache
+pagehide - entering page cache
+pageshow - from page cache
+PASS Page entered page cache
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/mediastream/RTCPeerConnection-page-cache.html b/LayoutTests/fast/mediastream/RTCPeerConnection-page-cache.html
new file mode 100644 (file)
index 0000000..d2e93aa
--- /dev/null
@@ -0,0 +1,34 @@
+<!-- webkit-test-runner [ enablePageCache=true ] -->
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test.js"></script>
+<script>
+description("Basic testing for RTCPeerConnection Page Caching.");
+jsTestIsAsync = true;
+
+window.addEventListener("pageshow", function(event) {
+  debug("pageshow - " + (event.persisted ? "" : "not ") + "from page cache");
+  if (event.persisted) {
+      testPassed("Page entered page cache");
+      finishJSTest();
+  }
+});
+
+window.addEventListener("pagehide", function(event) {
+    debug("pagehide - " + (event.persisted ? "" : "not ") + "entering page cache");
+    if (!event.persisted) {
+        testFailed("Page failed to enter page cache");
+        finishJSTest();
+    }
+});
+
+onload = () => {
+    connection = new RTCPeerConnection();
+    setTimeout(() => {
+        window.location = "../history/resources/page-cache-helper.html";
+    }, 0);
+};
+</script>
+</body>
+</html>
index d49cede..6a4960a 100644 (file)
@@ -1,5 +1,43 @@
 2019-09-25  Chris Dumez  <cdumez@apple.com>
 
+        Add very basic PageCache support for RTCPeerConnection
+        https://bugs.webkit.org/show_bug.cgi?id=202244
+
+        Reviewed by Geoffrey Garen.
+
+        RTCPeerConnection::canSuspendForDocumentSuspension() returns true if
+        RTCPeerConnection::hasPendingActivity() return false. Previously, hasPendingActivity()
+        would return true unless the JS has called close() on the RTCPeerConnection
+        object.
+
+        On weather.com, an ad would construct an RTCPeerConnection just to do some feature
+        checking and then let the variable go out of scope (without calling close()). Because
+        of the previous implementation of RTCPeerConnection::hasPendingActivity(), this wrapper
+        and its implementation object would stay alive forever AND prevent the page from
+        entering PageCache on navigation.
+
+        To improve this, the implementation of hadPendingActivity() has been updated so that
+        it keeps returning false if close() has been called, but will also return false if
+        there are no pending Promises to be resolved and no event listeners.
+
+        Test: fast/mediastream/RTCPeerConnection-page-cache.html
+
+        * Modules/mediastream/RTCPeerConnection.cpp:
+        WebCore::RTCPeerConnection::RTCPeerConnection:
+        Stop taking a pending activity in the constructor, this would be NOT keep the wrapper
+        alive since RTCPeerConnection::hasPendingActivity() was not checking
+        ActiveDOMObject::hasPendingActivity().
+
+        (WebCore::RTCPeerConnection::canSuspendForDocumentSuspension const):
+        (WebCore::RTCPeerConnection::hasPendingActivity const):
+        
+        * Modules/mediastream/RTCPeerConnection.h:
+
+        * bindings/js/JSDOMPromiseDeferred.h:
+        (WebCore::DOMPromiseDeferredBase::whenSettled):
+
+2019-09-25  Chris Dumez  <cdumez@apple.com>
+
         Improve Service worker support for Page Caching
         https://bugs.webkit.org/show_bug.cgi?id=202221
 
index e0f31a7..c999acc 100644 (file)
@@ -70,10 +70,7 @@ Ref<RTCPeerConnection> RTCPeerConnection::create(ScriptExecutionContext& context
     auto& document = downcast<Document>(context);
     auto peerConnection = adoptRef(*new RTCPeerConnection(document));
     peerConnection->suspendIfNeeded();
-    // RTCPeerConnection may send events at about any time during its lifetime.
-    // Let's make it uncollectable until the pc is closed by JS or the page stops it.
     if (!peerConnection->isClosed()) {
-        peerConnection->m_pendingActivity = peerConnection->makePendingActivity(peerConnection.get());
         if (auto* page = document.page()) {
             peerConnection->registerToController(page->rtcController());
             page->libWebRTCProvider().setEnableLogging(!page->sessionID().isEphemeral());
@@ -197,6 +194,7 @@ void RTCPeerConnection::queuedCreateOffer(RTCOfferOptions&& options, SessionDesc
         return;
     }
 
+    addPendingPromise(promise);
     m_backend->createOffer(WTFMove(options), WTFMove(promise));
 }
 
@@ -208,6 +206,7 @@ void RTCPeerConnection::queuedCreateAnswer(RTCAnswerOptions&& options, SessionDe
         return;
     }
 
+    addPendingPromise(promise);
     m_backend->createAnswer(WTFMove(options), WTFMove(promise));
 }
 
@@ -219,6 +218,7 @@ void RTCPeerConnection::queuedSetLocalDescription(RTCSessionDescription& descrip
         return;
     }
 
+    addPendingPromise(promise);
     m_backend->setLocalDescription(description, WTFMove(promise));
 }
 
@@ -245,6 +245,7 @@ void RTCPeerConnection::queuedSetRemoteDescription(RTCSessionDescription& descri
         promise.reject(InvalidStateError);
         return;
     }
+    addPendingPromise(promise);
     m_backend->setRemoteDescription(description, WTFMove(promise));
 }
 
@@ -272,6 +273,7 @@ void RTCPeerConnection::queuedAddIceCandidate(RTCIceCandidate* rtcCandidate, DOM
         return;
     }
 
+    addPendingPromise(promise);
     m_backend->addIceCandidate(rtcCandidate, WTFMove(promise));
 }
 
@@ -399,6 +401,7 @@ void RTCPeerConnection::getStats(MediaStreamTrack* selector, Ref<DeferredPromise
             }
         }
     }
+    addPendingPromise(promise.get());
     m_backend->getStats(WTFMove(promise));
 }
 
@@ -474,7 +477,6 @@ void RTCPeerConnection::doStop()
     m_isStopped = true;
 
     m_backend->stop();
-    m_pendingActivity = nullptr;
 }
 
 void RTCPeerConnection::registerToController(RTCController& controller)
@@ -494,6 +496,7 @@ const char* RTCPeerConnection::activeDOMObjectName() const
     return "RTCPeerConnection";
 }
 
+    // FIXME: We should do better here, it is way too easy to prevent PageCache.
 bool RTCPeerConnection::canSuspendForDocumentSuspension() const
 {
     return !hasPendingActivity();
@@ -501,7 +504,15 @@ bool RTCPeerConnection::canSuspendForDocumentSuspension() const
 
 bool RTCPeerConnection::hasPendingActivity() const
 {
-    return !m_isStopped;
+    if (m_isStopped)
+        return false;
+
+    // This returns true if we have pending promises to be resolved.
+    if (ActiveDOMObject::hasPendingActivity())
+        return true;
+
+    // As long as the connection is not stopped and it has event listeners, it may dispatch events.
+    return hasEventListeners();
 }
 
 void RTCPeerConnection::addTransceiver(Ref<RTCRtpTransceiver>&& transceiver)
index 74bfdae..c8ca96d 100644 (file)
@@ -185,6 +185,11 @@ public:
 #endif
 
 private:
+    template<typename PromiseType> void addPendingPromise(PromiseType& promise)
+    {
+        promise.whenSettled([pendingActivity = makePendingActivity(*this)] { });
+    }
+
     RTCPeerConnection(Document&);
 
     ExceptionOr<void> initializeConfiguration(RTCConfiguration&&);
@@ -230,7 +235,6 @@ private:
     RTCConfiguration m_configuration;
     RTCController* m_controller { nullptr };
     Vector<RefPtr<RTCCertificate>> m_certificates;
-    RefPtr<PendingActivity<RTCPeerConnection>> m_pendingActivity;
 };
 
 } // namespace WebCore
index 24c5ce7..226bef9 100644 (file)
@@ -57,7 +57,7 @@ void DOMPromise::whenSettled(std::function<void()>&& callback)
     whenPromiseIsSettled(globalObject(), promise(), WTFMove(callback));
 }
 
-void DOMPromise::whenPromiseIsSettled(JSDOMGlobalObject* globalObject, JSC::JSObject* promise, std::function<void()>&& callback)
+void DOMPromise::whenPromiseIsSettled(JSDOMGlobalObject* globalObject, JSC::JSObject* promise, Function<void()>&& callback)
 {
     auto& state = *globalObject->globalExec();
     auto& vm = state.vm();
index a9fb623..483e69f 100644 (file)
@@ -50,7 +50,7 @@ public:
     enum class Status { Pending, Fulfilled, Rejected };
     Status status() const;
 
-    static void whenPromiseIsSettled(JSDOMGlobalObject*, JSC::JSObject* promise, std::function<void()>&&);
+    static void whenPromiseIsSettled(JSDOMGlobalObject*, JSC::JSObject* promise, Function<void()>&&);
 
 private:
     DOMPromise(JSDOMGlobalObject& globalObject, JSC::JSPromise& promise)
index 98abb5f..02e6978 100644 (file)
@@ -69,7 +69,7 @@ void DeferredPromise::callFunction(ExecState& exec, JSValue function, JSValue re
         clear();
 }
 
-void DeferredPromise::whenSettled(std::function<void()>&& callback)
+void DeferredPromise::whenSettled(Function<void()>&& callback)
 {
     DOMPromise::whenPromiseIsSettled(globalObject(), deferred()->promise(), WTFMove(callback));
 }
index 702ca40..d96028b 100644 (file)
@@ -146,7 +146,7 @@ public:
 
     JSC::JSValue promise() const;
 
-    void whenSettled(std::function<void()>&&);
+    void whenSettled(Function<void()>&&);
 
 private:
     DeferredPromise(JSDOMGlobalObject& globalObject, JSC::JSPromiseDeferred& deferred, Mode mode)
@@ -213,6 +213,11 @@ public:
 
     JSC::JSValue promise() const { return m_promiseDeferred->promise(); };
 
+    void whenSettled(Function<void()>&& function)
+    {
+        m_promiseDeferred->whenSettled(WTFMove(function));
+    }
+
 protected:
     Ref<DeferredPromise> m_promiseDeferred;
 };