Bug 179463 – Isolated Heaps caused an increase in reported leaks on the bots

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Joseph Pecoraro]

Isolated Heaps caused an increase in reported leaks on the bots

The leaks bot saw a large jump in reported leaks once iso-heaps was enabled:
https://build.webkit.org/builders/Apple%20High%20Sierra%20%28Leaks%29?numbuilds=50

> Nov 07 14:26	#782	Failed 18278 total leaks found for a total of 2,786,128 bytes. 491 unique leaks found. 5 failures 86 new passes
> Nov 07 13:43	#781	Failed 17084 total leaks found for a total of 1,956,160 bytes. 468 unique leaks found. 2 failures 85 new passes
> Nov 07 12:59	#780	Failed 39904 total leaks found for a total of 2,416,544 bytes. 463 unique leaks found. 6 failures 85 new passes
> Nov 07 12:16	#779	Failed 19515 total leaks found for a total of 2,689,152 bytes. 513 unique leaks found. 3 failures 86 new passes
> Nov 07 11:29	#778	Failed 5109 total leaks found for a total of 483,168 bytes. 385 unique leaks found. 6 failures 86 new passes
> Nov 07 10:48	#777	Failed 5438 total leaks found for a total of 579,648 bytes. 438 unique leaks found. 5 failures 86 new passes
> Nov 07 10:07	#776	Failed 5766 total leaks found for a total of 645,120 bytes. 389 unique leaks found. 7 failures 86 new passes
> Nov 07 08:59	#775	Failed 4415 total leaks found for a total of 385,248 bytes. 252 unique leaks found. 3 failures 86 new passes

The jump came with Iso-Heaps:
https://build.webkit.org/builders/Apple%20High%20Sierra%20%28Leaks%29/builds/779

[Radar WebKit Bug Importer]

<rdar://problem/35433570>

[Geoffrey Garen]

Seems like maybe iOS-heaps didn't adopt the Zone API?

[Filip Pizlo]

(In reply to Geoffrey Garen from comment #2)
> Seems like maybe iOS-heaps didn't adopt the Zone API?

Yeah. And closely related, it doesn’t use DebugHeap when necessary. I’m fixing both things.

[Filip Pizlo]

Created attachment 326502 [details]
work in progress

[Filip Pizlo]

Created attachment 326526 [details]
more

[Filip Pizlo]

Created attachment 326545 [details]
it compiles

[Filip Pizlo]

Created attachment 327106 [details]
the patch

[EWS Watchlist]

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


ERROR: Source/bmalloc/bmalloc/bmalloc.h:73:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:73:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:82:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:82:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:93:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.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]
Total errors found: 6 in 12 files


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

[Joseph Pecoraro]

Comment on attachment 327106 [details]
the patch

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

Nice! I don't think I'm qualified to review this, but I'm excited to see it addressed!

> Source/bmalloc/bmalloc/IsoPage.cpp:23
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 

I see this continues the trend of all bmalloc copyright comments having trailing whitespace =P

[Filip Pizlo]

Created attachment 327244 [details]
the patch

[EWS Watchlist]

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


ERROR: Source/bmalloc/bmalloc/bmalloc.h:73:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:73:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:82:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:82:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:93:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.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]
Total errors found: 6 in 13 files


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

[Darin Adler]

Comment on attachment 327244 [details]
the patch

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

Read this carefully. Did not spot any errors. Yet all my comments are about trivia.

> Source/bmalloc/bmalloc/DebugHeap.h:50
> -
> +    

Added whitespace here.

> Source/bmalloc/bmalloc/IsoHeap.h:60
> -    
> +

Removed whitespace here.

Doesn’t seem like we should make both those changes.

> Source/bmalloc/bmalloc/IsoTLS.h:108
> -
> +    

Again adding whitespace.

> Source/bmalloc/bmalloc/IsoTLSInlines.h:99
> +    // since we don't want unpredictable behavior of offset or m_extent ever got corrupted.

typo: "of" instead of "if"

[Filip Pizlo]

