StringImpl should not be referred from StringImplCF.

StringImpl should not be referred from StringImplCF.

StringImplCF has different lifetime from StringImpl.
If Blink instance has been already destroyed, objective-c clean-up code will crash.
We should locally prepare NSString by using AtomicString and not use StringImpl::createCFString.

BUG=305885
TEST=all tests should pass.

[tasak, 2014-01-15 08:31:38]

I need this patch to land the following:
https://codereview.chromium.org/82583005/

The reason why r164691 crashes browser_tests is that objective-c has some
reference from StringImplCF to StringImpl and tries to release StringImplCF
instances after Blink instance is destroyed.

[dglazkov, 2014-01-15 18:47:07]

I am surprised that this is just now exposed by your change, but I have little
visibility into the Mac parts. Maybe abarth knows more?

https://codereview.chromium.org/139243002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderThemeChromiumMac.mm (right):

https://codereview.chromium.org/139243002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderThemeChromiumMac.mm:306: NSString* nsFamilyName =
[font webCoreFamilyName];
Should this be part of AtomicString constructor?

[eseidel, 2014-01-15 19:02:55]

I'm confused.  Why did these work for WebKit but not for us?

[Nico, 2014-01-15 19:06:12]

I too don't understand this change. It looks wrong to me.

https://codereview.chromium.org/139243002/diff/1/Source/platform/fonts/mac/Fo...
File Source/platform/fonts/mac/FontCacheMac.mm (right):

https://codereview.chromium.org/139243002/diff/1/Source/platform/fonts/mac/Fo...
Source/platform/fonts/mac/FontCacheMac.mm:200: NSString *nsFontFamily =
[[NSString alloc] initWithUTF8String:family.utf8().data()];
This isn't correct, this is never freed anywhere.

https://codereview.chromium.org/139243002/diff/1/Source/platform/text/LocaleM...
File Source/platform/text/LocaleMac.mm (right):

https://codereview.chromium.org/139243002/diff/1/Source/platform/text/LocaleM...
Source/platform/text/LocaleMac.mm:92: NSString *nsLanguage = [[NSString alloc]
initWithUTF8String:defaultLanguage().utf8().data()];
This too leaks

https://codereview.chromium.org/139243002/diff/1/Source/wtf/text/AtomicString.h
File Source/wtf/text/AtomicString.h (left):

https://codereview.chromium.org/139243002/diff/1/Source/wtf/text/AtomicString...
Source/wtf/text/AtomicString.h:157: #endif
The implementation in AtomicString.cpp is still around though?

[tasak, 2014-01-16 01:01:30]

Thank you for reviewing.

I would like to explain what I want to do.

On 2014/01/15 19:06:12, Nico wrote:
> I too don't understand this change. It looks wrong to me.

I ran browse_tests after applying r164691 in the following way:

gdb --args out/Debug/browser_tests --single-process
--gtest_filter=PrintWebViewHelperTest.PrintLayoutTest

Note: Google Test filter = PrintWebViewHelperTest.PrintLayoutTest
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from PrintWebViewHelperTestr
[ RUN      ] PrintWebViewHelperTest.PrintLayoutTest

