Bug 69942 – When an API test fails, TestWebKitAPI crashes

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Tools / Tests

[Simon Fraser (smfr)]

If I introduce a deliberate failure in an API test, like:

 TEST_F(MetaAllocatorTest, SimpleFullCoalesce32PlusPageLess32ThenPage)
 {
+    EXPECT_TRUE(false);
     testSimpleFullCoalesce(32, pageSize() - 32, pageSize());
 }

then TestWebKitAPI crashes, rather than just reporting the failure. The crash is:

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000000020d8

VM Regions Near 0x20d8:
--> 
    __TEXT                 00000001042de000-0000000104359000 [  492K] r-x/rwx SM=COW  /Volumes/VOLUME/*

Application Specific Information:
objc[91719]: garbage collection is OFF

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000104bc3a49 WTF::fastFree(void*) + 137 (TCPageMap.h:234)
1   TestWebKitAPI                 	0x0000000104320038 testing::Test::Run() + 92
2   TestWebKitAPI                 	0x00000001043206be testing::internal::TestInfoImpl::Run() + 158
3   TestWebKitAPI                 	0x0000000104320ae5 testing::TestCase::Run() + 161
4   TestWebKitAPI                 	0x0000000104323e7a testing::internal::UnitTestImpl::RunAllTests() + 594
5   TestWebKitAPI                 	0x00000001042dfdde TestWebKitAPI::TestsController::run(int, char**) + 36 (TestsController.cpp:51)
6   TestWebKitAPI                 	0x00000001042dfcf1 main + 96 (main.mm:35)
7   TestWebKitAPI                 	0x00000001042df934 start + 52

This looks like malloc/FastMalloc mixing.

[Simon Fraser (smfr)]

Only seems to be an issue in Release builds.

[Simon Fraser (smfr)]

This bug is causing every API test failure to look like a timeout, and therefore be much harder to spot in the output on the bots.

[Adam Roben (:aroben)]

I think this is an interaction between gtest and FastMalloc. One solution is to set USE_SYSTEM_MALLOC=1 for Release builds.

[Filip Pizlo]

I think that in my tests there are inline destructors that get compiled/inlined when building TestWebKitAPI, with the corresponding constructors being compiled/linked when building JavaScriptCore.  Hence setting USE_SYSTEM_MALLOC=1 will only worsen the situation. :-(

[David Kilzer (:ddkilzer)]

<rdar://problem/10607911>

[David Kilzer (:ddkilzer)]

This is trivial to reproduce by changing a StringBuilder test to fail:

--- a/Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp
+++ b/Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp
@@ -128,7 +128,7 @@ TEST(StringBuilderTest, ToStringPreserveCapacity)
     StringBuilder builder;
     builder.append("0123456789");
     String string = builder.toStringPreserveCapacity();
-    ASSERT_EQ(String("0123456789"), string);
+    ASSERT_EQ(String("01234567890"), string);
     ASSERT_EQ(string.impl(), builder.toStringPreserveCapacity().impl());
     ASSERT_EQ(string.characters(), builder.characters());

[David Kilzer (:ddkilzer)]

When compiling TestWebKitAPI with "-g -O0" (pardon the hack):

--- a/Tools/TestWebKitAPI/Configurations/Base.xcconfig
+++ b/Tools/TestWebKitAPI/Configurations/Base.xcconfig
@@ -38,1 +38,1 @@ GCC_TREAT_WARNINGS_AS_ERRORS = YES
-WARNING_CFLAGS = -Wall -W -Wno-unused-parameter
+WARNING_CFLAGS = -Wall -W -Wno-unused-parameter -g -O0

I see this crash when trying to run ./Tools/Scripts/run-api-tests while it's generating a list of tests:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000104c99f69 WTF::fastFree(void*) + 137 (TCPageMap.h:234)
1   TestWebKitAPI                 	0x0000000104290705 testing::internal::String::~String() + 21 (gtest-string.h:218)
2   libsystem_c.dylib             	0x00007fff8fe387c8 __cxa_finalize + 274
3   libsystem_c.dylib             	0x00007fff8fe38652 exit + 18
4   TestWebKitAPI                 	0x000000010428e77b start + 59

This indicates (to me) that gtest is trying to use FastMalloc for its destructors.  If gtest is using malloc to allocate this memory, it's not surprising that FastMalloc is crashing when it tries to release memory it's not managing.

Since JavaScriptCore, et al, are compiled with FastMalloc on release builds, I don't think that it would work to enable USE_SYSTEM_MALLOC to compile TestWebKitAPI (as Filip notes in Comment #4).

We may need to compile gtest using FastMalloc to make TestWebKitAPI not crash.

[David Levin]

Hmm... I know that Mitya (dslomov) did these changes to address this issue:
 http://trac.webkit.org/changeset/93426
 http://trac.webkit.org/changeset/92387

But I guess there are still issues.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> Hmm... I know that Mitya (dslomov) did these changes to address this issue:
>  http://trac.webkit.org/changeset/93426

Bug 66521

>  http://trac.webkit.org/changeset/92387

Bug 61812

> But I guess there are still issues.

r93426 was rolled out in r93451.  See Bug 66607.

[Dmitry Lomov]

(In reply to comment #9)
> (In reply to comment #8)
> > Hmm... I know that Mitya (dslomov) did these changes to address this issue:
> >  http://trac.webkit.org/changeset/93426
> 
> Bug 66521
> 
> >  http://trac.webkit.org/changeset/92387
> 
> Bug 61812
> 
> > But I guess there are still issues.
> 
> r93426 was rolled out in r93451.  See Bug 66607.

I guess I should work on reintroducing the changes I did related to bug 66521

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> I guess I should work on reintroducing the changes I did related to bug 66521

I have a potential fix for the Mac.  Will post a patch shortly.

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> (In reply to comment #10)
> > I guess I should work on reintroducing the changes I did related to bug 66521
> 
> I have a potential fix for the Mac.  Will post a patch shortly.

Well, I thought I had a fix.

What I thought I was seeing in release builds is that FastMalloc.h was redefining the 'new' and 'delete' operators to use fastMalloc() and fastFree() when ENABLE(GLOBAL_FASTMALLOC_NEW) was true.

This caused any use of 'new' and 'delete' in gtest headers to start using fastMalloc() and fastFree(), which contracted the use of malloc() and free() in gtest source code, so you ended up with crashes like the one in this bug.

My initial solution was to do this in TestWebKitAPI/config.h (thereby turning off the redefining of 'new' and 'delete'):

+#define ENABLE_GLOBAL_FASTMALLOC_NEW 0
 #include <wtf/Platform.h>

This change fixed crashes in Release builds when a WTF::String() comparison fails (and when compiling with "-g -O0"), but I'm still seeing crashes on Debug builds.  Investigating.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> This change fixed crashes in Release builds when a WTF::String() comparison fails (and when compiling with "-g -O0"), but I'm still seeing crashes on Debug builds.  Investigating.

LOL!  Looks like threading isn't being initialized properly in TestWebKitAPI for a new test I wrote (an assertion is firing):

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010792e203 WTF::isMainThread() + 195 (MainThreadMac.mm:144)
1   com.apple.WebCore             	0x00000001098a3efd _ZN7WebCoreL22buildBaseTextCodecMapsEv + 13 (TextEncodingRegistry.cpp:213)
2   com.apple.WebCore             	0x00000001098a3e2a WebCore::atomicCanonicalTextEncodingName(char const*) + 74 (TextEncodingRegistry.cpp:338)
3   com.apple.WebCore             	0x00000001098a2c21 WebCore::TextEncoding::TextEncoding(char const*) + 33 (TextEncoding.cpp:55)
4   com.apple.WebCore             	0x00000001098a2bed WebCore::TextEncoding::TextEncoding(char const*) + 29 (TextEncoding.cpp:58)
5   com.apple.WebCore             	0x00000001098a3598 WebCore::UTF8Encoding() + 40 (TextEncoding.cpp:250)
6   com.apple.WebCore             	0x000000010928e7b4 WebCore::decodeURLEscapeSequences(WTF::String const&) + 36 (KURL.cpp:919)
7   com.apple.WebCore             	0x000000010928e77a WebCore::KURL::host() const + 90 (KURL.cpp:586)
8   TestWebKitAPI                 	0x0000000106073fbf TestWebKitAPI::WebCore_KURLConstCharConstructor_Test::TestBody() + 2015 (KURL.cpp:54)
9   TestWebKitAPI                 	0x000000010607e7ca testing::Test::Run() + 154
10  TestWebKitAPI                 	0x000000010607f0cd testing::internal::TestInfoImpl::Run() + 189
11  TestWebKitAPI                 	0x000000010607f8fd testing::TestCase::Run() + 205
12  TestWebKitAPI                 	0x00000001060849e9 testing::internal::UnitTestImpl::RunAllTests() + 793
13  TestWebKitAPI                 	0x00000001060846c9 testing::UnitTest::Run() + 25
14  TestWebKitAPI                 	0x0000000105fe9870 TestWebKitAPI::TestsController::run(int, char**) + 48 (TestsController.cpp:51)
15  TestWebKitAPI                 	0x0000000105fe96e9 main + 121 (main.mm:35)
16  TestWebKitAPI                 	0x0000000105fe8ca4 start + 52

The assertion is here:

bool isMainThread()
{
    if (mainThreadEstablishedAsPthreadMain) {
        ASSERT(!mainThreadPthread);
        return pthread_main_np();
    }

    ASSERT(mainThreadPthread); // Line 144
    return pthread_equal(pthread_self(), mainThreadPthread);
}

That's a separate bug.  I'll post the patch for this bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > I guess I should work on reintroducing the changes I did related to bug 66521
> > 
> > I have a potential fix for the Mac.  Will post a patch shortly.
> 
> Well, I thought I had a fix.
> 
> What I thought I was seeing in release builds is that FastMalloc.h was redefining the 'new' and 'delete' operators to use fastMalloc() and fastFree() when ENABLE(GLOBAL_FASTMALLOC_NEW) was true.
> 
> This caused any use of 'new' and 'delete' in gtest headers to start using fastMalloc() and fastFree(), which contracted the use of malloc() and free() in gtest source code, so you ended up with crashes like the one in this bug.
> 
> My initial solution was to do this in TestWebKitAPI/config.h (thereby turning off the redefining of 'new' and 'delete'):
> 
> +#define ENABLE_GLOBAL_FASTMALLOC_NEW 0
>  #include <wtf/Platform.h>
> 
> This change fixed crashes in Release builds when a WTF::String() comparison fails (and when compiling with "-g -O0"), but I'm still seeing crashes on Debug builds.  Investigating.

Of course, the problem with this approach is that 'new' and 'delete' operators in JavaScriptCore headers now will NOT be redefined as fastMalloc() and fastFree(), so there will be mismatches when creating/deleting JSC objects.

[Adam Roben (:aroben)]

(In reply to comment #14)
> Of course, the problem with this approach is that 'new' and 'delete' operators in JavaScriptCore headers now will NOT be redefined as fastMalloc() and fastFree(), so there will be mismatches when creating/deleting JSC objects.

I don't think TestWebKitAPI ever uses new to create JSC objects.

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> Of course, the problem with this approach is that 'new' and 'delete' operators in JavaScriptCore headers now will NOT be redefined as fastMalloc() and fastFree(), so there will be mismatches when creating/deleting JSC objects.

I beginning to see Dmitry's point in Bug 66521 about making gtest use FastMalloc.h as the only sane solution here.

[David Kilzer (:ddkilzer)]

(In reply to comment #15)
> (In reply to comment #14)
> > Of course, the problem with this approach is that 'new' and 'delete' operators in JavaScriptCore headers now will NOT be redefined as fastMalloc() and fastFree(), so there will be mismatches when creating/deleting JSC objects.
> 
> I don't think TestWebKitAPI ever uses new to create JSC objects.

Well, more generally speaking, any JavaScriptCore, WebCore or WebKit header that uses operator 'new' or operator 'delete' is at risk of allocating memory using the wrong allocator if we disable these redefinitions in config.h.  (This assumes we want to be able to unit-test any class.)

[Dmitry Lomov]

(In reply to comment #16)
> (In reply to comment #14)
> > Of course, the problem with this approach is that 'new' and 'delete' operators in JavaScriptCore headers now will NOT be redefined as fastMalloc() and fastFree(), so there will be mismatches when creating/deleting JSC objects.
> 
> I beginning to see Dmitry's point in Bug 66521 about making gtest use FastMalloc.h as the only sane solution here.

Good :) Is that what are you shooting for now? Do you need help?

I do not think USE_SYSTEM_MALLOC is going to cut it. Basically, since these are webkit/webcore unit tests, we do want to be able to test in the exact regimes ports compile. This includes running unit tests with global fast malloc/free. The testing framework should not require any specific set of ENABLE_/DISABLE_ defines (individual tests might though).

[David Kilzer (:ddkilzer)]

(In reply to comment #18)
> (In reply to comment #16)
> > I['m] beginning to see Dmitry's point in Bug 66521 about making gtest use FastMalloc.h as the only sane solution here.
> 
> Good :) Is that what are you shooting for now? Do you need help?

I have your patch from Bug 66521 working on the Apple Mac port.  I need help testing the Windows vcproj changes (which I merged by hand).

> I do not think USE_SYSTEM_MALLOC is going to cut it. Basically, since these are webkit/webcore unit tests, we do want to be able to test in the exact regimes ports compile. This includes running unit tests with global fast malloc/free. The testing framework should not require any specific set of ENABLE_/DISABLE_ defines (individual tests might though).

I agree.  I've finally reached the same conclusion.  :)

Duping this to Bug 66521.  Fixing that also fixes this crash.

*** This bug has been marked as a duplicate of bug 66521 ***

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> (In reply to comment #12)
> > This change fixed crashes in Release builds when a WTF::String() comparison fails (and when compiling with "-g -O0"), but I'm still seeing crashes on Debug builds.  Investigating.
> 
> LOL!  Looks like threading isn't being initialized properly in TestWebKitAPI for a new test I wrote (an assertion is firing):
> [...]
> That's a separate bug.  I'll post the patch for this bug.

I filed Bug 75064 for this issue.