RTCDataChannel.bufferedAmount should stay the same even if channel is closed
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 29 Jun 2020 11:50:04 +0000 (11:50 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 29 Jun 2020 11:50:04 +0000 (11:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=213698

Reviewed by Darin Adler.

Source/WebCore:

bufferedAmount was set back to zero when closing.
Instead, we should keep the value in RTCDataChannel and update it either when sending data
or being notified or some data getting sent.

Test: webrtc/datachannel/bufferedAmount-afterClose.html

* Modules/mediastream/RTCDataChannel.cpp:
(WebCore::RTCDataChannel::bufferedAmountIsDecreasing):
* platform/mock/RTCDataChannelHandlerMock.h:

LayoutTests:

* webrtc/datachannel/bufferedAmount-afterClose-expected.txt: Added.
* webrtc/datachannel/bufferedAmount-afterClose.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/webrtc/datachannel/bufferedAmount-afterClose-expected.txt [new file with mode: 0644]
LayoutTests/webrtc/datachannel/bufferedAmount-afterClose.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/mediastream/RTCDataChannel.cpp
Source/WebCore/Modules/mediastream/RTCDataChannel.h
Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp
Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.h
Source/WebCore/fileapi/BlobLoader.h
Source/WebCore/platform/mediastream/RTCDataChannelHandler.h
Source/WebCore/platform/mock/RTCDataChannelHandlerMock.h

index 3152c1c..3146066 100644 (file)
@@ -1,5 +1,15 @@
 2020-06-29  Youenn Fablet  <youenn@apple.com>
 
+        RTCDataChannel.bufferedAmount should stay the same even if channel is closed
+        https://bugs.webkit.org/show_bug.cgi?id=213698
+
+        Reviewed by Darin Adler.
+
+        * webrtc/datachannel/bufferedAmount-afterClose-expected.txt: Added.
+        * webrtc/datachannel/bufferedAmount-afterClose.html: Added.
+
+2020-06-29  Youenn Fablet  <youenn@apple.com>
+
         MediaRecorder.start() Method is Ignoring the "timeslice" Parameter
         https://bugs.webkit.org/show_bug.cgi?id=202233
         <rdar://problem/55720555>
diff --git a/LayoutTests/webrtc/datachannel/bufferedAmount-afterClose-expected.txt b/LayoutTests/webrtc/datachannel/bufferedAmount-afterClose-expected.txt
new file mode 100644 (file)
index 0000000..a44cd00
--- /dev/null
@@ -0,0 +1,6 @@
+
+PASS Close does not affect bufferedAmount - for text 
+PASS Close does not affect bufferedAmount - blob 
+PASS Close does not affect bufferedAmount - array buffer 
+PASS Close does not affect bufferedAmount - array buffer view 
+
diff --git a/LayoutTests/webrtc/datachannel/bufferedAmount-afterClose.html b/LayoutTests/webrtc/datachannel/bufferedAmount-afterClose.html
new file mode 100644 (file)
index 0000000..2f9eaad
--- /dev/null
@@ -0,0 +1,106 @@
+<!doctype html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Testing bufferedAmount oafter close</title>
+    <script src="../../resources/testharness.js"></script>
+    <script src="../../resources/testharnessreport.js"></script>
+  </head>
+  <body>
+    <script src ="../routines.js"></script>
+    <script>
+window.addEventListener("unhandledrejection", event => {
+    event.preventDefault();
+});
+
+var localChannel;
+
+function closeDataChannels() {
+    localChannel.close();
+    closeConnections();
+}
+
+var string = "abcdefghij";
+var longString;
+for (var cptr = 0; cptr < 100; ++cptr)
+    longString += string;
+
+function sendMessages(channel)
+{
+    channel.send(longString);
+}
+
+function createArrayBuffer(length)
+{
+    var array = new Uint8Array(length);
+    for (var cptr = 0; cptr < length; cptr++)
+        array[cptr] = cptr + 1;
+    return array;
+}
+
+function createArrayBufferView(length)
+{
+    return createArrayBuffer(2 * length).subarray(length, 2 * length);
+}
+
+function sendBlob(channel)
+{
+    channel.send(new Blob([createArrayBuffer(1000)], ""));
+}
+
+function sendArrayBuffer(channel)
+{
+    channel.send(createArrayBuffer(1000));
+}
+
+function sendArrayBuffer(channel)
+{
+    channel.send(createArrayBufferView(1000));
+}
+
+async function doDataChannelTest(writeChannel)
+{
+    await new Promise((resolve, reject) => {
+        createConnections((localConnection) => {
+            localChannel = localConnection.createDataChannel('sendDataChannel');
+            assert_equals(localChannel.bufferedAmountLowThreshold, 0, "default bufferedAmountLowThreshold value");
+            localChannel.onopen = resolve;
+        }, (remoteConnection) => {
+            remoteConnection.ondatachannel = (event) => {
+                event.channel.onmessage = () => { };
+            };
+        });
+        setTimeout(() => reject('timed out 1'), 5000);
+    });
+
+    const promise = new Promise((resolve, reject) => {
+        localChannel.onbufferedamountlow = resolve;
+        setTimeout(() => reject('timed out 2'), 5000);
+    });
+
+    writeChannel(localChannel);
+
+    const currentBufferedAmount = localChannel.bufferedAmount;
+    assert_greater_than_equal(currentBufferedAmount, 1000);
+    closeDataChannels();
+    assert_equals(localChannel.bufferedAmount, currentBufferedAmount);
+}
+
+promise_test(() => {
+    return doDataChannelTest(channel => channel.send(longString));
+}, "Close does not affect bufferedAmount - for text");
+
+promise_test(() => {
+    return doDataChannelTest(channel => channel.send(new Blob([createArrayBuffer(1000)])));
+}, "Close does not affect bufferedAmount - blob");
+
+promise_test(() => {
+    return doDataChannelTest(channel => channel.send(createArrayBuffer(1000)));
+}, "Close does not affect bufferedAmount - array buffer");
+
+promise_test(() => {
+    return doDataChannelTest(channel => channel.send(createArrayBuffer(1000)));
+}, "Close does not affect bufferedAmount - array buffer view");
+    </script>
+  </body>
+</html>
index c30d034..bf1fa28 100644 (file)
@@ -1,3 +1,20 @@
+2020-06-29  Youenn Fablet  <youenn@apple.com>
+
+        RTCDataChannel.bufferedAmount should stay the same even if channel is closed
+        https://bugs.webkit.org/show_bug.cgi?id=213698
+
+        Reviewed by Darin Adler.
+
+        bufferedAmount was set back to zero when closing.
+        Instead, we should keep the value in RTCDataChannel and update it either when sending data
+        or being notified or some data getting sent.
+
+        Test: webrtc/datachannel/bufferedAmount-afterClose.html
+
+        * Modules/mediastream/RTCDataChannel.cpp:
+        (WebCore::RTCDataChannel::bufferedAmountIsDecreasing):
+        * platform/mock/RTCDataChannelHandlerMock.h:
+
 2020-06-29  Antti Koivisto  <antti@apple.com>
 
         checked overflow in WebCore::findClosestFont
index 9dd37aa..38f39dd 100644 (file)
@@ -90,14 +90,6 @@ RTCDataChannel::RTCDataChannel(Document& document, std::unique_ptr<RTCDataChanne
 {
 }
 
-size_t RTCDataChannel::bufferedAmount() const
-{
-    // FIXME: We should compute our own bufferedAmount and not count on m_handler which is made null at closing time.
-    if (m_stopped)
-        return 0;
-    return m_handler->bufferedAmount();
-}
-
 const AtomString& RTCDataChannel::binaryType() const
 {
     switch (m_binaryType) {
@@ -129,6 +121,7 @@ ExceptionOr<void> RTCDataChannel::send(const String& data)
     if (m_readyState != RTCDataChannelState::Open)
         return Exception { InvalidStateError };
 
+    m_bufferedAmount += data.utf8().length();
     m_messageQueue.enqueue(data);
     return { };
 }
@@ -138,6 +131,7 @@ ExceptionOr<void> RTCDataChannel::send(ArrayBuffer& data)
     if (m_readyState != RTCDataChannelState::Open)
         return Exception { InvalidStateError };
 
+    m_bufferedAmount += data.byteLength();
     m_messageQueue.enqueue(data, 0, data.byteLength());
     return { };
 }
@@ -147,6 +141,7 @@ ExceptionOr<void> RTCDataChannel::send(ArrayBufferView& data)
     if (m_readyState != RTCDataChannelState::Open)
         return Exception { InvalidStateError };
 
+    m_bufferedAmount += data.byteLength();
     m_messageQueue.enqueue(*data.unsharedBuffer(), data.byteOffset(), data.byteLength());
     return { };
 }
@@ -156,6 +151,7 @@ ExceptionOr<void> RTCDataChannel::send(Blob& blob)
     if (m_readyState != RTCDataChannelState::Open)
         return Exception { InvalidStateError };
 
+    m_bufferedAmount += blob.size();
     m_messageQueue.enqueue(blob);
     return { };
 }
@@ -219,7 +215,9 @@ void RTCDataChannel::didDetectError()
 
 void RTCDataChannel::bufferedAmountIsDecreasing(size_t amount)
 {
-    if (amount <= m_bufferedAmountLowThreshold)
+    auto previousBufferedAmount = m_bufferedAmount;
+    m_bufferedAmount -= amount;
+    if (previousBufferedAmount > m_bufferedAmountLowThreshold && m_bufferedAmount <= m_bufferedAmountLowThreshold)
         scheduleDispatchEvent(Event::create(eventNames().bufferedamountlowEvent, Event::CanBubble::No, Event::IsCancelable::No));
 }
 
index 1a5c873..3a8a3af 100644 (file)
@@ -61,7 +61,7 @@ public:
 
     String label() const { return m_label; }
     RTCDataChannelState readyState() const {return m_readyState; }
-    size_t bufferedAmount() const;
+    size_t bufferedAmount() const { return m_bufferedAmount; }
     size_t bufferedAmountLowThreshold() const { return m_bufferedAmountLowThreshold; }
     void setBufferedAmountLowThreshold(size_t value) { m_bufferedAmountLowThreshold = value; }
 
@@ -113,6 +113,7 @@ private:
 
     String m_label;
     RTCDataChannelInit m_options;
+    size_t m_bufferedAmount { 0 };
     size_t m_bufferedAmountLowThreshold { 0 };
 
     NetworkSendQueue m_messageQueue;
index 93b93bc..fcd690e 100644 (file)
@@ -153,15 +153,12 @@ void LibWebRTCDataChannelHandler::OnMessage(const webrtc::DataBuffer& buffer)
     });
 }
 
