Created attachment 182800 [details] Patch

Comment on attachment 182800 [details] Patch ok.

Comment on attachment 182800 [details] Patch Clearing flags on attachment: 182800 Committed r139760: <http://trac.webkit.org/changeset/139760>

All reviewed patches have been landed. Closing bug.

Comment on attachment 182800 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182800&action=review > Source/WebCore/xml/parser/MarkupTokenBase.h:529 > + size_t charactersLength() const > + { > + ASSERT(m_type == Token::Type::Character); > + return m_externalCharactersLength; > } While you are at it: this would be better as unsigned instead of size_t. Character buffers are usually unsigned (WTFString included). > Source/WebCore/xml/parser/MarkupTokenBase.h:588 > + size_t m_externalCharactersLength; ditto.

Comment on attachment 182800 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182800&action=review >> Source/WebCore/xml/parser/MarkupTokenBase.h:529 >> } > > While you are at it: this would be better as unsigned instead of size_t. > Character buffers are usually unsigned (WTFString included). abarth tells me that on a 64bit system, unsigned will still be 32 bits but size_t will be 64. Since Vectors are arbitrary length, using size_t is appropriate here.

I wonder if we should crash if we get a character token that's larger than 32 bits. I bet we do later anyway due to overflow checks in StringImpl.

> abarth tells me that on a 64bit system, unsigned will still be 32 bits but size_t will be 64. That is correct. > Since Vectors are arbitrary length, using size_t is appropriate here. Well, that's all nice when creating the Token, but the method you call will get you un trouble. E.g.: String characters = String(token->characters(), token->charactersLength()); Your patch changed from using Vector types to Character types, which is why I suggest we move to string conventions. > I wonder if we should crash if we get a character token that's larger than 32 bits. I bet we do later anyway due to overflow checks in StringImpl. Yep, I think you are right. Seeing what is here, I think that's the way to make this correct.

Yeah. Would you be willing to file a bug about this issue? The issue pre-dates this patch since the old code used Vector::size as well.