Bug 192491 – [WTF] Debug build fails due conflicting abort() method

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Adrian Perez]

When building passing -DCMAKE_BUILD_TYPE=Debug:

 - The XMLHttpRequest class defines an ::abort() method (in Source/WebCore/xml/XMLHttpRequest.h)
 - The CRASH() and CRASH_UNDER_CONSTEXPR_CONTEXT macros (in Source/WTF/wtf/Assertions.h) call
   the abort() function, but inside the code for XMLHttpRequest that would implicitly call
   the method. Ugh.

Probably the best solution is to make the macros in Assertions.h use the
namespace (so they expand to std::abort), to avoid the method being picked
when expandin the macros.

Compilation error follows.

----

In file included from DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:32,
                 from DerivedSources/ForwardingHeaders/wtf/FastMalloc.h:25,
                 from ../Source/WebCore/config.h:51,
                 from CSSValueKeywords.gperf:4,
                 from DerivedSources/WebCore/unified-sources/UnifiedSource1.cpp:1:
../Source/WebCore/xml/XMLHttpRequest.h: In static member function ‘static void* WebCore::XMLHttpRequest::operator new(size_t, NotNullTag, void*)’:
DerivedSources/ForwardingHeaders/wtf/Assertions.h:244:23: error: cannot call member function ‘void WebCore::XMLHttpRequest::abort()’ without object
 #define CRASH() abort()
                       ^
DerivedSources/ForwardingHeaders/wtf/Assertions.h:587:30: note: in expansion of macro ‘CRASH’
 #define CRASH_WITH_INFO(...) CRASH()
                              ^~~~~
DerivedSources/ForwardingHeaders/wtf/Assertions.h:323:9: note: in expansion of macro ‘CRASH_WITH_INFO’
         CRASH_WITH_INFO(__VA_ARGS__); \
         ^~~~~~~~~~~~~~~
DerivedSources/ForwardingHeaders/wtf/FastMalloc.h:288:9: note: in expansion of macro ‘ASSERT’
         ASSERT(location); \
         ^~~~~~
DerivedSources/ForwardingHeaders/wtf/FastMalloc.h:294:5: note: in expansion of macro ‘WTF_MAKE_FAST_ALLOCATED_IMPL’
     WTF_MAKE_FAST_ALLOCATED_IMPL \DerivedSources/ForwardingHeaders/wtf/Assertions.h:244:23: error: cannot call member function ‘void WebCore::XMLHttpRequest::abort()’ without object
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../Source/WebCore/xml/XMLHttpRequest.h:53:5: note: in expansion of macro ‘WTF_MAKE_FAST_ALLOCATED’
     WTF_MAKE_FAST_ALLOCATED;
     ^~~~~~~~~~~~~~~~~~~~~~~

[Adrian Perez]

Created attachment 356789 [details]
Patch

[Michael Catanzaro]

Comment on attachment 356789 [details]
Patch

I wonder why none of the bots noticed this. Or why it doesn't affect me locally. I've done many debug builds.

This was good thinking, but unfortunately this is one of a very few headers that get included in C (and Objective C) so we can't use namespaces here. E.g. from EWS:

/home/buildbot/ews/WebKit/Source/JavaScriptCore/API/tests/testapi.c: In function ‘createStringWithContentsOfFile’:
DerivedSources/ForwardingHeaders/wtf/Assertions.h:244:21: error: expected expression before ‘:’ token
 #define CRASH() std::abort()
                     ^

I was going to suggest renaming XMLHttpRequest::abort instead, but there are more abort()s in the codebase... NetworkResourceLoader.cpp, testbmalloc.cpp....

We could add a new conditional to check #if __cplusplus? But that would be messier.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 356789 [details]
> Patch
> 
> I wonder why none of the bots noticed this. Or why it doesn't affect me
> locally. I've done many debug builds.
> 
> This was good thinking, but unfortunately this is one of a very few headers
> that get included in C (and Objective C) so we can't use namespaces here.
> E.g. from EWS:
> 
> /home/buildbot/ews/WebKit/Source/JavaScriptCore/API/tests/testapi.c: In
> function ‘createStringWithContentsOfFile’:
> DerivedSources/ForwardingHeaders/wtf/Assertions.h:244:21: error: expected
> expression before ‘:’ token
>  #define CRASH() std::abort()
>                      ^
> 
> I was going to suggest renaming XMLHttpRequest::abort instead, but there are
> more abort()s in the codebase... NetworkResourceLoader.cpp,
> testbmalloc.cpp....
> 
> We could add a new conditional to check #if __cplusplus? But that would be
> messier.

I think checking for __cplusplus is reasonable, I'll update the patch.

[Adrian Perez]

Created attachment 356878 [details]
Patch

[Michael Catanzaro]

Comment on attachment 356878 [details]
Patch

:/

[WebKit Commit Bot]

Comment on attachment 356878 [details]
Patch

Clearing flags on attachment: 356878

Committed r239012: <https://trac.webkit.org/changeset/239012>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46576939>