(In reply to Darin Adler from comment #12)
> Comment on attachment 327244 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327244&action=review
> 
> Read this carefully. Did not spot any errors. Yet all my comments are about
> trivia.
> 
> > Source/bmalloc/bmalloc/DebugHeap.h:50
> > -
> > +    
> 
> Added whitespace here.

Removed.

> 
> > Source/bmalloc/bmalloc/IsoHeap.h:60
> > -    
> > +
> 
> Removed whitespace here.
> 
> Doesn’t seem like we should make both those changes.

Removed.

> 
> > Source/bmalloc/bmalloc/IsoTLS.h:108
> > -
> > +    
> 
> Again adding whitespace.

Fixed.

> 
> > Source/bmalloc/bmalloc/IsoTLSInlines.h:99
> > +    // since we don't want unpredictable behavior of offset or m_extent ever got corrupted.
> 
> typo: "of" instead of "if"

Fixed.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/225125/webkit

[Ryan Haddad]

(In reply to Filip Pizlo from comment #14)
> Landed in http://trac.webkit.org/changeset/225125/webkit
This change caused crashes on the leaks bot:
https://build.webkit.org/builders/Apple%20High%20Sierra%20%28Leaks%29/builds/1082

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010fc138c7 0x10f083000 + 12126407
1   com.apple.JavaScriptCore      	0x000000010fc1c6ce bmalloc::api::scavenge() + 46 (bmalloc.cpp:62)
2   com.apple.WebCore             	0x0000000112364ff4 WebCore::GCController::garbageCollectNow() + 180
3   DumpRenderTree                	0x000000010ee27fee collectCallback(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**) + 25 (GCController.cpp:49)
4   com.apple.JavaScriptCore      	0x000000010f0b68a5 long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*) + 501 (APICallbackFunction.h:63)
5   ???                           	0x000051c9bfe01023 0 + 89926949408803
6   com.apple.JavaScriptCore      	0x000000010f08b60b llint_entry + 28563
7   com.apple.JavaScriptCore      	0x000000010f08b60b llint_entry + 28563
8   com.apple.JavaScriptCore      	0x000000010f08b60b llint_entry + 28563
9   com.apple.JavaScriptCore      	0x000000010f084490 vmEntryToJavaScript + 304
10  com.apple.JavaScriptCore      	0x000000010f7706cf JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 127 (JITCode.cpp:82)
11  com.apple.JavaScriptCore      	0x000000010f73deba JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) + 14314 (Interpreter.cpp:940)
12  com.apple.JavaScriptCore      	0x000000010f92cd73 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 307 (Completion.cpp:103)
13  com.apple.WebCore             	0x0000000112396081 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) + 305 (ScriptController.cpp:177)
14  com.apple.WebCore             	0x0000000112394831 WebCore::ScriptController::executeScriptInWorld(WebCore::DOMWrapperWorld&, WTF::String const&, bool) + 529 (ScriptController.cpp:665)
15  com.apple.WebCore             	0x0000000112393e8c WebCore::ScheduledAction::execute(WebCore::Document&) + 172 (ScheduledAction.cpp:144)
16  com.apple.WebCore             	0x000000011292dc3b WebCore::DOMTimer::fired() + 715 (InspectorInstrumentation.h:819)
17  com.apple.WebCore             	0x0000000112a0c9d0 WebCore::ThreadTimers::sharedTimerFiredInternal() + 176 (ThreadTimers.cpp:121)
18  com.apple.WebCore             	0x0000000112a45bbf WebCore::timerFired(__CFRunLoopTimer*, void*) + 31 (MainThreadSharedTimerCF.cpp:75)
19  com.apple.CoreFoundation      	0x00007fff2e81c2d4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
20  com.apple.CoreFoundation      	0x00007fff2e81bf54 __CFRunLoopDoTimer + 1108
21  com.apple.CoreFoundation      	0x00007fff2e81ba4a __CFRunLoopDoTimers + 346
22  com.apple.CoreFoundation      	0x00007fff2e81321b __CFRunLoopRun + 2427
23  com.apple.CoreFoundation      	0x00007fff2e812607 CFRunLoopRunSpecific + 487
24  DumpRenderTree                	0x000000010ee1d2fe runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 2673 (DumpRenderTree.mm:2030)
25  DumpRenderTree                	0x000000010ee1c62e dumpRenderTree(int, char const**) + 3522 (DumpRenderTree.mm:1294)
26  DumpRenderTree                	0x000000010ee1ddd8 DumpRenderTreeMain(int, char const**) + 1454 (DumpRenderTree.mm:1410)
27  libdyld.dylib                 	0x00007fff55e1b145 start + 1

[Filip Pizlo]

