Bug 155409 - bmalloc: add logging for mmap() failures
Summary: bmalloc: add logging for mmap() failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks: 155992 158660 178740
  Show dependency treegraph
 
Reported: 2016-03-13 13:08 PDT by David Kilzer (:ddkilzer)
Modified: 2020-11-04 10:29 PST (History)
4 users (show)

See Also:


Attachments
Patch v1 (14.73 KB, patch)
2016-03-13 13:14 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (14.82 KB, patch)
2016-03-13 13:26 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (14.73 KB, patch)
2016-03-13 13:36 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v4 (15.23 KB, patch)
2016-03-15 12:49 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v5 (15.25 KB, patch)
2016-03-15 12:55 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v6 (15.23 KB, patch)
2016-03-15 15:30 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v7 (15.23 KB, patch)
2016-03-15 17:06 PDT, David Kilzer (:ddkilzer)
sbarati: review+
ddkilzer: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2016-03-13 13:08:30 PDT
We need to add logging for mmap() failures so we know how frequent they are, and under what conditions they occur.
Comment 1 David Kilzer (:ddkilzer) 2016-03-13 13:08:57 PDT
<rdar://problem/24568515>
Comment 2 David Kilzer (:ddkilzer) 2016-03-13 13:14:57 PDT
Created attachment 273897 [details]
Patch v1
Comment 3 WebKit Commit Bot 2016-03-13 13:17:21 PDT
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.
Comment 4 David Kilzer (:ddkilzer) 2016-03-13 13:26:35 PDT
Created attachment 273898 [details]
Patch v2
Comment 5 WebKit Commit Bot 2016-03-13 13:27:56 PDT
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.
Comment 6 David Kilzer (:ddkilzer) 2016-03-13 13:36:25 PDT
Created attachment 273899 [details]
Patch v3
Comment 7 WebKit Commit Bot 2016-03-13 13:38:37 PDT
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 8 Mark Lam 2016-03-14 15:38:48 PDT
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 9 Geoffrey Garen 2016-03-14 15:45:02 PDT
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 10 David Kilzer (:ddkilzer) 2016-03-14 22:19:05 PDT
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?
Comment 11 David Kilzer (:ddkilzer) 2016-03-15 12:49:27 PDT
Created attachment 274113 [details]
Patch v4
Comment 12 WebKit Commit Bot 2016-03-15 12:50:39 PDT
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.
Comment 13 David Kilzer (:ddkilzer) 2016-03-15 12:52:20 PDT
(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.
Comment 14 David Kilzer (:ddkilzer) 2016-03-15 12:55:25 PDT
Created attachment 274114 [details]
Patch v5
Comment 15 WebKit Commit Bot 2016-03-15 12:57:17 PDT
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.
Comment 16 David Kilzer (:ddkilzer) 2016-03-15 15:30:21 PDT
Created attachment 274145 [details]
Patch v6
Comment 17 WebKit Commit Bot 2016-03-15 15:32:50 PDT
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.
Comment 18 David Kilzer (:ddkilzer) 2016-03-15 17:06:53 PDT
Created attachment 274156 [details]
Patch v7
Comment 19 WebKit Commit Bot 2016-03-15 17:08:07 PDT
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 20 Saam Barati 2016-03-18 11:54:49 PDT
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 21 Saam Barati 2016-03-18 11:59:53 PDT
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 22 David Kilzer (:ddkilzer) 2016-03-29 16:07:22 PDT
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
Comment 23 David Kilzer (:ddkilzer) 2016-03-29 16:19:39 PDT
Committed r198809: <http://trac.webkit.org/changeset/198809>
Comment 24 David Kilzer (:ddkilzer) 2016-06-11 06:09:22 PDT
(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&&> >