Bug 179463 - Isolated Heaps caused an increase in reported leaks on the bots
Summary: Isolated Heaps caused an increase in reported leaks on the bots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-08 19:43 PST by Joseph Pecoraro
Modified: 2017-11-27 10:25 PST (History)
10 users (show)

See Also:

Attachments
work in progress (8.59 KB, patch)
2017-11-09 16:09 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
more (10.02 KB, patch)
2017-11-09 18:21 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
it compiles (19.50 KB, patch)
2017-11-09 20:49 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (21.85 KB, patch)
2017-11-16 14:14 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (22.26 KB, patch)
2017-11-17 14:43 PST, Filip Pizlo 		darin: review+
Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-11-08 19:43:39 PST 
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
Comment 1 Radar WebKit Bug Importer 2017-11-08 19:45:00 PST 
<rdar://problem/35433570>
Comment 2 Geoffrey Garen 2017-11-09 10:34:05 PST 
Seems like maybe iOS-heaps didn't adopt the Zone API?
Comment 3 Filip Pizlo 2017-11-09 11:56:49 PST 
(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.
Comment 4 Filip Pizlo 2017-11-09 16:09:43 PST 
Created attachment 326502 [details]
work in progress
Comment 5 Filip Pizlo 2017-11-09 18:21:29 PST 
Created attachment 326526 [details]
more
Comment 6 Filip Pizlo 2017-11-09 20:49:56 PST 
Created attachment 326545 [details]
it compiles
Comment 7 Filip Pizlo 2017-11-16 14:14:44 PST 
Created attachment 327106 [details]
the patch
Comment 8 EWS Watchlist 2017-11-16 14:16:54 PST 
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.
Comment 9 Joseph Pecoraro 2017-11-17 11:07:40 PST 
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
Comment 10 Filip Pizlo 2017-11-17 14:43:51 PST 
Created attachment 327244 [details]
the patch
Comment 11 EWS Watchlist 2017-11-17 14:45:03 PST 
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.
Comment 12 Darin Adler 2017-11-22 11:46:21 PST 
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"
Comment 13 Filip Pizlo 2017-11-23 16:46:18 PST 
(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.
Comment 14 Filip Pizlo 2017-11-23 16:48:40 PST 
Landed in http://trac.webkit.org/changeset/225125/webkit
Comment 15 Ryan Haddad 2017-11-27 10:06:14 PST 
(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
Comment 16 Filip Pizlo 2017-11-27 10:08:11 PST 
(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.
Comment 17 Filip Pizlo 2017-11-27 10:25:28 PST 
(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