(In reply to Ryan Haddad from comment #15)
> (In reply to Filip Pizlo from comment #14)
> > Landed in http://trac.webkit.org/changeset/225125/webkit
> This change caused crashes on the leaks bot:
> https://build.webkit.org/builders/Apple%20High%20Sierra%20%28Leaks%29/builds/
> 1082
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.JavaScriptCore      	0x000000010fc138c7 0x10f083000 + 12126407
> 1   com.apple.JavaScriptCore      	0x000000010fc1c6ce
> bmalloc::api::scavenge() + 46 (bmalloc.cpp:62)
> 2   com.apple.WebCore             	0x0000000112364ff4
> WebCore::GCController::garbageCollectNow() + 180
> 3   DumpRenderTree                	0x000000010ee27fee
> collectCallback(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*,
> unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**) + 25
> (GCController.cpp:49)
> 4   com.apple.JavaScriptCore      	0x000000010f0b68a5 long long
> JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*) +
> 501 (APICallbackFunction.h:63)
> 5   ???                           	0x000051c9bfe01023 0 + 89926949408803
> 6   com.apple.JavaScriptCore      	0x000000010f08b60b llint_entry + 28563
> 7   com.apple.JavaScriptCore      	0x000000010f08b60b llint_entry + 28563
> 8   com.apple.JavaScriptCore      	0x000000010f08b60b llint_entry + 28563
> 9   com.apple.JavaScriptCore      	0x000000010f084490 vmEntryToJavaScript +
> 304
> 10  com.apple.JavaScriptCore      	0x000000010f7706cf
> JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 127 (JITCode.cpp:82)
> 11  com.apple.JavaScriptCore      	0x000000010f73deba
> JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*,
> JSC::JSObject*) + 14314 (Interpreter.cpp:940)
> 12  com.apple.JavaScriptCore      	0x000000010f92cd73
> JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue,
> WTF::NakedPtr<JSC::Exception>&) + 307 (Completion.cpp:103)
> 13  com.apple.WebCore             	0x0000000112396081
> WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&,
> WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) + 305
> (ScriptController.cpp:177)
> 14  com.apple.WebCore             	0x0000000112394831
> WebCore::ScriptController::executeScriptInWorld(WebCore::DOMWrapperWorld&,
> WTF::String const&, bool) + 529 (ScriptController.cpp:665)
> 15  com.apple.WebCore             	0x0000000112393e8c
> WebCore::ScheduledAction::execute(WebCore::Document&) + 172
> (ScheduledAction.cpp:144)
> 16  com.apple.WebCore             	0x000000011292dc3b
> WebCore::DOMTimer::fired() + 715 (InspectorInstrumentation.h:819)
> 17  com.apple.WebCore             	0x0000000112a0c9d0
> WebCore::ThreadTimers::sharedTimerFiredInternal() + 176
> (ThreadTimers.cpp:121)
> 18  com.apple.WebCore             	0x0000000112a45bbf
> WebCore::timerFired(__CFRunLoopTimer*, void*) + 31
> (MainThreadSharedTimerCF.cpp:75)
> 19  com.apple.CoreFoundation      	0x00007fff2e81c2d4
> __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
> 20  com.apple.CoreFoundation      	0x00007fff2e81bf54 __CFRunLoopDoTimer +
> 1108
> 21  com.apple.CoreFoundation      	0x00007fff2e81ba4a __CFRunLoopDoTimers +
> 346
> 22  com.apple.CoreFoundation      	0x00007fff2e81321b __CFRunLoopRun + 2427
> 23  com.apple.CoreFoundation      	0x00007fff2e812607 CFRunLoopRunSpecific +
> 487
> 24  DumpRenderTree                	0x000000010ee1d2fe
> runTest(std::__1::basic_string<char, std::__1::char_traits<char>,
> std::__1::allocator<char> > const&) + 2673 (DumpRenderTree.mm:2030)
> 25  DumpRenderTree                	0x000000010ee1c62e dumpRenderTree(int,
> char const**) + 3522 (DumpRenderTree.mm:1294)
> 26  DumpRenderTree                	0x000000010ee1ddd8
> DumpRenderTreeMain(int, char const**) + 1454 (DumpRenderTree.mm:1410)
> 27  libdyld.dylib                 	0x00007fff55e1b145 start + 1