Breakpoint 1, WTF::StringWrapperCFAllocator::allocate (size=32) at
../../third_party/WebKit/Source/wtf/text/StringImplCF.cpp:56
56	    StringImpl* underlyingString = 0;
(gdb) where
#0  WTF::StringWrapperCFAllocator::allocate (size=32) at
../../third_party/WebKit/Source/wtf/text/StringImplCF.cpp:56
#1  0x966197a6 in CFAllocatorAllocate ()
#2  0x966195a1 in _CFRuntimeCreateInstance ()
#3  0x9661bb79 in __CFStringCreateImmutableFunnel3 ()
#4  0x966e73cd in CFStringCreateWithBytesNoCopy ()
#5  0x064c4e1b in WTF::StringImpl::createCFString (this=0x27ca5e24) at
../../third_party/WebKit/Source/wtf/text/StringImplCF.cpp:144
#6  0x064c5563 in WTF::StringImpl::operator NSString* (this=0x27ca5e24) at
../../third_party/WebKit/Source/wtf/text/StringImplMac.mm:45
#7  0x0850564f in WTF::String::operator NSString* (this=0x2077c3ac) at
WTFString.h:402
#8  0x06b3b73b in WTF::AtomicString::operator NSString* (this=0x2077c3ac) at
AtomicString.h:156
#9  0x068a2fb9 in WebCore::FontCache::createFontPlatformData (this=0x218763f0,
fontDescription=@0x2077c3ac, family=@0x2077c3ac, fontSize=16) at
../../third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:200
#10 0x0682c9a4 in WebCore::FontCache::getFontPlatformData (this=0x218763f0,
fontDescription=@0x2077c3ac, passedFamilyName=@0x2077c3ac,
checkingAlternateName=false) at
../../third_party/WebKit/Source/platform/fonts/FontCache.cpp:96
#11 0x0682cc01 in WebCore::FontCache::getFontData (this=0x218763f0,
fontDescription=@0x2077c3ac, family=@0x2077c3ac, checkingAlternateName=false,
shouldRetain=WebCore::Retain) at
../../third_party/WebKit/Source/platform/fonts/FontCache.cpp:145
#12 0x06844668 in WebCore::FontFallbackList::getFontData (this=0x2077c3f0,
fontDescription=@0x2077c3ac, familyIndex=@0x2077c464) at
../../third_party/WebKit/Source/platform/fonts/FontFallbackList.cpp:149
#13 0x06844369 in WebCore::FontFallbackList::fontDataAt (this=0x2077c3f0,
fontDescription=@0x2077c3ac, realizedFontIndex=0) at
../../third_party/WebKit/Source/platform/fonts/FontFallbackList.cpp:190
#14 0x06843eee in WebCore::FontFallbackList::primaryFontData (this=0x2077c3f0,
fontDescription=@0x2077c3ac) at
../../third_party/WebKit/Source/platform/fonts/FontFallbackList.cpp:112
#15 0x0689e862 in WebCore::FontFallbackList::primarySimpleFontData
(this=0x2077c3f0, fontDescription=@0x2077c3ac) at FontFallbackList.h:96
#16 0x068921f9 in WebCore::Font::primaryFont (this=0x2077c3ac) at Font.h:299
#17 0x085a8f7b in WebCore::Font::fontMetrics (this=0x2077c3ac) at Font.h:131
#18 0x0859ebbe in WebCore::RenderStyle::fontMetrics (this=0x2077c340) at
../../third_party/WebKit/Source/core/rendering/style/RenderStyle.cpp:1177

(gdb) p underlyingString
$3 = ('WTF::StringImpl' *) 0x27ca5e24

[       OK ] PrintWebViewHelperTest.PrintLayoutTest (122286 ms)
[----------] 1 test from PrintWebViewHelperTest (122312 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (122312 ms total)
[  PASSED  ] 1 test.

  YOU HAVE 336 DISABLED TESTS


Breakpoint 3, WTF::StringWrapperCFAllocator::deallocate (pointer=0x2077c624) at
../../third_party/WebKit/Source/wtf/text/StringImplCF.cpp:91
91	    if (!underlyingString) {
(gdb) where
#0  WTF::StringWrapperCFAllocator::deallocate (pointer=0x2077c624) at
../../third_party/WebKit/Source/wtf/text/StringImplCF.cpp:91
#1  0x9661a664 in CFAllocatorDeallocate ()
#2  0x9662f6b8 in CFRelease ()
#3  0x975ba497 in (anonymous namespace)::AutoreleasePoolPage::pop ()
#4  0x96650cdf in _CFAutoreleasePoolPop ()
#5  0x9898c934 in -[NSAutoreleasePool drain] ()
#6  0x02c49f2b in base::mac::ScopedNSAutoreleasePool::Recycle (this=0xbffff470)
at ../../base/mac/scoped_nsautorelease_pool.mm:26
#7  0x02db948e in base::TestSuite::Run (this=0xbffff4b0) at
../../base/test/test_suite.cc:219
#8  0x02b37613 in (anonymous
namespace)::ChromeTestLauncherDelegate::RunTestSuite (this=0xbffff9c8, argc=3,
argv=0xbffffa4c) at ../../chrome/test/base/chrome_test_launcher.cc:65
#9  0x0ce8de58 in content::LaunchTests (launcher_delegate=0xbffff9c8,
default_jobs=12, argc=3, argv=0xbffffa4c) at
../../content/public/test/test_launcher.cc:487
#10 0x02b3741e in LaunchChromeTests (default_jobs=12, argc=3, argv=0xbffffa4c)
at ../../chrome/test/base/chrome_test_launcher.cc:147
#11 0x00f6b06b in main (argc=3, argv=0xbffffa4c) at
../../chrome/test/base/browser_tests_main.cc:12

(gdb) p underlyingString
$4 = ('WTF::StringImpl' *) 0x27ca5e24

(gdb) p *underlyingString
Cannot access memory at address 0x27ca5e24

I also checked ('WTF::StringImpl' *) 0x27ca5e24's reference count.
WTF::StringImpl::~StringImpl for 0x27ca5e24 was never invoked.

