Bug 226623 – Use Vector<uint8_t> instead of Vector<char> to store bytes in SharedBuffer

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Use Vector<uint8_t> instead of Vector<char> to store bytes in SharedBuffer. Also have SharedBuffer::data() return a `const uint8_t*` instead of `const char*`.
This is our preferred type to store bytes.

[Chris Dumez]

Created attachment 430537 [details]
Patch

[Chris Dumez]

Created attachment 430542 [details]
Patch

[Chris Dumez]

Created attachment 430543 [details]
Patch

[Chris Dumez]

Created attachment 430545 [details]
Patch

[Chris Dumez]

Created attachment 430549 [details]
Patch

[Chris Dumez]

Created attachment 430552 [details]
Patch

[Chris Dumez]

Created attachment 430575 [details]
Patch

[Chris Dumez]

Created attachment 430576 [details]
Patch

[Chris Dumez]

Created attachment 430577 [details]
Patch

[Darin Adler]

Comment on attachment 430577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430577&action=review

We may want to work harder to make sure continue to adopt vectors in cases where this patch changes us to copy instead.

We may also want to use const void* and void* more on functions that take and process bytes, rather than using a specific byte pointer types.

> Source/WTF/wtf/FileSystem.h:156
> +WTF_EXPORT_PRIVATE int writeToFile(PlatformFileHandle, const char* data, int length); // FIXME: Should use `const uint8_t*`.

Maybe it should take const void* like the POSIX write() function.

> Source/WTF/wtf/FileSystem.h:158
> +WTF_EXPORT_PRIVATE int readFromFile(PlatformFileHandle, char* data, int length); // FIXME: Should use `uint8_t*`.

Maybe it should take void* like the POSIX read() function.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:197
> +        const char* nextBoundary = static_cast<const char*>(memmem(currentBoundary + boundaryLength, length - (currentBoundary + boundaryLength - reinterpret_cast<const char*>(data)), boundary.data(), boundaryLength));

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:203
> +            nextBoundary = static_cast<const char*>(memmem(nextBoundary + boundaryLength, length - (nextBoundary + boundaryLength - reinterpret_cast<const char*>(data)), boundary.data(), boundaryLength));

Ditto.

> Source/WebCore/Modules/highlight/AppHighlight.h:55
> +    encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(highlight->data()), highlight->size(), 1);

Why is a cast needed here? Doesn’t data() return a uint8_t*?

> Source/WebCore/Modules/indexeddb/IDBGetResult.cpp:37
> +    m_value = ThreadSafeDataBuffer::create({ buffer.data(), buffer.size() });

No cleaner idiom? Maybe a std::span or Vector?

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:172
> +        const uint8_t* data = reinterpret_cast<const uint8_t*>(buffer->data.data<uint8_t>());

auto would be better here.

Why is reinterpret_cast needed here? What type does data<uint8_t>() return? Why not uint8_t*?

