FetchResponse::BodyLoader should not be movable
authorzandobersek@gmail.com <zandobersek@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Jul 2019 05:43:11 +0000 (05:43 +0000)
committerzandobersek@gmail.com <zandobersek@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Jul 2019 05:43:11 +0000 (05:43 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199380

Reviewed by Youenn Fablet.

The FetchResponse::BodyLoader class has a FetchLoader member that is
initialized in the start() method with the reference of the owning
FetchResponse::BodyLoader object. This reference doesn't change when
the FetchResponse::BodyLoader object is moved into a different object
and the FetchLoader unique_ptr along with it, leading to problems when
that FetchLoader tries to invoke the FetchLoaderClient methods on the
FetchResponse::BodyLoader object that's been moved from and is possibly
already destroyed.

To avoid this, the FetchResponse::BodyLoader has the move constructor
removed and is now managed through std::unique_ptr instead of Optional,
ensuring the FetchResponse::BodyLoader object itself isn't moved around.

* Modules/fetch/FetchResponse.cpp:
(WebCore::FetchResponse::fetch):
(WebCore::FetchResponse::BodyLoader::didSucceed):
(WebCore::FetchResponse::BodyLoader::didFail):
* Modules/fetch/FetchResponse.h:

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

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

index 3df5f88..33a40a4 100644 (file)
@@ -1,3 +1,29 @@
+2019-07-02  Zan Dobersek  <zdobersek@igalia.com>
+
+        FetchResponse::BodyLoader should not be movable
+        https://bugs.webkit.org/show_bug.cgi?id=199380
+
+        Reviewed by Youenn Fablet.
+
+        The FetchResponse::BodyLoader class has a FetchLoader member that is
+        initialized in the start() method with the reference of the owning
+        FetchResponse::BodyLoader object. This reference doesn't change when
+        the FetchResponse::BodyLoader object is moved into a different object
+        and the FetchLoader unique_ptr along with it, leading to problems when
+        that FetchLoader tries to invoke the FetchLoaderClient methods on the
+        FetchResponse::BodyLoader object that's been moved from and is possibly
+        already destroyed.
+
+        To avoid this, the FetchResponse::BodyLoader has the move constructor
+        removed and is now managed through std::unique_ptr instead of Optional,
+        ensuring the FetchResponse::BodyLoader object itself isn't moved around.
+
+        * Modules/fetch/FetchResponse.cpp:
+        (WebCore::FetchResponse::fetch):
+        (WebCore::FetchResponse::BodyLoader::didSucceed):
+        (WebCore::FetchResponse::BodyLoader::didFail):
+        * Modules/fetch/FetchResponse.h:
+
 2019-07-02  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         Assertion fires when animating the 'class' attribute of an SVG element
index 4fe7616..d769607 100644 (file)
@@ -240,9 +240,9 @@ void FetchResponse::fetch(ScriptExecutionContext& context, FetchRequest& request
 
     response->addAbortSteps(request.signal());
 
-    response->m_bodyLoader.emplace(response.get(), WTFMove(responseCallback));
+    response->m_bodyLoader = std::make_unique<BodyLoader>(response.get(), WTFMove(responseCallback));
     if (!response->m_bodyLoader->start(context, request))
-        response->m_bodyLoader = WTF::nullopt;
+        response->m_bodyLoader = nullptr;
 }
 
 const String& FetchResponse::url() const
@@ -280,7 +280,7 @@ void FetchResponse::BodyLoader::didSucceed()
 
     if (m_loader->isStarted()) {
         Ref<FetchResponse> protector(m_response);
-        m_response.m_bodyLoader = WTF::nullopt;
+        m_response.m_bodyLoader = nullptr;
     }
 }
 
@@ -307,7 +307,7 @@ void FetchResponse::BodyLoader::didFail(const ResourceError& error)
     // Check whether didFail is called as part of FetchLoader::start.
     if (m_loader && m_loader->isStarted()) {
         Ref<FetchResponse> protector(m_response);
-        m_response.m_bodyLoader = WTF::nullopt;
+        m_response.m_bodyLoader = nullptr;
     }
 }
 
index 74c1454..5c83a37 100644 (file)
@@ -128,9 +128,9 @@ private:
     void addAbortSteps(Ref<AbortSignal>&&);
 
     class BodyLoader final : public FetchLoaderClient {
+        WTF_MAKE_FAST_ALLOCATED;
     public:
         BodyLoader(FetchResponse&, NotificationCallback&&);
-        BodyLoader(BodyLoader&&) = default;
         ~BodyLoader();
 
         bool start(ScriptExecutionContext&, const FetchRequest&);
@@ -160,7 +160,7 @@ private:
 
     mutable Optional<ResourceResponse> m_filteredResponse;
     ResourceResponse m_internalResponse;
-    Optional<BodyLoader> m_bodyLoader;
+    std::unique_ptr<BodyLoader> m_bodyLoader;
     mutable String m_responseURL;
     // Opaque responses will padd their body size when used with Cache API.
     uint64_t m_bodySizeWithPadding { 0 };