Bug 179463

Summary:

Isolated Heaps caused an increase in reported leaks on the bots

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

bmalloc

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, bfulgham, darin, ews-watchlist, fpizlo, ggaren, joepeck, rniwa, ryanhaddad, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
work in progress	none
more	none
it compiles	none
the patch	none
the patch	darin: review+

Joseph Pecoraro

Reported 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

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 attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2017-11-08 19:45:00 PST

<rdar://problem/35433570>

Geoffrey Garen

Comment 2 2017-11-09 10:34:05 PST

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

Filip Pizlo

Comment 3 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.

Filip Pizlo

Comment 4 2017-11-09 16:09:43 PST

Created attachment 326502 [details] work in progress

Filip Pizlo

Comment 5 2017-11-09 18:21:29 PST

Created attachment 326526 [details] more

Filip Pizlo

Comment 6 2017-11-09 20:49:56 PST

Created attachment 326545 [details] it compiles

Filip Pizlo

Comment 7 2017-11-16 14:14:44 PST

Created attachment 327106 [details] the patch

EWS Watchlist

Comment 8 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.

Joseph Pecoraro

Comment 9 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

Filip Pizlo

Comment 10 2017-11-17 14:43:51 PST

Created attachment 327244 [details] the patch

EWS Watchlist

Comment 11 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.

Darin Adler

Comment 12 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"

Filip Pizlo

Comment 13 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.

Filip Pizlo

Comment 14 2017-11-23 16:48:40 PST

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

Ryan Haddad

Comment 15 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

Filip Pizlo

Comment 16 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.

Filip Pizlo

Comment 17 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

Note You need to log in before you can comment on or make changes to this bug.