Ensure StringView lifetime is correct inside InlineTextBox
Created attachment 285390 [details] Patch
Created attachment 285394 [details] Patch
Created attachment 285408 [details] Patch
Comment on attachment 285408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285408&action=review > Source/WebCore/ChangeLog:26 > + A TextRun contains a StringView. Previously, we had a helper function which > + could possibly create a new string, and then create a TextRun whose StringView > + pointed inside this new string. Then, the string would be destroyed because it > + was on the stack. > + > + Luckily, this is benign because this new string would always share a > + StringImpl with the string passed into the function (and the StringView > + would point into this StringImpl). However, relying on this is dangerous > + and we are trying to enforce StringView lifetime checks in > + https://bugs.webkit.org/show_bug.cgi?id=160384. > + > + The solution is to make a helper function which returns this built string > + so that the caller can own this built string, thereby extending its lifetime. > + Unfortunately, this built string can't be a StringView because the same > + mechanism must be used for text-combine, which returns originalText(), which > + may return a built string if it is a RenderCounter. Perhaps it would be > + valuable to migrate RenderCounter::originalText() to something else, or to > + divorce text-combine from originalText(), but that is out of scope for this > + patch. Seems to me another benefit of the patch is that StringView::substring is faster to use than String::substringSharingImpl, since it does not allocate and destroy a StringImpl on the heap. A reason to make this change that has nothing to do with StringView lifetime checks. The comment about the built string doesn’t seem quite complete; another reason the built string can’t be a StringView is that our code for hyphenation requires allocating a new string and in fact the code paths that have nothing to do with originalText are using String, not StringView, for the built string. > Source/WebCore/rendering/InlineTextBox.cpp:56 > +#include <wtf/text/StringBuilder.h> No need for StringBuilder; see below. > Source/WebCore/rendering/InlineTextBox.cpp:209 > + if (ePos == m_len && hasHyphen()) In the code below we use a boolean named "respectHyphen" to help make code that is otherwise exactly like this be more self documenting. I don’t prefer either version, but I suggest making them consistent. > Source/WebCore/rendering/InlineTextBox.cpp:502 > + String stringToRender; Not sure this variable name is clear enough given that it will be null in the normal case and yet we will render not render the null/empty string; instead we will render the text from the renderer. Maybe alternateStringToRender or alternateString? > Source/WebCore/rendering/InlineTextBox.cpp:503 > + // Don't hyphenate combined text. This comment doesn’t seem all that helpful. It says what the code below says, however it doesn’t say why, which is the real job of a comment. Maybe leave the comment out. Or if you leave the comment in, find a way to efficiently say why. > Source/WebCore/rendering/InlineTextBox.cpp:640 > + hyphenatedStringForTextRun(style, length); Oops, must not have a test case covering this. Because this line of code is missing "hyphenatedString ="! What now!?! > Source/WebCore/rendering/InlineTextBox.cpp:641 > + TextRun textRun = constructTextRun(style, hyphenatedString, Optional<unsigned>(length)); Why is the typecast to Optional<unsigned> needed? I would have expected this to work without a cast. Maybe the local variable was typed int in a previous iteration of the patch? > Source/WebCore/rendering/InlineTextBox.cpp:1011 > + auto string = StringView(renderer().text()).substring(start(), overriddenLength.valueOr(len())); Would be nice to share this line of code with constructTextRun, but I suppose that's not practical if we don’t want to call renderer() and start() twice there. If we know renderer().text() can never be null, and I think we can know that, then we can call StringView(*renderer().text()) and remove one unnecessary null check from the generated code. But maybe we don’t know that. If we do, then we may want to change RenderText::text() to return StringImpl&. The use of valueOr here unfortunately will always call len() even if overriddenLength is not null. Can fix by using valueOrCompute([this] () { return len(); }) but that is a lot more ugly and not sure if it’s more efficient. > Source/WebCore/rendering/InlineTextBox.cpp:1015 > + StringBuilder builder; > + builder.append(string); > + builder.append(style.hyphenString()); > + return builder.toString(); Should just write: return makeString(string, style.hyphenString()); Not sure, but it’s possible this will work too: return string + style.hyphenString(); This is more efficient than StringBuilder when simply concatenating two strings. Could also consider a single line rather than using a local variable: return StringView(renderer().text()).substring(start(), overriddenLength.valueOr(len())) + style.hyphenString(); > Source/WebCore/rendering/InlineTextBox.cpp:1023 > + return constructTextRun(style, StringView(renderer.text()).substring(start, overriddenLength.valueOr(len())), renderer.textLength() - start); Same issues here with text() and valueOr() as above. > Source/WebCore/rendering/InlineTextBox.cpp:1025 > + return constructTextRun(style, maybeStringToRender, maybeStringToRender.length()); Consider adding: ASSERT(!overriddenLength || overriddenLength == maybeStringToRender.length()); Unless that is not the invariant. > Source/WebCore/rendering/InlineTextBox.h:115 > + String hyphenatedStringForTextRun(const RenderStyle&, Optional<unsigned> overriddenLength = Nullopt) const; The name overrridenLength is not good. The length passed in is not the one that will be overridden, it’s the length that will do the overriding. So "overridden length" is not what we should call it. > Source/WebCore/rendering/InlineTextBox.h:116 > + TextRun constructTextRun(const RenderStyle&, StringView maybeStringToRender = StringView(), Optional<unsigned> overriddenLength = Nullopt) const; Same issue with the name overriddenLength here. Also, I would call this an alternateStringToRender, not a maybeStringToRender. Seems not entirely elegant that both of these are sort of optional arguments, but we use different naming scheme for each. Both are about overriding the default behavior. Unclear that overriddenLength will be ignored if an alternate string is passed. I think I prefer { } to StringView() for the default value here in the function declaration, not sure if you agree. > Source/WebCore/rendering/RenderCombineText.cpp:90 > + return originalText(); If originalText() can ever return a null string then the caller might not do the right thing. Are we sure it won’t ever? > Source/WebCore/rendering/RenderCombineText.h:39 > + String maybeGetCombinedStringForRendering() const; I think this function should just be named combinedStringForRendering. I understand that returns null if this is not combined, but I am not sure that "maybe get" is the best way to express that, nor that it needs to be expressed in the name.
Created attachment 285626 [details] Patch for committing
Created attachment 285627 [details] Patch for committing
Comment on attachment 285627 [details] Patch for committing Clearing flags on attachment: 285627 Committed r204276: <http://trac.webkit.org/changeset/204276>
Comment on attachment 285408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285408&action=review >> Source/WebCore/rendering/InlineTextBox.cpp:641 >> + TextRun textRun = constructTextRun(style, hyphenatedString, Optional<unsigned>(length)); > > Why is the typecast to Optional<unsigned> needed? I would have expected this to work without a cast. Maybe the local variable was typed int in a previous iteration of the patch? It's because there is an override that takes an unsigned and another override that takes an Optional<unsigned>. This needs to specify which override is correct. >> Source/WebCore/rendering/InlineTextBox.cpp:1011 >> + auto string = StringView(renderer().text()).substring(start(), overriddenLength.valueOr(len())); > > Would be nice to share this line of code with constructTextRun, but I suppose that's not practical if we don’t want to call renderer() and start() twice there. > > If we know renderer().text() can never be null, and I think we can know that, then we can call StringView(*renderer().text()) and remove one unnecessary null check from the generated code. But maybe we don’t know that. If we do, then we may want to change RenderText::text() to return StringImpl&. > > The use of valueOr here unfortunately will always call len() even if overriddenLength is not null. Can fix by using valueOrCompute([this] () { return len(); }) but that is a lot more ugly and not sure if it’s more efficient. Regarding the null check from RenderText::text() to StringView(StringImpl*), I think it's a good idea, but out of scope for this patch. Given that len() is inlined and is just a simple accessor, I don't think it's valuable to use the lambda. >> Source/WebCore/rendering/InlineTextBox.cpp:1025 >> + return constructTextRun(style, maybeStringToRender, maybeStringToRender.length()); > > Consider adding: > > ASSERT(!overriddenLength || overriddenLength == maybeStringToRender.length()); > > Unless that is not the invariant. That's not the invariant. Instead, maybeStringToRender will be longer than overriddenLength by the hyphen string's length (if maybeStringToRender is not null). However, I think this function shouldn't know anything about hyphenation, and should just deal with the data it's given, as it's given.
Comment on attachment 285408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285408&action=review >>> Source/WebCore/rendering/InlineTextBox.cpp:641 >>> + TextRun textRun = constructTextRun(style, hyphenatedString, Optional<unsigned>(length)); >> >> Why is the typecast to Optional<unsigned> needed? I would have expected this to work without a cast. Maybe the local variable was typed int in a previous iteration of the patch? > > It's because there is an override that takes an unsigned and another override that takes an Optional<unsigned>. This needs to specify which override is correct. Might be worth revisiting. It seems like a dangerously ambiguous programming interface if the unsigned and the Optional<unsigned> arguments don’t have the same meaning. >>> Source/WebCore/rendering/InlineTextBox.cpp:1025 >>> + return constructTextRun(style, maybeStringToRender, maybeStringToRender.length()); >> >> Consider adding: >> >> ASSERT(!overriddenLength || overriddenLength == maybeStringToRender.length()); >> >> Unless that is not the invariant. > > That's not the invariant. Instead, maybeStringToRender will be longer than overriddenLength by the hyphen string's length (if maybeStringToRender is not null). However, I think this function shouldn't know anything about hyphenation, and should just deal with the data it's given, as it's given. Given that philosophy, why is it OK for this code path to ignore overriddenLength?