Adding Utf8String class to directly hold utf8 strings passed via JSRT #5348
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| //------------------------------------------------------------------------------------------------------- | ||
| // Copyright (C) Microsoft. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information. | ||
| //------------------------------------------------------------------------------------------------------- | ||
| #pragma once | ||
|
|
||
| #include "Codex/Utf8Codex.h" | ||
|
|
||
| namespace Js | ||
| { | ||
| class Utf8String : public JavascriptString | ||
| { | ||
| private: | ||
| typedef struct { | ||
| FieldNoBarrier(size_t) length; | ||
|
There was a problem hiding this comment. Why size_t not charcount_t? There was a problem hiding this comment. This is the length of the utf8 string, which can be up to 3 times as long as the same string in utf16. Since a utf16 string can be up to ~2^31 in chakra, the corresponding utf8 string may be more than 2^32 in length, so utf8 lengths need to be size_t There was a problem hiding this comment. |
||
| Field(char*) buffer; | ||
| } PrefixedUtf8String; | ||
| Field(PrefixedUtf8String*) utf8String; | ||
|
There was a problem hiding this comment. If PrefixedUtf8String is only two pointers big, could we just put it directly in the object rather than requiring a separate allocation (and a separate pointer dereference every time we use it)? There was a problem hiding this comment. Part of the original motivation of this was to allow other kinds of strings to convert themselves into a There was a problem hiding this comment. What had only one pointer's worth of space? All allocations are bumped up to the nearest 16 bytes anyway, so a 40-byte object actually gets allocated as 48 In reply to: 205622702 [](ancestors = 205622702) There was a problem hiding this comment. I believe it was specifically on 32bit one of the types was otherwise full... I don't recall which at the moment though. |
||
|
|
||
| protected: | ||
| DEFINE_VTABLE_CTOR(Utf8String, JavascriptString); | ||
|
|
||
| private: | ||
|
|
||
| void SetUtf8Buffer(_In_reads_(utf8Length) char* buffer, size_t utf8Length) | ||
|
There was a problem hiding this comment. I very much prefer reading the code with it all in the header like this, but I think we're meant to put most function definitions in cpp files unless they're templates |
||
| { | ||
| this->utf8String = RecyclerNew(this->GetRecycler(), PrefixedUtf8String); | ||
| this->utf8String->length = utf8Length; | ||
| this->utf8String->buffer = buffer; | ||
| } | ||
|
|
||
| public: | ||
|
|
||
| Utf8String(_In_ JavascriptString* originalString, _In_reads_(utf8Length) char* buffer, size_t utf8Length, _In_ StaticType* type) : | ||
| JavascriptString(type), | ||
| utf8String(nullptr) | ||
| { | ||
| SetUtf8Buffer(buffer, utf8Length); | ||
|
|
||
| this->SetLength(originalString->GetLength()); | ||
| this->SetBuffer(originalString->UnsafeGetBuffer()); | ||
|
There was a problem hiding this comment. This worries me. Maybe
There was a problem hiding this comment. My thinking was that this should keep the finalized-ness of the source string: if the original string isn't finalized, it won't have a buffer so we would be setting it to nullptr (and then if someone asks for it, we generate it from the utf8 version), and otherwise it was finalized so we can remain finalized. I hadn't considered the case when a string would have an invalid buffer like a substring would. Using There was a problem hiding this comment. I'd be fine with something that said explicitly "if finalized, GetSz, else null", but UnsafeGetBuffer is marked unsafe for a reason :) In reply to: 205623819 [](ancestors = 205623819) There was a problem hiding this comment. Actually even with GetSz the PropertyString problem persists. It returns an interior pointer to a PropertyRecord, which is kept alive by the string itself. We risk it getting collected from under us with this setup. In reply to: 205624092 [](ancestors = 205624092,205623819) There was a problem hiding this comment. Buffer ownership is also a very real problem with the conversion code: we swap out a vtable, overwrite our owning pointer with some other data, and then our data might get collected. In reply to: 205624399 [](ancestors = 205624399,205624092,205623819) |
||
| } | ||
|
|
||
| Utf8String(_In_reads_(utf8Length) char* buffer, size_t utf8Length, _In_ StaticType* type) : | ||
| JavascriptString(type), | ||
| utf8String(nullptr) | ||
| { | ||
| SetUtf8Buffer(buffer, utf8Length); | ||
|
|
||
| charcount_t utf16Length = 0; | ||
| utf8::DecodeOptions opts = utf8::DecodeOptions::doDefault; | ||
| LPCUTF8 buf = reinterpret_cast<LPCUTF8>(buffer); | ||
| LPCUTF8 end = buf + utf8Length; | ||
| while (buf < end) | ||
| { | ||
| if ((*buf & 0x80) == 0) | ||
| { | ||
| // Single byte character | ||
| utf16Length++; | ||
| buf++; | ||
| } | ||
| else | ||
| { | ||
| // Decode a single utf16 character, and increment the pointer | ||
| // to the start of the next character. | ||
| char16 c1 = *buf; | ||
| ++buf; | ||
| utf8::DecodeTail(c1, buf, end, opts); | ||
| utf16Length++; | ||
| } | ||
| } | ||
|
|
||
| this->SetLength(utf16Length); | ||
| this->SetBuffer(nullptr); | ||
| } | ||
|
|
||
|
|
||
| size_t Utf8Length() const | ||
| { | ||
| return this->utf8String->length; | ||
| } | ||
|
|
||
| const char* Utf8Buffer() const | ||
| { | ||
| return this->utf8String->buffer; | ||
| } | ||
|
|
||
| const char16* GetSz() override | ||
| { | ||
| if (this->IsFinalized()) | ||
| { | ||
| return this->UnsafeGetBuffer(); | ||
|
There was a problem hiding this comment. If this was the result of ConvertString from a SubString, then this buffer might not be null terminated, violating the rules of GetSz. This part I think is not actually dangerous, because any substring should have a null character eventually in the source string it came from, but we might end up reading a lot more data than intended. |
||
| } | ||
|
|
||
| // TODO: This is currently wrong in the presence of unmatched surrogate pairs. | ||
|
There was a problem hiding this comment. Nit: it might be nice to move this comment down to right before DecodeUnitsIntoAndNullTerminateNoAdvance, because in this position I originally though it was talking about the allocation size being wrong (which would be Very Bad). There was a problem hiding this comment. I think that's also not the 'correct' place for this comment; been a while since I wrote it. The actual issue is with the encoding into utf8, and determining how to decode it. As things stand I'm using 'actual' utf8 which doesn't allow lone surrogate halves, but we could instead use 'cesu-8' which does allow that. There was a problem hiding this comment. As far as I know canonical utf-8 doesn’t encode surrogate halves at all (paired or otherwise)—the character is encoded directly. If you need to preserve the surrogates independently I think cesu-8 is your only option. |
||
| // They will be converted to a 3 byte replacement character in utf8, and then | ||
| // converted back into a 1 utf16 character 0xfffd representation of that, not the original | ||
| // utf16 surrogate pair character | ||
| char16* buffer = RecyclerNewArrayLeaf(this->GetRecycler(), char16, this->GetLength() + 1); | ||
|
|
||
| // Fetching the char* from the Field(char*) first so we can then cast to LPCUTF8 | ||
| const char* bufferStart = this->utf8String->buffer; | ||
| LPCUTF8 start = reinterpret_cast<LPCUTF8>(bufferStart); | ||
| size_t decodeLength = utf8::DecodeUnitsIntoAndNullTerminateNoAdvance(buffer, start, start + this->utf8String->length); | ||
|
|
||
| Assert(decodeLength == this->GetLength()); | ||
|
|
||
| buffer[this->GetLength()] = 0; | ||
|
There was a problem hiding this comment. Seems to already be done by the |
||
|
|
||
| this->SetBuffer(buffer); | ||
| return this->UnsafeGetBuffer(); | ||
| } | ||
|
|
||
| static bool Is(RecyclableObject* obj) | ||
| { | ||
| return VirtualTableInfo<Js::Utf8String>::HasVirtualTable(obj); | ||
| } | ||
|
|
||
| static Utf8String* From(RecyclableObject* obj) | ||
| { | ||
| if (Utf8String::Is(obj)) | ||
| { | ||
| return static_cast<Utf8String*>(obj); | ||
| } | ||
|
|
||
| return nullptr; | ||
| } | ||
|
|
||
| template <typename StringType> | ||
| static Utf8String * ConvertString(StringType * originalString, _In_reads_(utf8Length) char* buffer, size_t utf8Length) | ||
| { | ||
| VirtualTableInfo<Utf8String>::SetVirtualTable(originalString); | ||
|
There was a problem hiding this comment. I kind of expected SetVirtualTable to be a template that static-asserted that the source type was big enough for the destination type, but I don't see any such assertion. Could you please add one? Otherwise this is concerning because LiteralString is not big enough to convert successfully. |
||
| Utf8String * convertedString = (Utf8String *)originalString; | ||
|
|
||
| convertedString->SetUtf8Buffer(buffer, utf8Length); | ||
|
|
||
| // length and buffer are preserved from the original string, since that is part of JavascriptString | ||
|
|
||
| return convertedString; | ||
| } | ||
| }; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@boingoing I've refactored this method and added a use in
CompileRun. As part of that I noticed that this method previously didn't cater for the case of anExternalArrayBufferwith utf16 encoding; my change supports that, but I'm not sure if that was intentional or not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that case was an oversight. Thanks for cleaning this up, looks good.