Bug 90720 – [Mac] Do not perform 8 to 16bits characters conversion when converting a WTFString to NSString/CFString

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Benjamin Poulain]

Do not perform 8 to 16bits characters conversion when converting a WTFString to NSString/CFString.

<rdar://problem/11822873>

[Benjamin Poulain]

Created attachment 151135 [details]
Patch

[Sam Weinig]

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?

[Benjamin Poulain]

> > 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).

[Anders Carlsson]

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.

[Benjamin Poulain]

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.

[Benjamin Poulain]

Created attachment 153888 [details]
Patch

[Geoffrey Garen]

Comment on attachment 153888 [details]
Patch

r=me

[Darin Adler]

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.

[Benjamin Poulain]

> 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.

[Benjamin Poulain]

Committed r123504: <http://trac.webkit.org/changeset/123504>

[Zhenyao Mo]

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

[Zhenyao Mo]

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

[Zhenyao Mo]

I can't reach you at irc, so I am going to roll this out for now.

[WebKit Review Bot]

Re-opened since this is blocked by 92169

[Benjamin Poulain]

Created attachment 158731 [details]
Patch

[Benjamin Poulain]

> 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.

[Geoffrey Garen]

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.

[Benjamin Poulain]

> > 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.

[Benjamin Poulain]

Committed r125809: <http://trac.webkit.org/changeset/125809>