> Source/WebCore/css/CSSFontFaceSource.cpp:168
> -                    m_generatedOTFBuffer = SharedBuffer::create(WTFMove(otfFont.value()));
> +                    m_generatedOTFBuffer = SharedBuffer::create(otfFont->data(), otfFont->size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/fileapi/Blob.cpp:117
> +    blobParts.append(Vector { buffer.data(), buffer.size() });

This seems to call for std::span. Or doesn’t SharedBuffer have a function that returns a Vector?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:583
> +        NetworkResourcesData::ResourceData const* resourceData = m_resourcesData->maybeAddResourceData(requestId, reinterpret_cast<const uint8_t*>(data), dataLength);

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:159
> +        m_client->didReceiveData(this, buffer ? buffer->data() : reinterpret_cast<const uint8_t*>(data), buffer ? buffer->size() : length);

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:157
> -        content->append(WTFMove(part));
> +        content->append(part.data(), part.size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:198
> +        auto decodedData = base64Decode(content->data()   , content->size());

Should probably get rid of the extra spaces here.

> Source/WebCore/loader/cache/CachedSVGFont.cpp:92
> -            m_convertedFont = SharedBuffer::create(WTFMove(convertedFont.value()));
> +            m_convertedFont = SharedBuffer::create(convertedFont->data(), convertedFont->size());

Is this changing from adopting data to copying it, or did the old version already copy the data?

> Source/WebCore/platform/SharedBuffer.cpp:142
> -    auto arrayBuffer = ArrayBuffer::tryCreateUninitialized(static_cast<unsigned>(size()), sizeof(char));
> +    auto arrayBuffer = ArrayBuffer::tryCreateUninitialized(static_cast<unsigned>(size()), sizeof(uint8_t));

Not new: What guarantees size() fits in unsigned?

Both of these sizeof expression are silly ways to write the number 1. I think we could just write 1.

> Source/WebCore/platform/SharedBuffer.cpp:232
> +        [](const RetainPtr<CFDataRef>& data) { return reinterpret_cast<const uint8_t*>(CFDataGetBytePtr(data.get())); },

No typecast needed any more: CFDataGetBytePtr returns const uint8_t*.

> Source/WebCore/platform/SharedBuffer.cpp:235
> +        [](const GRefPtr<GBytes>& data) { return reinterpret_cast<const uint8_t*>(g_bytes_get_data(data.get(), nullptr)); },

This should be static_cast, not reinterpret_cast; it’s a cast from a void*.

> Source/WebCore/platform/SharedBuffer.cpp:238
> +        [](const RefPtr<GstMappedOwnedBuffer>& data) { return reinterpret_cast<const uint8_t*>(data->data()); },

No typecast needed any more: GstMappedOwnedBuffer::data returns const uint8_t*.

> Source/WebCore/platform/SharedBuffer.cpp:240
> +        [](const FileSystem::MappedFileData& data) { return reinterpret_cast<const uint8_t*>(data.data()); }

This should be static_cast, not reinterpret_cast; it’s a cast from a void*.

> Source/WebCore/platform/SharedBuffer.cpp:357
> +    char* p = reinterpret_cast<char*>(buffer.data());

Always a sad moment when we have to add a reinterpret_cast; any way to avoid this one?

> Source/WebCore/platform/SharedBuffer.cpp:370
> +    buffer.shrink(p - reinterpret_cast<char*>(buffer.data()));

Always a sad moment when we have to add a reinterpret_cast; any way to avoid this one?

> Source/WebCore/platform/SharedBufferChunkReader.cpp:103
> +                chunk.append(reinterpret_cast<const uint8_t*>(m_separator.data()), m_separatorIndex);

Always a sad moment when we have to add a reinterpret_cast; any way to avoid this one?

> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:297
> -        buffer = SharedBuffer::create(WTFMove(convertedFont));
> +        buffer = SharedBuffer::create(convertedFont.data(), convertedFont.size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:239
> +    const uint8_t* source = sharedBuffer->data() + position;

auto

> Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:89
> +        size_t offset = reinterpret_cast<const uint8_t*>(position) - buffer.data();

This can be static_cast instead of reinterpret_cast since it’s from a void*.

> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:318
> +    return strncmp(&m_data->dataAsCharPtr()[imageOffset], "\x89PNG", 4) ? BMP : PNG;

This should use memcmp instead of strncmp--it was never useful to use strncmp here, no reason to handle null characters specially--and then we will not need dataAsCharPtr here.

> Source/WebCore/platform/network/FormData.cpp:417
> +    auto bytes = flatten();
> +    return SharedBuffer::create(bytes.data(), bytes.size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/platform/win/SharedBufferWin.cpp:55
> +        if (ReadFile(fileHandle, (LPVOID)buffer.data(), bytesToRead, &bytesRead, 0) && bytesToRead == bytesRead)

Why was this new typecast needed?

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:362
> -    OffsetBuffer(Vector<char> buffer)
> +    OffsetBuffer(Vector<uint8_t>&& buffer)

Nice optimization.

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:364
>          , m_currentOffset(0)

We should move this initialization down to where the data member is defined below.

> Source/WebKit/WebProcess/Plugins/Netscape/NetscapePluginStream.cpp:120
> +    deliverData(reinterpret_cast<const uint8_t*>(resultCString.data()), resultCString.length());

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1172
> +    m_pluginView->manualLoadDidReceiveData(reinterpret_cast<const uint8_t*>(data), length);

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

[Chris Dumez]

(In reply to Darin Adler from comment #10)
> Comment on attachment 430577 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430577&action=review
> 
> We may want to work harder to make sure continue to adopt vectors in cases
> where this patch changes us to copy instead.

Yes, I am definitely planning to follow-up to fix any case that got worse.

> 
> We may also want to use const void* and void* more on functions that take
> and process bytes, rather than using a specific byte pointer types.

Yes, I think that's a good idea.

> 
> > Source/WTF/wtf/FileSystem.h:156
> > +WTF_EXPORT_PRIVATE int writeToFile(PlatformFileHandle, const char* data, int length); // FIXME: Should use `const uint8_t*`.
> 
> Maybe it should take const void* like the POSIX write() function.

Doing this one in this patch since it is a significant improvement at call sites.

> > Source/WTF/wtf/FileSystem.h:158
> > +WTF_EXPORT_PRIVATE int readFromFile(PlatformFileHandle, char* data, int length); // FIXME: Should use `uint8_t*`.
> 
> Maybe it should take void* like the POSIX read() function.

Agree, will add a FIXME comment and address in a follow-up.

> 
> > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:197
> > +        const char* nextBoundary = static_cast<const char*>(memmem(currentBoundary + boundaryLength, length - (currentBoundary + boundaryLength - reinterpret_cast<const char*>(data)), boundary.data(), boundaryLength));
> 
> Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?
> 
> > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:203
> > +            nextBoundary = static_cast<const char*>(memmem(nextBoundary + boundaryLength, length - (nextBoundary + boundaryLength - reinterpret_cast<const char*>(data)), boundary.data(), boundaryLength));
> 
> Ditto.

Will check.

> 
> > Source/WebCore/Modules/highlight/AppHighlight.h:55
> > +    encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(highlight->data()), highlight->size(), 1);
> 
> Why is a cast needed here? Doesn’t data() return a uint8_t*?

Will drop.

> 
> > Source/WebCore/Modules/indexeddb/IDBGetResult.cpp:37
> > +    m_value = ThreadSafeDataBuffer::create({ buffer.data(), buffer.size() });
> 
> No cleaner idiom? Maybe a std::span or Vector?
> 
> > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:172
> > +        const uint8_t* data = reinterpret_cast<const uint8_t*>(buffer->data.data<uint8_t>());
> 
> auto would be better here.
> 
> Why is reinterpret_cast needed here? What type does data<uint8_t>() return?
> Why not uint8_t*?

Will use auto and drop the cast.

> 
> > Source/WebCore/css/CSSFontFaceSource.cpp:168
> > -                    m_generatedOTFBuffer = SharedBuffer::create(WTFMove(otfFont.value()));
> > +                    m_generatedOTFBuffer = SharedBuffer::create(otfFont->data(), otfFont->size());
> 
> I think this is now copying instead of adopting the Vector. That’s not as
> good.

It is. I am planning to look into this in a follow-up.

> > Source/WebCore/fileapi/Blob.cpp:117
> > +    blobParts.append(Vector { buffer.data(), buffer.size() });
> 
> This seems to call for std::span. Or doesn’t SharedBuffer have a function
> that returns a Vector?

AFAIK, std::span is not a thing YET in WebKit :) SharedBuffer doesn't have a function that returns a Vector at the moment.

> 
> > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:583
> > +        NetworkResourcesData::ResourceData const* resourceData = m_resourcesData->maybeAddResourceData(requestId, reinterpret_cast<const uint8_t*>(data), dataLength);
> 
> Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?
> 
> > Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:159
> > +        m_client->didReceiveData(this, buffer ? buffer->data() : reinterpret_cast<const uint8_t*>(data), buffer ? buffer->size() : length);
> 
> Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

Will check.

> 
> > Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:157
> > -        content->append(WTFMove(part));
> > +        content->append(part.data(), part.size());
> 
> I think this is now copying instead of adopting the Vector. That’s not as
> good.
> 
> > Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:198
> > +        auto decodedData = base64Decode(content->data()   , content->size());
> 
> Should probably get rid of the extra spaces here.

Will fix.

> 
> > Source/WebCore/loader/cache/CachedSVGFont.cpp:92
> > -            m_convertedFont = SharedBuffer::create(WTFMove(convertedFont.value()));
> > +            m_convertedFont = SharedBuffer::create(convertedFont->data(), convertedFont->size());
> 
> Is this changing from adopting data to copying it, or did the old version
> already copy the data?
> 
> > Source/WebCore/platform/SharedBuffer.cpp:142
> > -    auto arrayBuffer = ArrayBuffer::tryCreateUninitialized(static_cast<unsigned>(size()), sizeof(char));
> > +    auto arrayBuffer = ArrayBuffer::tryCreateUninitialized(static_cast<unsigned>(size()), sizeof(uint8_t));
> 
> Not new: What guarantees size() fits in unsigned?
> 
> Both of these sizeof expression are silly ways to write the number 1. I
> think we could just write 1.

Will use 1.

> 
> > Source/WebCore/platform/SharedBuffer.cpp:232
> > +        [](const RetainPtr<CFDataRef>& data) { return reinterpret_cast<const uint8_t*>(CFDataGetBytePtr(data.get())); },
> 
> No typecast needed any more: CFDataGetBytePtr returns const uint8_t*.
> 
> > Source/WebCore/platform/SharedBuffer.cpp:235
> > +        [](const GRefPtr<GBytes>& data) { return reinterpret_cast<const uint8_t*>(g_bytes_get_data(data.get(), nullptr)); },
> 
> This should be static_cast, not reinterpret_cast; it’s a cast from a void*.
> 
> > Source/WebCore/platform/SharedBuffer.cpp:238
> > +        [](const RefPtr<GstMappedOwnedBuffer>& data) { return reinterpret_cast<const uint8_t*>(data->data()); },
> 
> No typecast needed any more: GstMappedOwnedBuffer::data returns const
> uint8_t*.
> 
> > Source/WebCore/platform/SharedBuffer.cpp:240
> > +        [](const FileSystem::MappedFileData& data) { return reinterpret_cast<const uint8_t*>(data.data()); }
> 
> This should be static_cast, not reinterpret_cast; it’s a cast from a void*.

Fixing all those.

> 
> > Source/WebCore/platform/SharedBuffer.cpp:357
> > +    char* p = reinterpret_cast<char*>(buffer.data());
> 
> Always a sad moment when we have to add a reinterpret_cast; any way to avoid
> this one?
> 
> > Source/WebCore/platform/SharedBuffer.cpp:370
> > +    buffer.shrink(p - reinterpret_cast<char*>(buffer.data()));
> 
> Always a sad moment when we have to add a reinterpret_cast; any way to avoid
> this one?
> 
> > Source/WebCore/platform/SharedBufferChunkReader.cpp:103
> > +                chunk.append(reinterpret_cast<const uint8_t*>(m_separator.data()), m_separatorIndex);
> 
> Always a sad moment when we have to add a reinterpret_cast; any way to avoid
> this one?

Will check.

> 
> > Source/WebCore/platform/graphics/WOFFFileFormat.cpp:297
> > -        buffer = SharedBuffer::create(WTFMove(convertedFont));
> > +        buffer = SharedBuffer::create(convertedFont.data(), convertedFont.size());
> 
> I think this is now copying instead of adopting the Vector. That’s not as
> good.
> 
> > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:239
> > +    const uint8_t* source = sharedBuffer->data() + position;
> 
> auto
> 
> > Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:89
> > +        size_t offset = reinterpret_cast<const uint8_t*>(position) - buffer.data();
> 
> This can be static_cast instead of reinterpret_cast since it’s from a void*.
> 
> > Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:318
> > +    return strncmp(&m_data->dataAsCharPtr()[imageOffset], "\x89PNG", 4) ? BMP : PNG;
> 
> This should use memcmp instead of strncmp--it was never useful to use
> strncmp here, no reason to handle null characters specially--and then we
> will not need dataAsCharPtr here.
> 
> > Source/WebCore/platform/network/FormData.cpp:417
> > +    auto bytes = flatten();
> > +    return SharedBuffer::create(bytes.data(), bytes.size());
> 
> I think this is now copying instead of adopting the Vector. That’s not as
> good.
> 
> > Source/WebCore/platform/win/SharedBufferWin.cpp:55
> > +        if (ReadFile(fileHandle, (LPVOID)buffer.data(), bytesToRead, &bytesRead, 0) && bytesToRead == bytesRead)
> 
> Why was this new typecast needed?

Probably not needed, will drop.

> 
> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:362
> > -    OffsetBuffer(Vector<char> buffer)
> > +    OffsetBuffer(Vector<uint8_t>&& buffer)
> 
> Nice optimization.
> 
> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:364
> >          , m_currentOffset(0)
> 
> We should move this initialization down to where the data member is defined
> below.

Will fix.

> 
> > Source/WebKit/WebProcess/Plugins/Netscape/NetscapePluginStream.cpp:120
> > +    deliverData(reinterpret_cast<const uint8_t*>(resultCString.data()), resultCString.length());
> 
> Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1172
> > +    m_pluginView->manualLoadDidReceiveData(reinterpret_cast<const uint8_t*>(data), length);
> 
> Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

Will check. Part of the issue is that I was trying to keep the size of this patch small. Switching to uint8_t propagates through the code base quickly. Right now, all the didReceiveData() functions take a const char* and I was planning to switch them all to const uint8_t* in the follow-up given the size.

[Chris Dumez]

Created attachment 430613 [details]
Patch

[Chris Dumez]

Created attachment 430614 [details]
Patch

[Chris Dumez]

Created attachment 430615 [details]
Patch

[Chris Dumez]

Created attachment 430617 [details]
Patch

[Chris Dumez]

Created attachment 430620 [details]
Patch

[Chris Dumez]

Created attachment 430622 [details]
Patch

[Chris Dumez]

Created attachment 430624 [details]
Patch

[Chris Dumez]

Created attachment 430628 [details]
Patch

[Sam Weinig]

Not to throw a wrench into this, but should we consider std::byte instead? That would be my preference all things being equal.

[Sam Weinig]

(In reply to Sam Weinig from comment #20)
> Not to throw a wrench into this, but should we consider std::byte instead?
> That would be my preference all things being equal.

That said, this is a great improvement, and we can always make that change another time if we want to, so don't block yourself on this question too much.

[Chris Dumez]

(In reply to Sam Weinig from comment #20)
> Not to throw a wrench into this, but should we consider std::byte instead?
> That would be my preference all things being equal.

lol, you're killing me. I saw you switch code from `const char*` to `const uint8_t*` just recently in one of your patches :)

[Sam Weinig]

(In reply to Chris Dumez from comment #22)
> (In reply to Sam Weinig from comment #20)
> > Not to throw a wrench into this, but should we consider std::byte instead?
> > That would be my preference all things being equal.
> 
> lol, you're killing me. I saw you switch code from `const char*` to `const
> uint8_t*` just recently in one of your patches :)

Ha! Sorry! As I said, I think this is a great improvement and makes things way more consistent. std::byte is more controversial, so I didn't want to make that change while making the base64 change. Having this in will make that kind of change much easier, so its all win-win. I just thought you and Darin would be a good audience for the question :).

[Chris Dumez]

(In reply to Sam Weinig from comment #23)
> (In reply to Chris Dumez from comment #22)
> > (In reply to Sam Weinig from comment #20)
> > > Not to throw a wrench into this, but should we consider std::byte instead?
> > > That would be my preference all things being equal.
> > 
> > lol, you're killing me. I saw you switch code from `const char*` to `const
> > uint8_t*` just recently in one of your patches :)
> 
> Ha! Sorry! As I said, I think this is a great improvement and makes things
> way more consistent. std::byte is more controversial, so I didn't want to
> make that change while making the base64 change. Having this in will make
> that kind of change much easier, so its all win-win. I just thought you and
> Darin would be a good audience for the question :).

I hadn't heard about std::byte before honestly. I kinda like the strong typing. However, I have a feeling that using it may cause us to have have to reinterpret_cast more, in cases where we call into library functions or system calls that expect a "const uint8_t*"? There's plenty of usage of `const uint8_t*` / `const unsigned char*` out there but not much `const std::byte*`.

For now, my priority is for us to be consistent. Right now we have some classes that use uint8_t and some others char.

[EWS]

Committed r278516 (238515@main): <https://commits.webkit.org/238515@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430628 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78894758>

[Darin Adler]

Wow, getting to std::byte at *some* point seems pretty neat. I want to learn more about it.