FetchResponse should close its stream when loading finishes
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Jul 2018 18:32:53 +0000 (18:32 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Jul 2018 18:32:53 +0000 (18:32 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187790

Reviewed by Chris Dumez.

It simplifies for a FetchResponse to push all its data into its stream if already created at end of load time.
Did some refactoring in FetchBodyOwner to have a cleaner relationship with the stream source.
Did a minor refactoring to expose the error description when loading fails as part of the rejected promise.
This is consistent to errors sent back through callbacks.

Covered by existing tests.

* Modules/fetch/FetchBodyOwner.cpp:
(WebCore::FetchBodyOwner::~FetchBodyOwner):
* Modules/fetch/FetchBodyOwner.h:
* Modules/fetch/FetchBodySource.cpp:
(WebCore::FetchBodySource::FetchBodySource):
(WebCore::FetchBodySource::setActive):
(WebCore::FetchBodySource::setInactive):
(WebCore::FetchBodySource::doStart):
(WebCore::FetchBodySource::doPull):
(WebCore::FetchBodySource::doCancel):
(WebCore::FetchBodySource::cleanBodyOwner):
* Modules/fetch/FetchBodySource.h:
* Modules/fetch/FetchResponse.cpp:
(WebCore::FetchResponse::BodyLoader::didSucceed):
(WebCore::FetchResponse::BodyLoader::didFail):

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

Source/WebCore/ChangeLog
Source/WebCore/Modules/fetch/FetchBodyOwner.cpp
Source/WebCore/Modules/fetch/FetchBodyOwner.h
Source/WebCore/Modules/fetch/FetchBodySource.cpp
Source/WebCore/Modules/fetch/FetchBodySource.h
Source/WebCore/Modules/fetch/FetchResponse.cpp

index 2b9b80a..305faa6 100644 (file)
@@ -1,3 +1,33 @@
+2018-07-19  Youenn Fablet  <youenn@apple.com>
+
+        FetchResponse should close its stream when loading finishes
+        https://bugs.webkit.org/show_bug.cgi?id=187790
+
+        Reviewed by Chris Dumez.
+
+        It simplifies for a FetchResponse to push all its data into its stream if already created at end of load time.
+        Did some refactoring in FetchBodyOwner to have a cleaner relationship with the stream source.
+        Did a minor refactoring to expose the error description when loading fails as part of the rejected promise.
+        This is consistent to errors sent back through callbacks.
+
+        Covered by existing tests.
+
+        * Modules/fetch/FetchBodyOwner.cpp:
+        (WebCore::FetchBodyOwner::~FetchBodyOwner):
+        * Modules/fetch/FetchBodyOwner.h:
+        * Modules/fetch/FetchBodySource.cpp:
+        (WebCore::FetchBodySource::FetchBodySource):
+        (WebCore::FetchBodySource::setActive):
+        (WebCore::FetchBodySource::setInactive):
+        (WebCore::FetchBodySource::doStart):
+        (WebCore::FetchBodySource::doPull):
+        (WebCore::FetchBodySource::doCancel):
+        (WebCore::FetchBodySource::cleanBodyOwner):
+        * Modules/fetch/FetchBodySource.h:
+        * Modules/fetch/FetchResponse.cpp:
+        (WebCore::FetchResponse::BodyLoader::didSucceed):
+        (WebCore::FetchResponse::BodyLoader::didFail):
+
 2018-07-19  Jon Lee  <jonlee@apple.com>
 
         Update iOS fullscreen alert text again
index fa31a19..03b94f1 100644 (file)
@@ -45,6 +45,12 @@ FetchBodyOwner::FetchBodyOwner(ScriptExecutionContext& context, std::optional<Fe
     suspendIfNeeded();
 }
 
+FetchBodyOwner::~FetchBodyOwner()
+{
+    if (m_readableStreamSource)
+        m_readableStreamSource->detach();
+}
+
 void FetchBodyOwner::stop()
 {
     if (m_body)
index d6ed863..cc0ef2c 100644 (file)
@@ -41,6 +41,7 @@ namespace WebCore {
 class FetchBodyOwner : public RefCounted<FetchBodyOwner>, public ActiveDOMObject {
 public:
     FetchBodyOwner(ScriptExecutionContext&, std::optional<FetchBody>&&, Ref<FetchHeaders>&&);
+    ~FetchBodyOwner();
 
     bool bodyUsed() const { return isDisturbed(); }
     void arrayBuffer(Ref<DeferredPromise>&&);
index 237a460..e8831fd 100644 (file)
 namespace WebCore {
 
 FetchBodySource::FetchBodySource(FetchBodyOwner& bodyOwner)
-    : m_bodyOwner(bodyOwner)
+    : m_bodyOwner(&bodyOwner)
 {
 }
 
 void FetchBodySource::setActive()
 {
-    m_bodyOwner.setPendingActivity(&m_bodyOwner);
+    ASSERT(m_bodyOwner);
+    if (m_bodyOwner)
+        m_bodyOwner->setPendingActivity(m_bodyOwner);
 }
 
 void FetchBodySource::setInactive()
 {
-    m_bodyOwner.unsetPendingActivity(&m_bodyOwner);
+    ASSERT(m_bodyOwner);
+    if (m_bodyOwner)
+        m_bodyOwner->unsetPendingActivity(m_bodyOwner);
 }
 
 void FetchBodySource::doStart()
 {
-    m_bodyOwner.consumeBodyAsStream();
+    ASSERT(m_bodyOwner);
+    if (m_bodyOwner)
+        m_bodyOwner->consumeBodyAsStream();
 }
 
 void FetchBodySource::doPull()
 {
-    m_bodyOwner.feedStream();
+    ASSERT(m_bodyOwner);
+    if (m_bodyOwner)
+        m_bodyOwner->feedStream();
 }
 
 void FetchBodySource::doCancel()
 {
     m_isCancelling = true;
-    m_bodyOwner.cancel();
+    ASSERT(m_bodyOwner);
+    if (!m_bodyOwner)
+        return;
+
+    m_bodyOwner->cancel();
+    m_bodyOwner = nullptr;
 }
 
 void FetchBodySource::close()
 {
+    m_bodyOwner = nullptr;
     controller().close();
     clean();
 }
 
 void FetchBodySource::error(const String& value)
 {
+    m_bodyOwner = nullptr;
     controller().error(value);
     clean();
 }
index 98159b8..f96335c 100644 (file)
@@ -49,6 +49,7 @@ public:
     bool isCancelling() const { return m_isCancelling; }
 
     void resolvePullPromise() { pullFinished(); }
+    void detach() { m_bodyOwner = nullptr; }
 
 private:
     void doStart() final;
@@ -57,7 +58,7 @@ private:
     void setActive() final;
     void setInactive() final;
 
-    FetchBodyOwner& m_bodyOwner;
+    FetchBodyOwner* m_bodyOwner;
     bool m_isCancelling { false };
 };
 
index ef4a7c2..ff8c3a2 100644 (file)
@@ -225,8 +225,12 @@ void FetchResponse::BodyLoader::didSucceed()
     m_response.m_body->loadingSucceeded();
 
 #if ENABLE(STREAMS_API)
-    if (m_response.m_readableStreamSource && !m_response.body().consumer().hasData())
+    if (m_response.m_readableStreamSource) {
+        if (m_response.body().consumer().hasData())
+            m_response.m_readableStreamSource->enqueue(m_response.body().consumer().takeAsArrayBuffer());
+
         m_response.closeStream();
+    }
 #endif
     if (auto consumeDataCallback = WTFMove(m_consumeDataCallback))
         consumeDataCallback(nullptr);
@@ -252,7 +256,7 @@ void FetchResponse::BodyLoader::didFail(const ResourceError& error)
 #if ENABLE(STREAMS_API)
     if (m_response.m_readableStreamSource) {
         if (!m_response.m_readableStreamSource->isCancelling())
-            m_response.m_readableStreamSource->error("Loading failed"_s);
+            m_response.m_readableStreamSource->error(makeString("Loading failed: "_s, error.localizedDescription()));
         m_response.m_readableStreamSource = nullptr;
     }
 #endif