Do not perform 8 to 16bits characters conversion when converting a WTFString to NSString/CFString. <rdar://problem/11822873>
Created attachment 151135 [details] Patch
Comment on attachment 151135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151135&action=review > Source/WebCore/ChangeLog:19 > + (WTF::String::createCFString): CFSTR() create static CFString, it is unecessary to retain it. Is this true on Windows as well?
> > Source/WebCore/ChangeLog:19 > > + (WTF::String::createCFString): CFSTR() create static CFString, it is unecessary to retain it. > > Is this true on Windows as well? I don't know for sure but it is used elsewhere without extra retain on Windows (e.g. LocalizedStringsWin.cpp).
Comment on attachment 151135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151135&action=review > Source/WebCore/platform/text/cf/StringImplCF.cpp:139 > + return CFStringCreateWithBytes(0, reinterpret_cast<const UInt8*>(characters8()), m_length, kCFStringEncodingASCII, false); I think this should be Latin-1 and not ASCII. > Source/WebCore/platform/text/cf/StringImplCF.cpp:150 > + string = CFStringCreateWithBytesNoCopy(allocator, reinterpret_cast<const UInt8*>(characters8()), m_length, kCFStringEncodingASCII, false, kCFAllocatorNull); Ditto.
Comment on attachment 151135 [details] Patch > > Source/WebCore/platform/text/cf/StringImplCF.cpp:139 > > + return CFStringCreateWithBytes(0, reinterpret_cast<const UInt8*>(characters8()), m_length, kCFStringEncodingASCII, false); > > I think this should be Latin-1 and not ASCII. Oops, you are right, that could have ended up badly. I'll update.
Created attachment 153888 [details] Patch
Comment on attachment 153888 [details] Patch r=me
Comment on attachment 153888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153888&action=review > Source/WebCore/platform/text/cf/StringImplCF.cpp:141 > + if (!m_length || !isMainThread()) { > + if (is8Bit()) > + return CFStringCreateWithBytes(0, reinterpret_cast<const UInt8*>(characters8()), m_length, kCFStringEncodingISOLatin1, false); > + return CFStringCreateWithCharacters(0, reinterpret_cast<const UniChar*>(characters16()), m_length); > + } Seems to me that since we’re already checking explicitly for m_length, we should just return CFSTR("") here for that case, rather than sharing code with the general non-main thread case and calling CFStringCreateWithBytes or CFStringCreateWithCharacters.
> Seems to me that since we’re already checking explicitly for m_length, we should just return CFSTR("") here for that case, rather than sharing code with the general non-main thread case and calling CFStringCreateWithBytes or CFStringCreateWithCharacters. The reasons I did not do that is m_length == 0 is very uncommon in practice. I did not want to add a special case for something that almost never happen.
Committed r123504: <http://trac.webkit.org/changeset/123504>
This patch broke a bunch of text related layout tests in chromium mac debug canvas/philip/tests/2d.text.draw.fill.basic.html fast/css/font-face-default-font.html fast/forms/text-control-intrinsic-widths.html fast/lists/w3-css3-list-styles-fallback-style.html fast/text/custom-font-data-crash2.html fast/text/format-control.html fast/writing-mode/text-orientation-basic.html http/tests/misc/SVGFont-delayed-load.html platform/chromium/virtual/gpu/canvas/philip/tests/2d.text.draw.fill.basic.html platform/chromium/virtual/gpu/fast/canvas/text-globalAlpha.html svg/W3C-SVG-1.1/text-fonts-01-t.svg svg/W3C-SVG-1.1/text-intro-01-t.svg svg/animations/animate-elem-02-t-drt.html svg/custom/font-face-move.svg svg/custom/svg-allowed-in-dashboard-object.html svg/custom/svg-fonts-segmented.xhtml svg/custom/use-multiple-on-nested-disallowed-font.html svg/text/text-fonts-01-t.svg svg/text/text-path-01-b.svg tables/mozilla_expected_failures/bugs/bug89315.html
The failure is below: crash log for DumpRenderTree (pid 4157): STDOUT: <empty> STDERR: ASSERTION FAILED: !StringWrapperCFAllocator::currentString STDERR: /Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../platform/text/cf/StringImplCF.cpp(155) : CFStringRef WTF::StringImpl::createCFString() STDERR: 1 0x3c1d6d0e WTF::StringImpl::createCFString() STDERR: 2 0x3c1d974c WTF::StringImpl::operator NSString*() STDERR: 3 0x3c1f32af WTF::String::operator NSString*() const STDERR: 4 0x3c0f420c WTF::AtomicString::operator NSString*() const STDERR: 5 0x3c0f3d48 WebCore::FontCache::createFontPlatformData(WebCore::FontDescription const&, WTF::AtomicString const&) STDERR: 6 0x3bfe1597 WebCore::FontCache::getCachedFontPlatformData(WebCore::FontDescription const&, WTF::AtomicString const&, bool) STDERR: 7 0x3bfe1b37 WebCore::FontCache::getCachedFontData(WebCore::FontDescription const&, WTF::AtomicString const&, bool, WebCore::FontCache::ShouldRetain) STDERR: 8 0x3bfe2c86 WebCore::FontCache::getFontData(WebCore::Font const&, int&, WebCore::FontSelector*) STDERR: 9 0x3bffb002 WebCore::FontFallbackList::fontDataAt(WebCore::Font const*, unsigned int) const STDERR: 10 0x3c0e32a7 WebCore::Font::fontDataAt(unsigned int) const STDERR: 11 0x3bffd621 WebCore::Font::glyphDataAndPageForCharacter(int, bool, WebCore::FontDataVariant) const STDERR: 12 0x3bffcef3 WebCore::Font::glyphDataForCharacter(int, bool, WebCore::FontDataVariant) const STDERR: 13 0x3c06863d WebCore::WidthIterator::glyphDataForCharacter(int, bool, int, unsigned int&) STDERR: 14 0x3c06892c WebCore::WidthIterator::advance(int, WebCore::GlyphBuffer*) STDERR: 15 0x3d8f8633 WebCore::SVGTextMetricsBuilder::advanceSimpleText() STDERR: 16 0x3d8f8352 WebCore::SVGTextMetricsBuilder::advance() STDERR: 17 0x3d8f8c2a WebCore::SVGTextMetricsBuilder::measureTextRenderer(WebCore::RenderSVGInlineText*, WebCore::MeasureTextData*) STDERR: 18 0x3d8f900d WebCore::SVGTextMetricsBuilder::walkTree(WebCore::RenderObject*, WebCore::RenderSVGInlineText*, WebCore::MeasureTextData*) STDERR: 19 0x3d8f9280 WebCore::SVGTextMetricsBuilder::buildMetricsAndLayoutAttributes(WebCore::RenderSVGText*, WebCore::RenderSVGInlineText*, WTF::HashMap<unsigned int, WebCore::SVGCharacterData, WTF::IntHash<unsigned int>, WTF::HashTraits<unsigned int>, WTF::HashTraits<WebCore::SVGCharacterData> >&) STDERR: 20 0x3d8ef253 WebCore::SVGTextLayoutAttributesBuilder::buildLayoutAttributesForForSubtree(WebCore::RenderSVGText*) STDERR: 21 0x3d8b563b WebCore::RenderSVGText::layout() STDERR: 22 0x3d8c8bd4 WebCore::SVGRenderSupport::layoutChildren(WebCore::RenderObject*, bool) STDERR: 23 0x3d86c8e2 WebCore::RenderSVGContainer::layout() STDERR: 24 0x3d8c8bd4 WebCore::SVGRenderSupport::layoutChildren(WebCore::RenderObject*, bool) STDERR: 25 0x3d86c8e2 WebCore::RenderSVGContainer::layout() STDERR: 26 0x3d8c8bd4 WebCore::SVGRenderSupport::layoutChildren(WebCore::RenderObject*, bool) STDERR: 27 0x3d8a860c WebCore::RenderSVGRoot::layout() STDERR: 28 0x3d07749d WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) STDERR: 29 0x3d06db88 WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::FractionalLayoutUnit&) STDERR: 30 0x3d06b147 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) STDERR: 31 0x3d06a165 WebCore::RenderBlock::layout() STDERR: [4157:-1602484928:5699341254117:ERROR:process_util_posix.cc(143)] Received signal 11 STDERR: 0 DumpRenderTree 0x3a4807cf base::debug::StackTrace::StackTrace() + 63 STDERR: 1 DumpRenderTree 0x3a48076b base::debug::StackTrace::StackTrace() + 43 STDERR: 2 DumpRenderTree 0x3a536d27 base::(anonymous namespace)::StackDumpSignalHandler(int, __siginfo*, __darwin_ucontext*) + 295 STDERR: 3 libSystem.B.dylib 0x9539405b _sigtramp + 43 STDERR: 4 ??? 0xffffffff 0x0 + 4294967295 STDERR: 5 DumpRenderTree 0x3c1d974c WTF::StringImpl::operator NSString*() + 44 STDERR: 6 DumpRenderTree 0x3c1f32af WTF::String::operator NSString*() const + 95 STDERR: 7 DumpRenderTree 0x3c0f420c WTF::AtomicString::operator NSString*() const + 44 STDERR: 8 DumpRenderTree 0x3c0f3d48 WebCore::FontCache::createFontPlatformData(WebCore::FontDescription const&, WTF::AtomicString const&) + 152 STDERR: 9 DumpRenderTree 0x3bfe1597 WebCore::FontCache::getCachedFontPlatformData(WebCore::FontDescription const&, WTF::AtomicString const&, bool) + 567 STDERR: 10 DumpRenderTree 0x3bfe1b37 WebCore::FontCache::getCachedFontData(WebCore::FontDescription const&, WTF::AtomicString const&, bool, WebCore::FontCache::ShouldRetain) + 103 STDERR: 11 DumpRenderTree 0x3bfe2c86 WebCore::FontCache::getFontData(WebCore::Font const&, int&, WebCore::FontSelector*) + 438 STDERR: 12 DumpRenderTree 0x3bffb002 WebCore::FontFallbackList::fontDataAt(WebCore::Font const*, unsigned int) const + 466 STDERR: 13 DumpRenderTree 0x3c0e32a7 WebCore::Font::fontDataAt(unsigned int) const + 199 STDERR: 14 DumpRenderTree 0x3bffd621 WebCore::Font::glyphDataAndPageForCharacter(int, bool, WebCore::FontDataVariant) const + 1777 STDERR: 15 DumpRenderTree 0x3bffcef3 WebCore::Font::glyphDataForCharacter(int, bool, WebCore::FontDataVariant) const + 99 STDERR: 16 DumpRenderTree 0x3c06863d WebCore::WidthIterator::glyphDataForCharacter(int, bool, int, unsigned int&) + 333 STDERR: 17 DumpRenderTree 0x3c06892c WebCore::WidthIterator::advance(int, WebCore::GlyphBuffer*) + 684 STDERR: 18 DumpRenderTree 0x3d8f8633 WebCore::SVGTextMetricsBuilder::advanceSimpleText() + 99 STDERR: 19 DumpRenderTree 0x3d8f8352 WebCore::SVGTextMetricsBuilder::advance() + 146 STDERR: 20 DumpRenderTree 0x3d8f8c2a WebCore::SVGTextMetricsBuilder::measureTextRenderer(WebCore::RenderSVGInlineText*, WebCore::MeasureTextData*) + 330 STDERR: 21 DumpRenderTree 0x3d8f900d WebCore::SVGTextMetricsBuilder::walkTree(WebCore::RenderObject*, WebCore::RenderSVGInlineText*, WebCore::MeasureTextData*) + 221 STDERR: 22 DumpRenderTree 0x3d8f9280 WebCore::SVGTextMetricsBuilder::buildMetricsAndLayoutAttributes(WebCore::RenderSVGText*, WebCore::RenderSVGInlineText*, WTF::HashMap<unsigned int, WebCore::SVGCharacterData, WTF::IntHash<unsigned int>, WTF::HashTraits<unsigned int>, WTF::HashTraits<WebCore::SVGCharacterData> >&) + 208 STDERR: 23 DumpRenderTree 0x3d8ef253 WebCore::SVGTextLayoutAttributesBuilder::buildLayoutAttributesForForSubtree(WebCore::RenderSVGText*) + 323 STDERR: 24 DumpRenderTree 0x3d8b563b WebCore::RenderSVGText::layout() + 619 STDERR: 25 DumpRenderTree 0x3d8c8bd4 WebCore::SVGRenderSupport::layoutChildren(WebCore::RenderObject*, bool) + 548 STDERR: 26 DumpRenderTree 0x3d86c8e2 WebCore::RenderSVGContainer::layout() + 466 STDERR: 27 DumpRenderTree 0x3d8c8bd4 WebCore::SVGRenderSupport::layoutChildren(WebCore::RenderObject*, bool) + 548 STDERR: 28 DumpRenderTree 0x3d86c8e2 WebCore::RenderSVGContainer::layout() + 466 STDERR: 29 DumpRenderTree 0x3d8c8bd4 WebCore::SVGRenderSupport::layoutChildren(WebCore::RenderObject*, bool) + 548 STDERR: 30 DumpRenderTree 0x3d8a860c WebCore::RenderSVGRoot::layout() + 652 STDERR: 31 DumpRenderTree 0x3d07749d WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) + 1117 STDERR: 32 DumpRenderTree 0x3d06db88 WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::FractionalLayoutUnit&) + 1560 STDERR: 33 DumpRenderTree 0x3d06b147 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) + 1991 STDERR: 34 DumpRenderTree 0x3d06a165 WebCore::RenderBlock::layout() + 149 STDERR: 35 DumpRenderTree 0x3d315dbb WebCore::RenderView::layout() + 1211 STDERR: 36 DumpRenderTree 0x3ce919cd WebCore::FrameView::layout(bool) + 3677 STDERR: 37 DumpRenderTree 0x3a0ed9c6 WebCore::Document::updateLayout() + 326 STDERR: 38 DumpRenderTree 0x3a0edaea WebCore::Document::updateLayoutIgnorePendingStylesheets() + 250 STDERR: 39 DumpRenderTree 0x3ca67448 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 184 STDERR: 40 DumpRenderTree 0x3a0fa433 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 131 STDERR: 41 DumpRenderTree 0x3d6cb31e WebCore::DocumentV8Internal::execCommandCallback(v8::Arguments const&) + 750 STDERR: 42 DumpRenderTree 0x3ad99ff6 v8::internal::MaybeObject* v8::internal::HandleApiCallHelper<false>(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) + 1318 STDERR: 43 DumpRenderTree 0x3ad99a9a v8::internal::Builtin_Impl_HandleApiCall(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) + 74 STDERR: 44 DumpRenderTree 0x3ad8fdac v8::internal::Builtin_HandleApiCall(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) + 172 STDERR: 45 ??? 0x5ea0a336 0x0 + 1587585846 STDERR: ax: bbadbeef, bx: 135660, cx: 1e6da69, dx: 1e6da69 STDERR: di: 3e34ef50, si: 3e34ef22, bp: bfffa0b8, sp: bfffa050, ss: 23, flags: 10286 STDERR: ip: 3c1d6d18, cs: 1b, ds: 23, es: 23, fs: 0, gs: f
I can't reach you at irc, so I am going to roll this out for now.
Re-opened since this is blocked by 92169
Created attachment 158731 [details] Patch
> STDERR: ASSERTION FAILED: !StringWrapperCFAllocator::currentString It turns out CFStringCreateWithBytesNoCopy() is even more awesome than expected :) In some cases, the function reuses an existing string instead of allocating a new one. In this case, we never use the allocator and currentString is not cleared.
Comment on attachment 158731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158731&action=review r=me > Source/WebCore/platform/text/cf/StringImplCF.cpp:153 > + ASSERT(!StringWrapperCFAllocator::currentString); Seems like you should remove this ASSERT too. CFStringCreateWithCharactersNoCopy might implement the same optimization -- now, or in the future.
> > Source/WebCore/platform/text/cf/StringImplCF.cpp:153 > > + ASSERT(!StringWrapperCFAllocator::currentString); > > Seems like you should remove this ASSERT too. CFStringCreateWithCharactersNoCopy might implement the same optimization -- now, or in the future. Yep, I think that is a fair assumption. This assertion is too restrictive.
Committed r125809: <http://trac.webkit.org/changeset/125809>