So I think, after running one test, browser_tests destroys Blink instance.

However, "ScopedNSAutoreleasePool" was not invoked before destroying Blink not
at the same time when destroying Blink.

Now WTF::StringImpl::createCFString has a StringImpl instance allocated  by
Blink instance (e.g. 0x27ca5e24) and WTF::StringWrapperCFAllocator::deallocate
tries to deref the StringImpl.

So,

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x27ca5e24

[tasak, 2014-01-17 05:30:01]

If possible, I would like to remove StringImplCF.cpp.
At least, I would like to avoid invoking StringImpl::createCFString.

On 2014/01/16 01:01:30, tasak wrote:

>#4  0x966e73cd in CFStringCreateWithBytesNoCopy ()
>#5  0x064c4e1b in WTF::StringImpl::createCFString (this=0x27ca5e24) at
../../third_party/WebKit/Source/wtf/text/StringImplCF.cpp:144
>#6  0x064c5563 in WTF::StringImpl::operator NSString* (this=0x27ca5e24) at
../../third_party/WebKit/Source/wtf/text/StringImplMac.mm:45
>#7  0x0850564f in WTF::String::operator NSString* (this=0x2077c3ac) at
WTFString.h:402
>#8  0x06b3b73b in WTF::AtomicString::operator NSString* (this=0x2077c3ac) at
AtomicString.h:156

I would like to remove the above code path, because StringImpl::createCFString
does:

   // Put pointer to the StringImpl in a global so the allocator can store it
with the CFString.
   StringWrapperCFAllocator::currentString = this; 

The currentString is a global variable, i.e.

   namespace WTF {
   namespace StringWrapperCFAllocator {
   static StringImpl* currentString;

and is used in "static void* allocate(CFIndex size, CFOptionFlags, void*)" in
StringImplCF.cpp, i.e.

       underlyingString = currentString;
...
       StringImpl** header =
static_cast<StringImpl**>(fastMalloc(sizeof(StringImpl*) + size));
      *header = underlyingString;
      return header + 1;

The allocate is invoked from Mac allocator (I think). So deallocator is also
invoked from Mac deallocator, i.e.

>  #0  WTF::StringWrapperCFAllocator::deallocate (pointer=0x2077c624) at
../../third_party/WebKit/Source/wtf/text/StringImplCF.cpp:91
> #1  0x9661a664 in CFAllocatorDeallocate ()
> #2  0x9662f6b8 in CFRelease ()

However, at this time, Blink memory is still available?

browser_tests probably has already destroyed blink.

e.g.
> (gdb) p underlyingString
> $4 = ('WTF::StringImpl' *) 0x27ca5e24
> 
> (gdb) p *underlyingString
> Cannot access memory at address 0x27ca5e24

[tasak, 2014-01-17 08:17:29]

https://codereview.chromium.org/139243002/diff/1/Source/platform/fonts/mac/Fo...
File Source/platform/fonts/mac/FontCacheMac.mm (right):

https://codereview.chromium.org/139243002/diff/1/Source/platform/fonts/mac/Fo...
Source/platform/fonts/mac/FontCacheMac.mm:200: NSString *nsFontFamily =
[[NSString alloc] initWithUTF8String:family.utf8().data()];
On 2014/01/15 19:06:12, Nico wrote:
> This isn't correct, this is never freed anywhere.

Done.

https://codereview.chromium.org/139243002/diff/1/Source/platform/text/LocaleM...
File Source/platform/text/LocaleMac.mm (right):

https://codereview.chromium.org/139243002/diff/1/Source/platform/text/LocaleM...
Source/platform/text/LocaleMac.mm:92: NSString *nsLanguage = [[NSString alloc]
initWithUTF8String:defaultLanguage().utf8().data()];
On 2014/01/15 19:06:12, Nico wrote:
> This too leaks

Done.

https://codereview.chromium.org/139243002/diff/1/Source/wtf/text/AtomicString.h
File Source/wtf/text/AtomicString.h (left):

https://codereview.chromium.org/139243002/diff/1/Source/wtf/text/AtomicString...
Source/wtf/text/AtomicString.h:157: #endif
On 2014/01/15 19:06:12, Nico wrote:
> The implementation in AtomicString.cpp is still around though?

I see. I removed AtomicStringCF.cpp (i.e. AtomicString::add(CFStringRef)).

[tasak, 2014-01-23 08:55:50]

PTAL?

[Nico, 2014-01-24 01:01:06]

https://codereview.chromium.org/139243002/diff/160001/Source/wtf/text/AtomicS...
File Source/wtf/text/AtomicStringCF.cpp (left):

https://codereview.chromium.org/139243002/diff/160001/Source/wtf/text/AtomicS...
Source/wtf/text/AtomicStringCF.cpp:51: return add(reinterpret_cast<const
UChar*>(ucharBuffer.data()), length);
Does removing this method have any effect on what you're doing? No, right? This
just calls the regular add() methods, so the CFStringRef data gets always copied
by this method, correct?

[Nico, 2014-01-24 01:09:11]

If I understand your patch right, you're saying that the "operator NSString" in
AtomicString doesn't work right (and the deletion of add(CFStringImpl) and most
of the changes in this patch don't actually have an effect and are harmless /
functional no-ops).

The operator NSString ends up calling Source/wtf/text/StringImplCF.cpp , and
you're saying that this method is broken. If that's what you think: Why do you
only thing this for AtomicStrings?

The allocate() call there tries to keep a ref to the underlyingString object, so
even if the nsstring outlives blink objects (to which the right fix to me sounds
like adding a ScopedNSAutoreleasePool somewhere), it should keep the
underlyingString object around until it's no longer needed (?)

[tasak, 2014-01-24 07:00:10]

Thank you for reviewing.

On 2014/01/24 01:09:11, Nico wrote:
> If I understand your patch right, you're saying that the "operator NSString"
in
> AtomicString doesn't work right (and the deletion of add(CFStringImpl) and
most
> of the changes in this patch don't actually have an effect and are harmless /
> functional no-ops).