-void LibWebRTCDataChannelHandler::OnBufferedAmountChange(uint64_t previousAmount)
+void LibWebRTCDataChannelHandler::OnBufferedAmountChange(uint64_t amount)
 {
     if (!m_client)
         return;
 
-    if (previousAmount <= m_channel->buffered_amount())
-        return;
-
-    callOnMainThread([protectedClient = makeRef(*m_client), amount = m_channel->buffered_amount()] {
+    callOnMainThread([protectedClient = makeRef(*m_client), amount] {
         protectedClient->bufferedAmountIsDecreasing(static_cast<size_t>(amount));
     });
 }
index a1072a7..5d2cd89 100644 (file)
@@ -63,7 +63,6 @@ private:
     bool sendStringData(const String&) final;
     bool sendRawData(const char*, size_t) final;
     void close() final;
-    size_t bufferedAmount() const final { return static_cast<size_t>(m_channel->buffered_amount()); }
 
     // webrtc::DataChannelObserver API
     void OnStateChange();
index e0bcc9f..d6507c4 100644 (file)
@@ -40,7 +40,7 @@ namespace WebCore {
 class BlobLoader final : public FileReaderLoaderClient {
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    BlobLoader(Document*, Blob&, CompletionHandler<void()>&&);
+    BlobLoader(Document*, Blob&, Function<void()>&&);
     ~BlobLoader();
 
     bool isLoading() const { return !!m_loader; }
@@ -58,10 +58,10 @@ private:
     std::unique_ptr<FileReaderLoader> m_loader;
     RefPtr<JSC::ArrayBuffer> m_buffer;
     Optional<ExceptionCode> m_errorCode;
-    CompletionHandler<void()> m_completionHandler;
+    Function<void()> m_completionHandler;
 };
 
-inline BlobLoader::BlobLoader(WebCore::Document* document, Blob& blob, CompletionHandler<void()>&& completionHandler)
+inline BlobLoader::BlobLoader(WebCore::Document* document, Blob& blob, Function<void()>&& completionHandler)
     : m_loader(makeUnique<FileReaderLoader>(FileReaderLoader::ReadAsArrayBuffer, this))
     , m_completionHandler(WTFMove(completionHandler))
 {
@@ -72,6 +72,7 @@ inline BlobLoader::~BlobLoader()
 {
     if (m_loader)
         m_loader->cancel();
+    // FIXME: Call m_completionHandler to migrate it from a Function to a CompletionHandler.
 }
 
 inline void BlobLoader::didFinishLoading()
index 6d2d38a..778c745 100644 (file)
@@ -52,8 +52,6 @@ public:
     virtual bool sendStringData(const String&) = 0;
     virtual bool sendRawData(const char*, size_t) = 0;
     virtual void close() = 0;
-
-    virtual size_t bufferedAmount() const = 0;
 };
 
 } // namespace WebCore
index 1388ad0..2b16b2a 100644 (file)
@@ -43,7 +43,6 @@ private:
     bool sendStringData(const String&) final;
     bool sendRawData(const char*, size_t) final;
     void close() final;
-    size_t bufferedAmount() const final { return 0; }
 
     RTCDataChannelHandlerClient* m_client { nullptr };