Bug 57502 – Make malloc validation useful

• Component: Web Template Framework
• Status: REOPENED

[Oliver Hunt]

Make malloc validation useful

[Oliver Hunt]

Created attachment 87606 [details]
Patch

[WebKit Review Bot]

Attachment 87606 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/FastMalloc.h:193:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:169:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:227:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:340:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1304:  pageheap_lock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:2480:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:3827:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:3992:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 8 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Attachment 87606 [details] did not build on win:
Build output: http://queues.webkit.org/results/8280969

[Geoffrey Garen]

2>..\..\wtf\FastMalloc.cpp(236) : error C2220: warning treated as error - no 'object' file generated
2>..\..\wtf\FastMalloc.cpp(236) : warning C4245: '=' : conversion from '' to 'WTF::Internal::ValidationTag', signed/unsigned mismatch

[Geoffrey Garen]

+    memset(p, 0xCC, header->m_size);

I think 0xbbadbeef would be better, since that's that standard dereference we look for to indicate assertion failure.

+        //if (Internal::fastMallocMatchValidationType(p) != allocType)
+        //    Internal::fastMallocMatchFailed(p);

Boo on commented-out code.

+    //ASSERT(IsHeld(pageheap_lock));

Boo on commented-out code.

-    ASSERT(IsHeld(pageheap_lock));
+    ASSERT(pageheap_lock.IsHeld());

Why all of these changes?

r- because of the build failure.

[Oliver Hunt]

(In reply to comment #5)
> +    memset(p, 0xCC, header->m_size);
> 
> I think 0xbbadbeef would be better, since that's that standard dereference we look for to indicate assertion failure.

yeah you'd think memset would be able to plan an entire int given that it takes an int argument, but it converts to unsigned char first.  Hence 0xCC.

> 
> +        //if (Internal::fastMallocMatchValidationType(p) != allocType)
> +        //    Internal::fastMallocMatchFailed(p);
> 
> Boo on commented-out code.
Removed -- it would be nice to add this back, but currently new/free mismatches exist all over the place

> 
> +    //ASSERT(IsHeld(pageheap_lock));
> 
> Boo on commented-out code.
Fixed

> 
> -    ASSERT(IsHeld(pageheap_lock));
> +    ASSERT(pageheap_lock.IsHeld());
> 
> Why all of these changes?

Apparently no one has done a debug build of fast malloc for a long time.

> 
> r- because of the build failure.

[Oliver Hunt]

Created attachment 87622 [details]
Patch

[WebKit Review Bot]

Attachment 87622 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/FastMalloc.cpp:169:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:227:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:340:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1304:  pageheap_lock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:3827:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:3992:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Attachment 87622 [details] did not build on win:
Build output: http://queues.webkit.org/results/8311020

[Geoffrey Garen]

Comment on attachment 87622 [details]
Patch

Still failing to build.

[Oliver Hunt]

Created attachment 88089 [details]
Patch

[WebKit Review Bot]

Attachment 88089 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/JavaScriptCore/wtf/FastMalloc.cpp:169:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:227:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:340:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1304:  pageheap_lock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:3827:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:3992:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Created attachment 88103 [details]
Patch

[WebKit Review Bot]

Attachment 88103 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/JavaScriptCore/wtf/FastMalloc.cpp:169:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:227:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:340:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1306:  pageheap_lock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:3829:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:3994:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 88103 [details]
Patch

r=me, now that the build works and debug builds do not ASSERT.

[Oliver Hunt]

Committed r82876: <http://trac.webkit.org/changeset/82876>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82876 might have broken GTK Linux 32-bit Release

[Kent Tamura]

(In reply to comment #16)
> Committed r82876: <http://trac.webkit.org/changeset/82876>

I reverted it because of test crashes on the SnowLeopard bot. (Release-only?)