Yeah, the "operator NSString*" invoked from objective-c code causes a complex
side effect: memory managed by CFAllocator/CFDeallocator has some reference to
memory managed by FastMalloc/FastFree.

The patch just removes the reference: CFAllocator/CFDeallocator doesn't need to
do anything for FastMalloc/FastFree world.

> The operator NSString ends up calling Source/wtf/text/StringImplCF.cpp , and
> you're saying that this method is broken. If that's what you think: Why do you
> only thing this for AtomicStrings?

This is a minimal patch to land my reverted r164691 (for fixing release-blocking
bug).

If possible, I would like to remove left things, i.e. StringCF.cpp and
StringImplCF.cpp, in another patch.

> The allocate() call there tries to keep a ref to the underlyingString object,
so
> even if the nsstring outlives blink objects (to which the right fix to me
sounds
> like adding a ScopedNSAutoreleasePool somewhere), it should keep the
> underlyingString object around until it's no longer needed (?)

So we should fix browser_tests? e.g.

 TEST_F(PrintWebViewHelperTest, PrintLayoutTest) {
+#if defined(OS_MACOSX)
+  base::mac::ScopedNSAutoreleasePool scoped_pool;
+#endif
   bool baseline = false;
 
   EXPECT_TRUE(chrome_render_thread_->printer() != NULL);
@@ -430,6 +442,9 @@ TEST_F(PrintWebViewHelperTest, PrintLayoutTest) {
       chrome_render_thread_->printer()->SaveBitmap(0, bitmap_path);
     }
   }
+#if defined(OS_MACOSX)
+  scoped_pool.Recycle();
+#endif
 }

It's ok for me.

However, I'm afraid that we must be always careful about ScopedNSAutoreleasePool
when modifying Blink or browser_tests.

I mean, this issue is performance improvement (using some hacky code to avoid
converting StringImpl (CFStringRef) into CFStringRef (StringImpl)) vs
programmability.

https://codereview.chromium.org/139243002/diff/160001/Source/wtf/text/AtomicS...
File Source/wtf/text/AtomicStringCF.cpp (left):

https://codereview.chromium.org/139243002/diff/160001/Source/wtf/text/AtomicS...
Source/wtf/text/AtomicStringCF.cpp:51: return add(reinterpret_cast<const
UChar*>(ucharBuffer.data()), length);
On 2014/01/24 01:01:06, Nico wrote:
> Does removing this method have any effect on what you're doing? No, right?
This
> just calls the regular add() methods, so the CFStringRef data gets always
copied
> by this method, correct?

I think, this method is only used from AtomicString constructor.

Since I removed AtomicString(CFStringRef) and AtomicString(NSString*), now we
don't need this method.

I'm now trying to run "ninja -C out/Release all". (When I ran "ninja -C
out/Debug blink_tests content_shell, I saw no compile / no link error.)

I think, the build test will pass.

[Nico, 2014-01-24 23:27:28]

