Bug 155409

<rdar://problem/24568515>

Created attachment 273897 [details] Patch v1

Attachment 273897 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/BSoftLinking.h:47: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/BSoftLinking.h:53: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:32: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 273898 [details] Patch v2

Attachment 273898 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/BSoftLinking.h:47: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/BSoftLinking.h:53: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:34: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 273899 [details] Patch v3

Attachment 273899 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/BSoftLinking.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/BSoftLinking.h:54: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:34: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 273899 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=273899&action=review > Source/bmalloc/bmalloc/BSoftLinking.h:63 > +#define BSOFT_LINK_FRAMEWORK(framework) \ > + static void* framework##Library() \ > + { \ > + static void* frameworkLibrary = ^{ \ > + void* result = dlopen("/System/Library/Frameworks/" #framework ".framework/" #framework, RTLD_NOW); \ > + RELEASE_BASSERT_WITH_MESSAGE(result, "%s", dlerror()); \ > + return result; \ > + }(); \ > + return frameworkLibrary; \ > + } > + > +#define BSOFT_LINK_FUNCTION(framework, functionName, resultType, parameterDeclarations, parameterNames) \ > + extern "C" { \ > + resultType functionName parameterDeclarations; \ > + } \ > + static resultType init##functionName parameterDeclarations; \ > + static resultType (*softLink##functionName) parameterDeclarations = init##functionName; \ > + \ > + static resultType init##functionName parameterDeclarations \ > + { \ > + static dispatch_once_t once; \ > + dispatch_once(&once, ^{ \ > + softLink##functionName = (resultType (*) parameterDeclarations) dlsym(framework##Library(), #functionName); \ > + RELEASE_BASSERT_WITH_MESSAGE(softLink##functionName, "%s", dlerror()); \ > + }); \ > + return softLink##functionName parameterNames; \ > + } \ > + \ > + inline resultType functionName parameterDeclarations \ > + { \ > + return softLink##functionName parameterNames; \ > + } I see that these are similar to the SOFT_LINK_FRAMEWORK() and SOFT_LINK() macros in WebCore/platform/mac/SoftLinking.h. But here it is not #ifdef'ed as Mac/iOS specific. Should it be?

Comment on attachment 273899 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=273899&action=review > Source/bmalloc/bmalloc/BSoftLinking.h:55 > + static dispatch_once_t once; \ > + dispatch_once(&once, ^{ \ > + softLink##functionName = (resultType (*) parameterDeclarations) dlsym(framework##Library(), #functionName); \ > + RELEASE_BASSERT_WITH_MESSAGE(softLink##functionName, "%s", dlerror()); \ Can you use some mechanism other than dispatch? We'd like to avoid relying on lower-level libraries inside malloc. For example, you can probably use a static std::atomic.

Comment on attachment 273899 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=273899&action=review >> Source/bmalloc/bmalloc/BSoftLinking.h:55 >> + RELEASE_BASSERT_WITH_MESSAGE(softLink##functionName, "%s", dlerror()); \ > > Can you use some mechanism other than dispatch? We'd like to avoid relying on lower-level libraries inside malloc. > > For example, you can probably use a static std::atomic. Makes sense. Will do so in a follow-up patch. >> Source/bmalloc/bmalloc/BSoftLinking.h:63 >> + } > > I see that these are similar to the SOFT_LINK_FRAMEWORK() and SOFT_LINK() macros in WebCore/platform/mac/SoftLinking.h. But here it is not #ifdef'ed as Mac/iOS specific. Should it be? Well, it's only used inside a #if BPLATFORM(IOS) macro in Logging.cpp, so it doesn't need it currently. I'd be happy to: - Start creating platform subdirectories (like 'darwin'?) for these files. - Add #if macros around them. As another example, Zone.{cpp,h} is also Darwin-only, but doesn't have the header. (It's also missing in the CMake build for OS X and iOS because it can't be included for all platform due to the missing macro protection.) Looking forward, it's probably something we don't want to have to protect explicitly in source files, so I'll add the macros. Should I add the darwin/ directory as well?

Created attachment 274113 [details] Patch v4

Attachment 274113 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/darwin/BSoftLinking.h:50: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/darwin/BSoftLinking.h:56: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:34: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #11) > Created attachment 274113 [details] > Patch v4 I didn't add the #if OS(DARWIN)/#endif macros to BSoftLinking.h because it's only used in that context already. (Also discussed it with Geoff.) I did put BSoftLinking.h into a new darwin/ subdirectory, and changed both soft-linking methods to use std::call_once to be consistent.

Created attachment 274114 [details] Patch v5

Attachment 274114 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/darwin/BSoftLinking.h:51: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/darwin/BSoftLinking.h:57: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:34: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 274145 [details] Patch v6

Attachment 274145 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/darwin/BSoftLinking.h:49: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/darwin/BSoftLinking.h:55: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:34: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 274156 [details] Patch v7

Attachment 274156 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/darwin/BSoftLinking.h:49: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/darwin/BSoftLinking.h:55: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/bmalloc/bmalloc/Logging.cpp:34: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 274156 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=274156&action=review r=me > Source/bmalloc/bmalloc/BAssert.h:66 > +// FIXME: Output log message. Is there a bugzilla opened for this? If not I think we should open one and link it here.

Comment on attachment 274156 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=274156&action=review > Source/bmalloc/ChangeLog:8 > + I think a line or two of explanation here is also helpful.

Comment on attachment 274156 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=274156&action=review >> Source/bmalloc/ChangeLog:8 >> + > > I think a line or two of explanation here is also helpful. Will do. >> Source/bmalloc/bmalloc/BAssert.h:66 >> +// FIXME: Output log message. > > Is there a bugzilla opened for this? If not I think we should open one and link it here. Filed: https://bugs.webkit.org/show_bug.cgi?id=155992

Committed r198809: <http://trac.webkit.org/changeset/198809>

(In reply to comment #23) > Committed r198809: <http://trac.webkit.org/changeset/198809> This caused: Bug 158660: Crash in com.apple.WebKit.WebContent at std::__1::__call_once_proxy<std::__1::tuple<CrashReporterSupportLibrary()::$_0&&> >