r278702 broke the login page on appaloosa-store.com.
<rdar://80596391>
Created attachment 433843 [details] Patch
Comment on attachment 433843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433843&action=review > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:358 > + data.append(m_buffer->data(), m_buffer->size()); This could also call forEachSegment to have fewer copies.
Comment on attachment 433843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433843&action=review >> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:358 >> + data.append(m_buffer->data(), m_buffer->size()); > > This could also call forEachSegment to have fewer copies. Good idea. Will update.
Created attachment 433878 [details] Patch
Committed r280085 (239810@main): <https://commits.webkit.org/239810@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433878 [details].
Comment on attachment 433878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433878&action=review > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:362 > + if (buffer->hasOneRef()) > + data = buffer->takeData(); // Avoid copying the data in the case where the buffer isn't shared. > + else { > + buffer->forEachSegment([&data](auto& span) { > + data.append(span); > + }); > + } If this pattern comes up again, perhaps we should consider a function with this logic? A safer "takeData" that will only do so when it’s singly owned, and would instead copy the data into a vector when it’s shared. The trick would be how to abstract the two different approaches, but I think it might be relevant. Here’s one possible implementation: Vector<uint8_t> extractData(Ref<SharedBuffer>&& buffer) { return buffer->hasOneRef() ? buffer->takeData() : buffer->copyData(); } And, since the forEachSegment/append optimization is important, we would move that optimization into SharedBuffer::copyData, unless there are some contexts where the current implementation of SharedBuffer::copyData is superior for some reason.
(In reply to Darin Adler from comment #7) > Comment on attachment 433878 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433878&action=review > > > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:362 > > + if (buffer->hasOneRef()) > > + data = buffer->takeData(); // Avoid copying the data in the case where the buffer isn't shared. > > + else { > > + buffer->forEachSegment([&data](auto& span) { > > + data.append(span); > > + }); > > + } > > If this pattern comes up again, perhaps we should consider a function with > this logic? A safer "takeData" that will only do so when it’s singly owned, > and would instead copy the data into a vector when it’s shared. The trick > would be how to abstract the two different approaches, but I think it might > be relevant. Here’s one possible implementation: > > Vector<uint8_t> extractData(Ref<SharedBuffer>&& buffer) > { > return buffer->hasOneRef() ? buffer->takeData() : buffer->copyData(); > } > > And, since the forEachSegment/append optimization is important, we would > move that optimization into SharedBuffer::copyData, unless there are some > contexts where the current implementation of SharedBuffer::copyData is > superior for some reason. Yes, I think this is a good idea. Now that I have experienced this bug, I actually think takeData() is too dangerous and we should just replace it with the safer alternative that copies when refCount is > 1.