On 2014/01/24 07:00:10, tasak wrote:
> Thank you for reviewing.
> 
> On 2014/01/24 01:09:11, Nico wrote:
> > If I understand your patch right, you're saying that the "operator NSString"
> in
> > AtomicString doesn't work right (and the deletion of add(CFStringImpl) and
> most
> > of the changes in this patch don't actually have an effect and are harmless
/
> > functional no-ops).
> 
> Yeah, the "operator NSString*" invoked from objective-c code causes a complex
> side effect: memory managed by CFAllocator/CFDeallocator has some reference to
> memory managed by FastMalloc/FastFree.
> 
> The patch just removes the reference: CFAllocator/CFDeallocator doesn't need
to
> do anything for FastMalloc/FastFree world.
> 
> > The operator NSString ends up calling Source/wtf/text/StringImplCF.cpp , and
> > you're saying that this method is broken. If that's what you think: Why do
you
> > only thing this for AtomicStrings?
> 
> This is a minimal patch to land my reverted r164691 (for fixing
release-blocking
> bug).
> 
> If possible, I would like to remove left things, i.e. StringCF.cpp and
> StringImplCF.cpp, in another patch.
> 
> > The allocate() call there tries to keep a ref to the underlyingString
object,
> so
> > even if the nsstring outlives blink objects (to which the right fix to me
> sounds
> > like adding a ScopedNSAutoreleasePool somewhere), it should keep the
> > underlyingString object around until it's no longer needed (?)
> 
> So we should fix browser_tests? e.g.
> 
>  TEST_F(PrintWebViewHelperTest, PrintLayoutTest) {
> +#if defined(OS_MACOSX)
> +  base::mac::ScopedNSAutoreleasePool scoped_pool;
> +#endif
>    bool baseline = false;
>  
>    EXPECT_TRUE(chrome_render_thread_->printer() != NULL);
> @@ -430,6 +442,9 @@ TEST_F(PrintWebViewHelperTest, PrintLayoutTest) {
>        chrome_render_thread_->printer()->SaveBitmap(0, bitmap_path);
>      }
>    }
> +#if defined(OS_MACOSX)
> +  scoped_pool.Recycle();
> +#endif
>  }
> 
> It's ok for me.

If browser_tests currently doesn't have an autorelease pool around every test, I
think we should add one. (Not in each test of course, but in the test runner.)

If the only place that uses StringImplCF.cpp is the NSString* operator in
AtomicString, and that's only called in a handful places, then removing that
sounds fine to me too from a code complexity point of view, but it doesn't feel
like the right fix for your bug. (Which seems more autorelease-pool-lifetime
related.)

In any case, removing the add() overload doesn't seem necessary.

> 
> However, I'm afraid that we must be always careful about
ScopedNSAutoreleasePool
> when modifying Blink or browser_tests.
> 
> I mean, this issue is performance improvement (using some hacky code to avoid
> converting StringImpl (CFStringRef) into CFStringRef (StringImpl)) vs
> programmability.
> 
>
https://codereview.chromium.org/139243002/diff/160001/Source/wtf/text/AtomicS...
> File Source/wtf/text/AtomicStringCF.cpp (left):
> 
>
https://codereview.chromium.org/139243002/diff/160001/Source/wtf/text/AtomicS...
> Source/wtf/text/AtomicStringCF.cpp:51: return add(reinterpret_cast<const
> UChar*>(ucharBuffer.data()), length);
> On 2014/01/24 01:01:06, Nico wrote:
> > Does removing this method have any effect on what you're doing? No, right?
> This
> > just calls the regular add() methods, so the CFStringRef data gets always
> copied
> > by this method, correct?
> 
> I think, this method is only used from AtomicString constructor.
> 
> Since I removed AtomicString(CFStringRef) and AtomicString(NSString*), now we
> don't need this method.
> 
> I'm now trying to run "ninja -C out/Release all". (When I ran "ninja -C
> out/Debug blink_tests content_shell, I saw no compile / no link error.)
> 
> I think, the build test will pass.

[tasak, 2014-01-29 04:09:04]

Thank you for comments.

On 2014/01/24 23:27:28, Nico wrote:
> 
> If browser_tests currently doesn't have an autorelease pool around every test,
I
> think we should add one. (Not in each test of course, but in the test runner.)
> 
> If the only place that uses StringImplCF.cpp is the NSString* operator in
> AtomicString, and that's only called in a handful places, then removing that
> sounds fine to me too from a code complexity point of view, but it doesn't
feel
> like the right fix for your bug. (Which seems more autorelease-pool-lifetime
> related.)

I see.
I agree that this patch is too much for landing the patch in
https://codereview.chromium.org/82583005/.

If I want to fix StringImplCF-related things, it is better to file a new bug.

I will close this cl and upload a patch to add autorelease pool to
PringLayoutTest.