Convert ContentSecurityPolicy related parsers over to using StringParsingBuffer
Created attachment 402836 [details] Patch
Created attachment 402839 [details] Patch
Created attachment 402874 [details] Patch
Comment on attachment 402874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402874&action=review > Source/WebCore/html/parser/ParsingUtilities.h:38 > +template<typename CharType> > +inline bool isNotASCIISpace(CharType c) CharacterType, single line, constexpr? > Source/WebCore/html/parser/ParsingUtilities.h:44 > +template<typename CharType, typename DelimiterType> > +bool skipExactly(const CharType*& position, const CharType* end, DelimiterType delimiter) CharacterType, single line? > Source/WebCore/html/parser/ParsingUtilities.h:54 > +template<typename CharType, typename DelimiterType> > +bool skipExactly(StringParsingBuffer<CharType>& parsingBuffer, DelimiterType delimiter) CharacterType, single line? I suggest naming the local variable just "buffer". Single words seem to work well in short functions. > Source/WebCore/html/parser/ParsingUtilities.h:74 > +template<typename CharType, bool characterPredicate(CharType)> > +bool skipExactly(StringParsingBuffer<CharType>& parsingBuffer) CharacterType, single line? I suggest naming the local variable just "buffer". Single words seem to work well in short functions. > Source/WebCore/html/parser/ParsingUtilities.h:84 > +template<typename CharType, typename DelimiterType> > +void skipUntil(const CharType*& position, const CharType* end, DelimiterType delimiter) CharacterType, single line? > Source/WebCore/html/parser/ParsingUtilities.h:91 > +template<typename CharType, typename DelimiterType> > +void skipUntil(StringParsingBuffer<CharType>& parsingBuffer, DelimiterType delimiter) CharacterType, single line? > Source/WebCore/html/parser/ParsingUtilities.h:105 > template<typename CharType, bool characterPredicate(CharType)> > +void skipUntil(StringParsingBuffer<CharType>& parsingBuffer) CharacterType, single line, "buffer"? > Source/WebCore/html/parser/ParsingUtilities.h:119 > template<typename CharType, bool characterPredicate(CharType)> > +void skipWhile(StringParsingBuffer<CharType>& parsingBuffer) CharacterType, single line, "buffer"? > Source/WebCore/html/parser/ParsingUtilities.h:143 > +template<typename CharacterType, unsigned lowercaseLettersLength> I think that "length" is not a great name for the size of an array that contains a null-terminated string. We use it elsewhere, but it seems like a mistake. > Source/WebCore/html/parser/ParsingUtilities.h:146 > + if (parsingBuffer.lengthRemaining() < lowercaseLettersLength - 1) Seems like the whole function would be better if it said: constexpr auto lowercaseLettersLength = lowercaseLettersArraySize - 1; ASSERT(!lowercaseLetters[lowercaseLettersLength]); Maybe with some kind of comment, and might need to be the right kind of assert, maybe not ASSERT. > Source/WebCore/html/parser/ParsingUtilities.h:150 > + parsingBuffer += (lowercaseLettersLength - 1); Not sure the parentheses make this clearer. > Source/WebCore/loader/ResourceCryptographicDigest.cpp:37 > +template<typename CharacterType> static Optional<ResourceCryptographicDigest::Algorithm> parseHashAlgorithmAdvancingPosition(StringParsingBuffer<CharacterType>& parsingBuffer) "buffer" > Source/WebCore/loader/ResourceCryptographicDigest.cpp:52 > +template<typename CharacterType> static Optional<ResourceCryptographicDigest> parseCryptographicDigestImpl(StringParsingBuffer<CharacterType>& parsingBuffer) "buffer" > Source/WebCore/loader/ResourceCryptographicDigest.cpp:67 > + skipExactly<CharacterType>(parsingBuffer, '='); > + skipExactly<CharacterType>(parsingBuffer, '='); Compiler can’t deduce CharacterType? > Source/WebCore/loader/ResourceCryptographicDigest.cpp:82 > +Optional<ResourceCryptographicDigest> parseCryptographicDigest(StringParsingBuffer<UChar>& parsingBuffer) "buffer" (I’ll stop commenting this each time) > Source/WebCore/page/csp/ContentSecurityPolicyMediaListDirective.cpp:39 > +template<typename CharacterType> > +static bool isMediaTypeCharacter(CharacterType c) Single-line?
Created attachment 402884 [details] Patch
Comment on attachment 402884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402884&action=review > Source/WebCore/html/parser/ParsingUtilities.h:37 > +template<typename CharacterType> inline bool isNotASCIISpace(CharacterType c) Just curious, why not constexpr? Also, really no need to write inline here — can’t imagine it has any effect. > Source/WebCore/html/parser/ParsingUtilities.h:127 > + if (!WTF::equalLettersIgnoringASCIICase(position, lowercaseLettersLength - 1, lowercaseLetters)) I accidentally tricked you into breaking the code! The "-1" here is now wrong. > Source/WebCore/html/parser/ParsingUtilities.h:129 > + position += (lowercaseLettersLength - 1); Ditto. > Source/WebCore/html/parser/ParsingUtilities.h:141 > + buffer += (lowercaseLettersLength); Parentheses here are peculiar.
(In reply to Darin Adler from comment #7) > Comment on attachment 402884 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=402884&action=review > > > Source/WebCore/html/parser/ParsingUtilities.h:37 > > +template<typename CharacterType> inline bool isNotASCIISpace(CharacterType c) > > Just curious, why not constexpr? Also, really no need to write inline here — > can’t imagine it has any effect. No good reason, just didn't think of it, will fix. > > > Source/WebCore/html/parser/ParsingUtilities.h:127 > > + if (!WTF::equalLettersIgnoringASCIICase(position, lowercaseLettersLength - 1, lowercaseLetters)) > > I accidentally tricked you into breaking the code! The "-1" here is now > wrong. > > > Source/WebCore/html/parser/ParsingUtilities.h:129 > > + position += (lowercaseLettersLength - 1); > > Ditto. Yikes. > > > Source/WebCore/html/parser/ParsingUtilities.h:141 > > + buffer += (lowercaseLettersLength); > > Parentheses here are peculiar. Yeah, those are dumb. Will remove.
(In reply to Sam Weinig from comment #8) > (In reply to Darin Adler from comment #7) > > Comment on attachment 402884 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=402884&action=review > > > > > Source/WebCore/html/parser/ParsingUtilities.h:37 > > > +template<typename CharacterType> inline bool isNotASCIISpace(CharacterType c) > > > > Just curious, why not constexpr? Also, really no need to write inline here — > > can’t imagine it has any effect. > > No good reason, just didn't think of it, will fix. Oh, actually, now I remember why. isASCIISpace is not constexpr yet. Was going to address that in a follow up.
Created attachment 402889 [details] Patch
Occurs to me that having this tightly bound pair of pointer/end can be used not just for refactoring, but for additional hardening. If some of those assertions are changed to RELEASE_ASSERT. Of course, then it makes it a consequential decision if you get a pointer from the buffer and use it, since that gets rid of the hardening. Seems like this is not our current goal, but an interesting future possibility.
Committed r263581: <https://trac.webkit.org/changeset/263581> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402889 [details].
<rdar://problem/64820967>
<rdar://problem/64820968>
(In reply to Darin Adler from comment #11) > Occurs to me that having this tightly bound pair of pointer/end can be used > not just for refactoring, but for additional hardening. If some of those > assertions are changed to RELEASE_ASSERT. > > Of course, then it makes it a consequential decision if you get a pointer > from the buffer and use it, since that gets rid of the hardening. > > Seems like this is not our current goal, but an interesting future > possibility. I think there are additional assertions (not necessarily release ones) beyond what is there so far such as adding the UnderlyingString type checks that StringView has to StringParsingBuffer.