Bug 228096 – REGRESSION (r278702): Cannot login to appaloosa-store.com/users/sign_in

• Component: Page Loading
• Status: RESOLVED FIXED

[Chris Dumez]

r278702 broke the login page on appaloosa-store.com.

[Chris Dumez]

<rdar://80596391>

[Chris Dumez]

Created attachment 433843 [details]
Patch

[Alex Christensen]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 433878 [details]
Patch

[EWS]

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].

[Darin Adler]

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.

[Chris Dumez]

(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.