Weird, it didn't happen locally.  But it looks obvious.  Fixing.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #16)
> (In reply to Ryan Haddad from comment #15)
> > (In reply to Filip Pizlo from comment #14)
> > > Landed in http://trac.webkit.org/changeset/225125/webkit
> > This change caused crashes on the leaks bot:
> > https://build.webkit.org/builders/Apple%20High%20Sierra%20%28Leaks%29/builds/
> > 1082
> > 
> > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> > 0   com.apple.JavaScriptCore      	0x000000010fc138c7 0x10f083000 + 12126407
> > 1   com.apple.JavaScriptCore      	0x000000010fc1c6ce
> > bmalloc::api::scavenge() + 46 (bmalloc.cpp:62)
> > 2   com.apple.WebCore             	0x0000000112364ff4
> > WebCore::GCController::garbageCollectNow() + 180
> > 3   DumpRenderTree                	0x000000010ee27fee
> > collectCallback(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*,
> > unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**) + 25
> > (GCController.cpp:49)
> > 4   com.apple.JavaScriptCore      	0x000000010f0b68a5 long long
> > JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*) +
> > 501 (APICallbackFunction.h:63)
> > 5   ???                           	0x000051c9bfe01023 0 + 89926949408803
> > 6   com.apple.JavaScriptCore      	0x000000010f08b60b llint_entry + 28563
> > 7   com.apple.JavaScriptCore      	0x000000010f08b60b llint_entry + 28563
> > 8   com.apple.JavaScriptCore      	0x000000010f08b60b llint_entry + 28563
> > 9   com.apple.JavaScriptCore      	0x000000010f084490 vmEntryToJavaScript +
> > 304
> > 10  com.apple.JavaScriptCore      	0x000000010f7706cf
> > JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 127 (JITCode.cpp:82)
> > 11  com.apple.JavaScriptCore      	0x000000010f73deba
> > JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*,
> > JSC::JSObject*) + 14314 (Interpreter.cpp:940)
> > 12  com.apple.JavaScriptCore      	0x000000010f92cd73
> > JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue,
> > WTF::NakedPtr<JSC::Exception>&) + 307 (Completion.cpp:103)
> > 13  com.apple.WebCore             	0x0000000112396081
> > WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&,
> > WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) + 305
> > (ScriptController.cpp:177)
> > 14  com.apple.WebCore             	0x0000000112394831
> > WebCore::ScriptController::executeScriptInWorld(WebCore::DOMWrapperWorld&,
> > WTF::String const&, bool) + 529 (ScriptController.cpp:665)
> > 15  com.apple.WebCore             	0x0000000112393e8c
> > WebCore::ScheduledAction::execute(WebCore::Document&) + 172
> > (ScheduledAction.cpp:144)
> > 16  com.apple.WebCore             	0x000000011292dc3b
> > WebCore::DOMTimer::fired() + 715 (InspectorInstrumentation.h:819)
> > 17  com.apple.WebCore             	0x0000000112a0c9d0
> > WebCore::ThreadTimers::sharedTimerFiredInternal() + 176
> > (ThreadTimers.cpp:121)
> > 18  com.apple.WebCore             	0x0000000112a45bbf
> > WebCore::timerFired(__CFRunLoopTimer*, void*) + 31
> > (MainThreadSharedTimerCF.cpp:75)
> > 19  com.apple.CoreFoundation      	0x00007fff2e81c2d4
> > __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
> > 20  com.apple.CoreFoundation      	0x00007fff2e81bf54 __CFRunLoopDoTimer +
> > 1108
> > 21  com.apple.CoreFoundation      	0x00007fff2e81ba4a __CFRunLoopDoTimers +
> > 346
> > 22  com.apple.CoreFoundation      	0x00007fff2e81321b __CFRunLoopRun + 2427
> > 23  com.apple.CoreFoundation      	0x00007fff2e812607 CFRunLoopRunSpecific +
> > 487
> > 24  DumpRenderTree                	0x000000010ee1d2fe
> > runTest(std::__1::basic_string<char, std::__1::char_traits<char>,
> > std::__1::allocator<char> > const&) + 2673 (DumpRenderTree.mm:2030)
> > 25  DumpRenderTree                	0x000000010ee1c62e dumpRenderTree(int,
> > char const**) + 3522 (DumpRenderTree.mm:1294)
> > 26  DumpRenderTree                	0x000000010ee1ddd8
> > DumpRenderTreeMain(int, char const**) + 1454 (DumpRenderTree.mm:1410)
> > 27  libdyld.dylib                 	0x00007fff55e1b145 start + 1
> 
> Weird, it didn't happen locally.  But it looks obvious.  Fixing.

It didn't happen locally because my test sucked.

Fix and better test landed in http://trac.webkit.org/changeset/225180/webkit