Bug 69403 – [WK2] [GTK] WebKitTestRunner crashes with heap corruption

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alejandro G. Castro]

WebKitTestRunner is crashing randomly due to heap memory corruption. The stacks of the crash does not give much information about the issue. The valgrind log shows we have problems with Mutex size in some situations:


==962== Invalid write of size 8
==962==    at 0x37FC808963: pthread_mutex_init (pthread_mutex_init.c:83)
==962==    by 0x8EC5C53: WTF::Mutex::Mutex() (ThreadingPthreads.cpp:228)
==962==    by 0xD8BC1B0: WTF::HashTable<unsigned int, std::pair<unsigned int, WebKit::WKRetainPtr<OpaqueWKBundleScriptWorld const*> >, WTF::PairFirstExtractor<std::pair<unsigned int, WebKit::WKRetainPtr<OpaqueWKBundleScriptWorld const*> > >, WTF::IntHash<unsigned int>, $
==962==    by 0xD8BC061: WTF::HashMap<unsigned int, WebKit::WKRetainPtr<OpaqueWKBundleScriptWorld const*>, WTF::IntHash<unsigned int>, WTF::HashTraits<unsigned int>, WTF::HashTraits<WebKit::WKRetainPtr<OpaqueWKBundleScriptWorld const*> > >::HashMap() (HashMap.h:32)
==962==    by 0xD8BBB97: WTR::worldMap() (LayoutTestController.cpp:461)
==962==    by 0xD8BBBD5: WTR::LayoutTestController::worldIDForWorld(OpaqueWKBundleScriptWorld const*) (LayoutTestController.cpp:467)
==962==    by 0xD8B4E6F: WTR::InjectedBundlePage::didClearWindowForFrame(OpaqueWKBundleFrame const*, OpaqueWKBundleScriptWorld const*) (InjectedBundlePage.cpp:639)
==962==    by 0xD8B3F16: WTR::InjectedBundlePage::didClearWindowForFrame(OpaqueWKBundlePage const*, OpaqueWKBundleFrame const*, OpaqueWKBundleScriptWorld const*, void const*) (InjectedBundlePage.cpp:358)
...
==962==  Address 0xdc76b48 is 0 bytes after a block of size 40 alloc'd
==962==    at 0x4A05E46: malloc (vg_replace_malloc.c:195)
==962==    by 0x8EA9E22: WTF::fastMalloc(unsigned long) (FastMalloc.cpp:264)
==962==    by 0xD8BC135: WTF::HashMap<unsigned int, WebKit::WKRetainPtr<OpaqueWKBundleScriptWorld const*>, WTF::IntHash<unsigned int>, WTF::HashTraits<unsigned int>, WTF::HashTraits<WebKit::WKRetainPtr<OpaqueWKBundleScriptWorld const*> > >::operator new(unsigned long) ($
==962==    by 0xD8BBB89: WTR::worldMap() (LayoutTestController.cpp:461)
==962==    by 0xD8BBBD5: WTR::LayoutTestController::worldIDForWorld(OpaqueWKBundleScriptWorld const*) (LayoutTestController.cpp:467)
...

[Alejandro G. Castro]

Created attachment 109753 [details]
Proposed patch

[Martin Robinson]

Comment on attachment 109753 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109753&action=review

Would it be sufficient to simply add 
#if defined (BUILDING_GTK__)
#include "autotoolsconfig.h"
#endif /* defined (BUILDING_GTK__) */

to the prefix header?

I'm wondering if there is some way we can #error out if the includes are in the wrong order. What do you think?

> Tools/WebKitTestRunner/PixelDumpSupport.h:32
> +#include <stdlib.h>

Hrm...why is this necessary?

[Alejandro G. Castro]

(In reply to comment #2)
> (From update of attachment 109753 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109753&action=review
> 
> Would it be sufficient to simply add 
> #if defined (BUILDING_GTK__)
> #include "autotoolsconfig.h"
> #endif /* defined (BUILDING_GTK__) */
> 
> to the prefix header?
> 

Probably it would work, but I was trying to get the same structure we have in all the  other config.h files in the repository, with the autotoolsconfig.h before the Platform.h. And I thought it is a better option to have the WebKit2.h included in the files that requires it instead of adding it to all the compilations.

Actually I was wondering if we could even remove the Prefix.h and just have the config.h.

> I'm wondering if there is some way we can #error out if the includes are in the wrong order. What do you think?
> 

Never tried it, I can check it.

> > Tools/WebKitTestRunner/PixelDumpSupport.h:32
> > +#include <stdlib.h>
> 
> Hrm...why is this necessary?

Apparently it is needed for the size_t now that WebKit2.h is not included in every compilation.

[Alejandro G. Castro]

Created attachment 111946 [details]
Proposed patch

Used the solution of adding the include to the prefix file. I've checked the option to add an #error in the Platform.h but we are not respecting the rule in JSC. Currently it is not a problem because those files do not use the DEFINES in Platform.h, so the code would be a little bit overkill in the my opinion to be added in this patch. I think we can check it in the future if we find this situation is common.

[Martin Robinson]

Comment on attachment 111946 [details]
Proposed patch

Nice!

[Alejandro G. Castro]

Thanks! Landed: http://trac.webkit.org/changeset/98221