Remove convenience constructors for TextRun
Created attachment 252607 [details] Patch
Comment on attachment 252607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252607&action=review > Source/WebCore/platform/graphics/TextRun.h:57 > + explicit TextRun(StringView stringView, float xpos = 0, float expansion = 0, ExpansionBehavior expansionBehavior = AllowTrailingExpansion | ForbidLeadingExpansion, TextDirection direction = LTR, bool directionalOverride = false, bool characterScanForCodePath = true, RoundingHacks roundingHacks = RunRounding | WordRounding) > + : m_text(stringView) > + , m_charactersLength(stringView.length()) Maybe text instead of stringView? > Source/WebCore/rendering/SimpleLineLayout.cpp:208 > + if (style.fontCascade().codePath(TextRun(StringView(textRenderer.text()))) != FontCascade::Simple) I’m surprised the explicit cast to StringView is needed. I think we should fix whatever problem is leading to our need to do that. > Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:200 > - TextRun run(segment.text.characters<CharacterType>() + segmentFrom, segmentTo - segmentFrom); > + TextRun run(StringView(segment.text.characters<CharacterType>() + segmentFrom, segmentTo - segmentFrom)); I think this shows us a bit of a refactoring mistake. I’m not sure this function template needs to be a template at all, and if segment.text is already a StringView then this code should just be: TextRun run(segment.text.substring(segmentFrom, segmentTo - segmentFrom));
Created attachment 252663 [details] Patch for landing
Comment on attachment 252663 [details] Patch for landing r+ on the WTF changes.
(In reply to comment #4) > Comment on attachment 252663 [details] > Patch for landing > > r+ on the WTF changes. Darin r+'ed the rest of this so I'll land it.
Committed r183996: <http://trac.webkit.org/changeset/183996>
There are two new text-related failures on Windows after this commit: https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r183996%20(51695)/results.html I'm going to watch it a little longer before rolling out, it's not clear enough how this caused the problem.
Oh, this change broke all tests on ASan, that may explain it. Rolling out. ==272==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000109a64 at pc 0x0001170c9e77 bp 0x7fff564a04b0 sp 0x7fff564a04a8 READ of size 1 at 0x603000109a64 thread T0 ==272==WARNING: failed to fork external symbolizer (errno: 1) #0 0x1170c9e76 in WebCore::Latin1TextIterator::consume(int&, unsigned int&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x239fe76) #1 0x114e6ea72 in unsigned int WebCore::WidthIterator::advanceInternal<WebCore::Latin1TextIterator>(WebCore::Latin1TextIterator&, WebCore::GlyphBuffer*) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x144a72) #2 0x114d3b80b in WebCore::WidthIterator::advance(int, WebCore::GlyphBuffer*) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1180b) #3 0x1159489ec in WebCore::FontCascade::floatWidthForSimpleText(WebCore::TextRun const&, WTF::HashSet<WebCore::Font const*, WTF::PtrHash<WebCore::Font const*>, WTF::HashTraits<WebCore::Font const*> >*, WebCore::GlyphOverflow*) const (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0xc1e9ec) #4 0x1159487c1 in WebCore::FontCascade::width(WebCore::TextRun const&, WTF::HashSet<WebCore::Font const*, WTF::PtrHash<WebCore::Font const*>, WTF::HashTraits<WebCore::Font const*> >*, WebCore::GlyphOverflow*) const (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0xc1e7c1) #5 0x116bb7fb9 in float WebCore::SimpleLineLayout::TextFragmentIterator::runWidth<unsigned char>(WebCore::SimpleLineLayout::FlowContents::Segment const&, unsigned int, unsigned int, float) const (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1e8dfb9) #6 0x116bb7646 in WebCore::SimpleLineLayout::TextFragmentIterator::skipToNextPosition(WebCore::SimpleLineLayout::TextFragmentIterator::PositionType, unsigned int, float&, float, bool&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1e8d646) #7 0x116bb6ef9 in WebCore::SimpleLineLayout::TextFragmentIterator::findNextTextFragment(float) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1e8cef9) #8 0x116bb6aef in WebCore::SimpleLineLayout::TextFragmentIterator::nextTextFragment(float) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1e8caef) #9 0x116c7245a in WebCore::SimpleLineLayout::firstFragment(WebCore::SimpleLineLayout::TextFragmentIterator&, WebCore::SimpleLineLayout::LineState&, WebCore::SimpleLineLayout::LineState const&, WTF::Vector<WebCore::SimpleLineLayout::Run, 10ul, WTF::CrashOnOverflow, 16ul>&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1f4845a) #10 0x116c71ca2 in WebCore::SimpleLineLayout::createLineRuns(WebCore::SimpleLineLayout::LineState&, WebCore::SimpleLineLayout::LineState const&, WTF::Vector<WebCore::SimpleLineLayout::Run, 10ul, WTF::CrashOnOverflow, 16ul>&, WebCore::SimpleLineLayout::TextFragmentIterator&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1f47ca2) #11 0x116c71425 in WebCore::SimpleLineLayout::createTextRuns(WTF::Vector<WebCore::SimpleLineLayout::Run, 10ul, WTF::CrashOnOverflow, 16ul>&, WebCore::RenderBlockFlow&, unsigned int&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1f47425) #12 0x116c70f70 in WebCore::SimpleLineLayout::create(WebCore::RenderBlockFlow&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1f46f70) #13 0x116825114 in WebCore::RenderBlockFlow::layoutSimpleLines(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1afb114) #14 0x11681f702 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af5702) #15 0x114dc5ae2 in WebCore::RenderBlock::layout() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x9bae2) #16 0x116823ef3 in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af9ef3) #17 0x116821192 in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af7192) #18 0x11681f716 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af5716) #19 0x114dc5ae2 in WebCore::RenderBlock::layout() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x9bae2) #20 0x116823ef3 in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af9ef3) #21 0x116821192 in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af7192) #22 0x11681f716 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af5716) #23 0x114dc5ae2 in WebCore::RenderBlock::layout() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x9bae2) #24 0x116823ef3 in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af9ef3) #25 0x116821192 in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af7192) #26 0x11681f716 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af5716) #27 0x114dc5ae2 in WebCore::RenderBlock::layout() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x9bae2) #28 0x116823ef3 in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af9ef3) #29 0x116821192 in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af7192) #30 0x11681f716 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af5716) #31 0x114dc5ae2 in WebCore::RenderBlock::layout() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x9bae2) #32 0x116823ef3 in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af9ef3) #33 0x116821192 in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af7192) #34 0x11681f716 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1af5716) #35 0x114dc5ae2 in WebCore::RenderBlock::layout() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x9bae2) #36 0x116acfc1d in WebCore::RenderView::layoutContent(WebCore::LayoutState const&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1da5c1d) #37 0x114dc557c in WebCore::RenderView::layout() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x9b57c) #38 0x114db9ccb in WebCore::FrameView::layout(bool) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x8fccb) #39 0x114ea37e7 in WebCore::Document::updateLayout() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1797e7) #40 0x115680121 in WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x956121) #41 0x1150c5f62 in WebCore::Element::focus(bool, WebCore::FocusDirection) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x39bf62) #42 0x115124807 in WebCore::jsElementPrototypeFunctionFocus(JSC::ExecState*) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x3fa807) #43 0x5b6cb5c01027 (<unknown module>) #44 0x11418f2d4 in llint_entry (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0xa802d4) #45 0x1141897e5 in vmEntryToJavaScript (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0xa7a7e5) #46 0x113fad48f in JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x89e48f) #47 0x113742521 in JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x33521) #48 0x11373da20 in JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x2ea20) #49 0x1161a698d in WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x147c98d) #50 0x116b4b932 in WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1e21932) #51 0x114e3c9f8 in WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1129f8) #52 0x114e3c8dd in WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1128dd) #53 0x114e3b93d in WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x11193d) #54 0x114e3a5b4 in WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1105b4) #55 0x114e3a2c5 in WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x1102c5) #56 0x114e39d5d in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x10fd5d) #57 0x114d81c53 in WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x57c53) #58 0x114d80628 in WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x56628) #59 0x114eadf7d in WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x183f7d) #60 0x114f71cc1 in WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x247cc1) #61 0x114e57c67 in WebCore::CachedResource::checkNotify() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x12dc67) #62 0x114e57a28 in WebCore::SubresourceLoader::didFinishLoading(double) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x12da28) #63 0x1125ba235 in void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(double)>(IPC::MessageDecoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(double)) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebKit.framework/WebKit+0x8ca235) #64 0x1125b94aa in WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebKit.framework/WebKit+0x8c94aa) #65 0x112033cca in WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebKit.framework/WebKit+0x343cca) #66 0x111eb6706 in IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebKit.framework/WebKit+0x1c6706) #67 0x111ebca8c in IPC::Connection::dispatchOneMessage() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/WebKit.framework/WebKit+0x1cca8c) #68 0x1143b91f7 in WTF::RunLoop::performWork() (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0xcaa1f7) #69 0x1143ba02e in WTF::RunLoop::performWork(void*) (/Volumes/Data/slave/yosemite-asan-production-wk2-tests/build/buildToTest/JavaScriptCore.framework/Versions/
Re-opened since this is blocked by bug 144806
Comment on attachment 252663 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=252663&action=review > Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:200 > + TextRun run(StringView(segment.text.substring(segmentFrom, segmentTo - segmentFrom))); Well, this is embarrassing. segment.text is a string, and String::substring() returns a string, which, in this context, is a temporary. StringView is non-owning, so it immediately points into the temporary, which gets deleted as soon as this line concludes. Then, we use the StringView below.
Committed r184037: <http://trac.webkit.org/changeset/184037>
Comment on attachment 252663 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=252663&action=review >> Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:200 >> + TextRun run(StringView(segment.text.substring(segmentFrom, segmentTo - segmentFrom))); > > Well, this is embarrassing. segment.text is a string, and String::substring() returns a string, which, in this context, is a temporary. StringView is non-owning, so it immediately points into the temporary, which gets deleted as soon as this line concludes. Then, we use the StringView below. This is not what we want. This allocates memory and we don’t want to do that; copying the substring into a new StringImpl hurts performance in a way that should be measurable. Here’s what we want to stay efficient: TextRun run(StringView(segment.text).substring(segmentFrom, segmentTo - segmentFrom));
Committed r184138: <http://trac.webkit.org/changeset/184138>