- Revision
- 280085
- Author
- [email protected]
- Date
- 2021-07-20 10:32:55 -0700 (Tue, 20 Jul 2021)
Log Message
REGRESSION (r278702): Cannot login to appaloosa-store.com/users/sign_in
https://bugs.webkit.org/show_bug.cgi?id=228096
<rdar://80596391>
Reviewed by Alex Christensen.
Source/WebCore:
r278702 added an optimization to FetchBodyConsumer::takeAsBlob() to avoid a copy of the data.
What I didn't realized when I wrote this optimization is that FetchBodyConsumer is copy-constructible
and its copy constructor gets called when calling FetchResponse.clone(). The copy constructor only
does a shallow-copy of its internal buffer so several FetchResponse objects can end up with their
FetchBodyConsumer using the same underlying SharedBuffer object. When that SharedBuffer is shared,
calling takeData() on it is unacceptable as it will disturb the body of other Fetch responses.
To address the issue, we now only call SharedBuffer::takeData() and avoid the copy when the
SharedBuffer has a RefCount of 1, meaning that is it not actually shared. In the cases where it
is shared, we copy the data, like we used to do before r278702.
Test: http/tests/fetch/response-clone-blob.html
* Modules/fetch/FetchBodyConsumer.cpp:
(WebCore::FetchBodyConsumer::takeAsBlob):
LayoutTests:
Add layout test coverage.
* http/tests/fetch/response-clone-blob-expected.txt: Added.
* http/tests/fetch/response-clone-blob.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (280084 => 280085)
--- trunk/LayoutTests/ChangeLog 2021-07-20 17:07:58 UTC (rev 280084)
+++ trunk/LayoutTests/ChangeLog 2021-07-20 17:32:55 UTC (rev 280085)
@@ -1,3 +1,16 @@
+2021-07-20 Chris Dumez <[email protected]>
+
+ REGRESSION (r278702): Cannot login to appaloosa-store.com/users/sign_in
+ https://bugs.webkit.org/show_bug.cgi?id=228096
+ <rdar://80596391>
+
+ Reviewed by Alex Christensen.
+
+ Add layout test coverage.
+
+ * http/tests/fetch/response-clone-blob-expected.txt: Added.
+ * http/tests/fetch/response-clone-blob.html: Added.
+
2021-07-20 Chris Lord <[email protected]>
Canvas and OffscreenCanvas getContext should check if argument is an object before trying to convert it to a dictionary
Added: trunk/LayoutTests/http/tests/fetch/response-clone-blob-expected.txt (0 => 280085)
--- trunk/LayoutTests/http/tests/fetch/response-clone-blob-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/fetch/response-clone-blob-expected.txt 2021-07-20 17:32:55 UTC (rev 280085)
@@ -0,0 +1,10 @@
+Tests that calling blob() on a cloned Fetch Response does not disturb the original response.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS originalText == cloneText is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/http/tests/fetch/response-clone-blob.html (0 => 280085)
--- trunk/LayoutTests/http/tests/fetch/response-clone-blob.html (rev 0)
+++ trunk/LayoutTests/http/tests/fetch/response-clone-blob.html 2021-07-20 17:32:55 UTC (rev 280085)
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<script>
+description("Tests that calling blob() on a cloned Fetch Response does not disturb the original response.");
+jsTestIsAsync = true;
+
+async function runTest() {
+ let originalResponse = await fetch("/resources/testharnessreport.js");
+ let clonedResponse = originalResponse.clone();
+
+ let originalBlob = await originalResponse.blob();
+ let cloneBlob = await clonedResponse.blob();
+
+ originalText = await originalBlob.text();
+ cloneText = await cloneBlob.text();
+
+ shouldBeTrue("originalText == cloneText");
+
+ finishJSTest();
+}
+
+_onload_ = runTest;
+</script>
+</body>
+</html>
Modified: trunk/Source/WTF/wtf/Vector.h (280084 => 280085)
--- trunk/Source/WTF/wtf/Vector.h 2021-07-20 17:07:58 UTC (rev 280084)
+++ trunk/Source/WTF/wtf/Vector.h 2021-07-20 17:32:55 UTC (rev 280085)
@@ -34,6 +34,7 @@
#include <wtf/MathExtras.h>
#include <wtf/Noncopyable.h>
#include <wtf/NotFound.h>
+#include <wtf/Span.h>
#include <wtf/StdLibExtras.h>
#include <wtf/ValueCheck.h>
#include <wtf/VectorTraits.h>
@@ -784,6 +785,7 @@
template<typename U> ALWAYS_INLINE void append(const U* u, size_t size) { append<FailureAction::Crash>(u, size); }
template<typename U> ALWAYS_INLINE bool tryAppend(const U* u, size_t size) { return append<FailureAction::Report>(u, size); }
+ template<typename U> ALWAYS_INLINE void append(Span<const U> span) { append(span.data(), span.size()); }
template<typename U, size_t otherCapacity, typename OtherOverflowHandler, size_t otherMinCapacity, typename OtherMalloc> void appendVector(const Vector<U, otherCapacity, OtherOverflowHandler, otherMinCapacity, OtherMalloc>&);
template<typename U, size_t otherCapacity, typename OtherOverflowHandler, size_t otherMinCapacity, typename OtherMalloc> void appendVector(Vector<U, otherCapacity, OtherOverflowHandler, otherMinCapacity, OtherMalloc>&&);
Modified: trunk/Source/WebCore/ChangeLog (280084 => 280085)
--- trunk/Source/WebCore/ChangeLog 2021-07-20 17:07:58 UTC (rev 280084)
+++ trunk/Source/WebCore/ChangeLog 2021-07-20 17:32:55 UTC (rev 280085)
@@ -1,3 +1,27 @@
+2021-07-20 Chris Dumez <[email protected]>
+
+ REGRESSION (r278702): Cannot login to appaloosa-store.com/users/sign_in
+ https://bugs.webkit.org/show_bug.cgi?id=228096
+ <rdar://80596391>
+
+ Reviewed by Alex Christensen.
+
+ r278702 added an optimization to FetchBodyConsumer::takeAsBlob() to avoid a copy of the data.
+ What I didn't realized when I wrote this optimization is that FetchBodyConsumer is copy-constructible
+ and its copy constructor gets called when calling FetchResponse.clone(). The copy constructor only
+ does a shallow-copy of its internal buffer so several FetchResponse objects can end up with their
+ FetchBodyConsumer using the same underlying SharedBuffer object. When that SharedBuffer is shared,
+ calling takeData() on it is unacceptable as it will disturb the body of other Fetch responses.
+
+ To address the issue, we now only call SharedBuffer::takeData() and avoid the copy when the
+ SharedBuffer has a RefCount of 1, meaning that is it not actually shared. In the cases where it
+ is shared, we copy the data, like we used to do before r278702.
+
+ Test: http/tests/fetch/response-clone-blob.html
+
+ * Modules/fetch/FetchBodyConsumer.cpp:
+ (WebCore::FetchBodyConsumer::takeAsBlob):
+
2021-07-20 Chris Lord <[email protected]>
Canvas and OffscreenCanvas getContext should check if argument is an object before trying to convert it to a dictionary
Modified: trunk/Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp (280084 => 280085)
--- trunk/Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp 2021-07-20 17:07:58 UTC (rev 280084)
+++ trunk/Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp 2021-07-20 17:32:55 UTC (rev 280085)
@@ -351,7 +351,15 @@
if (!m_buffer)
return Blob::create(context, Vector<uint8_t>(), Blob::normalizedContentType(m_contentType));
- auto data = "" nullptr)->takeData();
+ Vector<uint8_t> data;
+ auto buffer = std::exchange(m_buffer, nullptr);
+ if (buffer->hasOneRef())
+ data = "" // Avoid copying the data in the case where the buffer isn't shared.
+ else {
+ buffer->forEachSegment([&data](auto& span) {
+ data.append(span);
+ });
+ }
return blobFromData(context, WTFMove(data), m_contentType);
}
