Bug 142510 - Web Inspector: Include Garbage Collection Event in Timeline
Summary: Web Inspector: Include Garbage Collection Event in Timeline
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-03-09 15:19 PDT by Tobias Bosch
Modified: 2015-10-15 20:55 PDT (History)
11 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (77.41 KB, patch)
2015-09-28 17:37 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[IMAGE] GC in Overview (93.19 KB, image/png)
2015-09-28 17:39 PDT, Joseph Pecoraro
no flags Details
[IMAGE] GC in Scripts & Events (95.55 KB, image/png)
2015-09-28 17:39 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (77.45 KB, patch)
2015-10-01 19:15 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (Improve Test, Fix Builds) (78.86 KB, patch)
2015-10-01 19:36 PDT, Joseph Pecoraro
ggaren: review+
Details | Formatted Diff | Diff
[PATCH] For Bots (79.32 KB, patch)
2015-10-02 11:31 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (79.94 KB, patch)
2015-10-14 18:54 PDT, Joseph Pecoraro
bburg: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
[PATCH] For Landing (79.91 KB, patch)
2015-10-15 11:46 PDT, Joseph Pecoraro
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Bosch 2015-03-09 15:19:51 PDT
The Angular team is creating a benchmark tool, which reads out the inspector timeline data
to get detailed numbers about script time, rendering time as well as garbage collection time/amount.

To get stable numbers we are subtracting the gc time from script execution time. However,
GCEvents are no more reported in newer versions of Safari (see commits below). 
Is there a chance of getting them back? Also, showing them in the inspector UI would be nice for 
regular web developers as well.

Btw, is there a way to force gc in Safari / mobile Safari? We do this in Chrome and it helps to
stabilize our benchmark numbers further.

Relevant commits:
- File https://github.com/WebKit/webkit/blob/master/Source/WebCore/inspector/InspectorTimelineAgent.cpp,
- Commit d5b7f4f8f0956a2b0c05162ac1a25287a8c9c74c removed reporting GCEvents and switched to 
adding a `usedHeapSize` property to all records
- Commit 7fe486eb595c650aad97401aebc5086257cae8c4 removed gc reporting alltogether.

Thanks a lot,
Tobias
Comment 1 Joseph Pecoraro 2015-09-28 17:17:16 PDT
Retitling. Include Garbage Collection (GC) information in the Inspector Timeline.
Comment 2 Radar WebKit Bug Importer 2015-09-28 17:18:38 PDT
<rdar://problem/22889788>
Comment 3 Joseph Pecoraro 2015-09-28 17:37:28 PDT
Created attachment 262032 [details]
[PATCH] Proposed Fix
Comment 4 Joseph Pecoraro 2015-09-28 17:39:25 PDT
Created attachment 262033 [details]
[IMAGE] GC in Overview
Comment 5 Joseph Pecoraro 2015-09-28 17:39:48 PDT
Created attachment 262034 [details]
[IMAGE] GC in Scripts & Events
Comment 6 Tobias Bosch 2015-09-28 19:58:32 PDT
Nice, this looks promising!
Two questions: 

1. In which mobile safari version will this be included? In IOS 9.0 already?
2. Would it be even possible to report the gcAmount again in these events?

Thanks a lot for working on this!
Comment 7 Joseph Pecoraro 2015-09-28 21:10:04 PDT
(In reply to comment #6)
> Nice, this looks promising!
> Two questions: 
> 
> 1. In which mobile safari version will this be included? In IOS 9.0 already?

I can't comment on future versions of Safari.

> 2. Would it be even possible to report the gcAmount again in these events?

Yes, I'm looking into including the number of bytes freed by the collection. That is currently a FIXME in the patch above.
Comment 8 Timothy Hatcher 2015-09-28 21:46:45 PDT
iOS 9 has already shipped, so it would not have this change that was just posted.
Comment 9 Joseph Pecoraro 2015-10-01 19:15:39 PDT
Created attachment 262312 [details]
[PATCH] Proposed Fix

Rebaselined.
Comment 10 Joseph Pecoraro 2015-10-01 19:36:05 PDT
Created attachment 262313 [details]
[PATCH] Proposed Fix (Improve Test, Fix Builds)
Comment 11 Geoffrey Garen 2015-10-01 20:57:54 PDT
Comment on attachment 262313 [details]
[PATCH] Proposed Fix (Improve Test, Fix Builds)

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

r=me

> Source/JavaScriptCore/heap/Heap.cpp:1176
> +    for (auto* observer : m_observers)

I think WebKit usually calls this "client" rather than "observer".

> Source/JavaScriptCore/heap/Heap.cpp:1326
> +    HeapOperation operation = m_operationInProgress;

Revert this line?
Comment 12 Joseph Pecoraro 2015-10-02 11:04:21 PDT
Comment on attachment 262313 [details]
[PATCH] Proposed Fix (Improve Test, Fix Builds)

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

>> Source/JavaScriptCore/heap/Heap.cpp:1326
>> +    HeapOperation operation = m_operationInProgress;
> 
> Revert this line?

It is actually needed. We stash the operation before the member gets reassigned to NoOperation below (line 1348). Waiting until after the operation was reassigned was necessary because (in WK1) sending the event to the frontend means evaluating javascript in the frontend page. That would trigger an ASSERT if there was a GC HeapOperation in progress.
Comment 13 Joseph Pecoraro 2015-10-02 11:31:20 PDT
Created attachment 262344 [details]
[PATCH] For Bots
Comment 14 WebKit Commit Bot 2015-10-02 13:41:15 PDT
Comment on attachment 262344 [details]
[PATCH] For Bots

Clearing flags on attachment: 262344

Committed r190520: <http://trac.webkit.org/changeset/190520>
Comment 15 Joseph Pecoraro 2015-10-02 15:30:20 PDT
Rolled out in <http://trac.webkit.org/changeset/190527>. A few tests crashed / asserted.
Comment 16 Joseph Pecoraro 2015-10-02 15:43:35 PDT
Failures were:
https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK1%20(Tests)/r190523%20(81)/results.html

On initial inspection they don't seem like they were related to this patch.

WebKit 1 Debug. Looks like forcing a GC via GCController.garbageCollectNow in a test is happening while in the middle of running JavaScript. This seems not directly related to this new change but would be worth fixing.

> Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
> Exception Codes:       KERN_INVALID_ADDRESS at 0x00000000bbadbeef
> Exception Note:        EXC_CORPSE_NOTIFY
> 
> VM Regions Near 0xbbadbeef:
> --> 
>     __TEXT                 00000001001a8000-00000001001f0000 [  288K] r-x/rwx SM=COW  /Volumes/VOLUME/*
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0  WTFCrash + 62 (Assertions.cpp:321)
> 1  0x100343000 + 5283368
> 2  0x100343000 + 5281771
> 3  JSC::Sweep::ReturnType JSC::MarkedSpace::forEachBlock<JSC::Sweep>(JSC::Sweep&) + 379 (MarkedAllocator.h:140)
> 4  JSC::MarkedSpace::sweep() + 41 (MarkedSpace.cpp:120)
> 5  JSC::Heap::collectAndSweep(JSC::HeapOperation) + 61 (Heap.cpp:1029)
> 6  WebCore::GCController::garbageCollectNow() + 58 (GCController.cpp:89)
> 7  collectCallback(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**) + 25 (GCController.cpp:49)
> 8  long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*) + 571 (APICallbackFunction.h:61)
> 9  JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) + 592 (LLIntSlowPaths.cpp:1090)
> 10 llint_entry + 23124
> 11 vmEntryToJavaScript + 299
> 12 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 158 (JITCode.cpp:81)
> 13 JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*) + 1839 (Interpreter.cpp:1270)
> 14 JSC::globalFuncEval(JSC::ExecState*) + 1476 (JSGlobalObjectFunctions.cpp:588)
> 15 0 + 86571225190440
> 16 llint_entry + 23024
> 17 llint_entry + 23024
> 18 llint_entry + 23024
> 19 vmEntryToJavaScript + 299
> 20 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 158 (JITCode.cpp:81)
> 21 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 447 (Interpreter.cpp:1025)
> 22 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 71 (CallData.cpp:45)
> 23 WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 96 (JSMainThreadExecState.h:56)
> 24 Deprecated::ScriptFunctionCall::call(bool&) + 409 (ScriptFunctionCall.cpp:138)
> 25 Inspector::InjectedScriptBase::callFunctionWithEvalEnabled(Deprecated::ScriptFunctionCall&, bool&) const + 237 (InjectedScriptBase.cpp:87)
> 26 Inspector::InjectedScriptBase::makeCall(Deprecated::ScriptFunctionCall&, WTF::RefPtr<Inspector::InspectorValue>*) + 83 (InjectedScriptBase.cpp:107)
> 27 Inspector::InjectedScriptBase::makeEvalCall(WTF::String&, Deprecated::ScriptFunctionCall&, WTF::RefPtr<Inspector::Protocol::Runtime::RemoteObject>*, Inspector::Protocol::OptOutput<bool>*, Inspector::Protocol::OptOutput<int>*) + 55 (InjectedScriptBase.cpp:119)
> 28 Inspector::InjectedScript::evaluate(WTF::String&, WTF::String const&, WTF::String const&, bool, bool, bool, bool, WTF::RefPtr<Inspector::Protocol::Runtime::RemoteObject>*, Inspector::Protocol::OptOutput<bool>*, Inspector::Protocol::OptOutput<int>*) + 267 (InjectedScript.cpp:69)
> 29 Inspector::InspectorRuntimeAgent::evaluate(WTF::String&, WTF::String const&, WTF::String const*, bool const*, bool const*, int const*, bool const*, bool const*, bool const*, WTF::RefPtr<Inspector::Protocol::Runtime::RemoteObject>&, Inspector::Protocol::OptOutput<bool>*, Inspector::Protocol::OptOutput<int>*) + 329 (InspectorRuntimeAgent.cpp:128)
> 30 Inspector::RuntimeBackendDispatcher::evaluate(long, WTF::RefPtr<Inspector::InspectorObject>&&) + 1385 (InspectorBackendDispatchers.cpp:4971)
> 31 Inspector::RuntimeBackendDispatcher::dispatch(long, WTF::String const&, WTF::Ref<Inspector::InspectorObject>&&) + 607 (InspectorBackendDispatchers.cpp:4897)
> ...


WebKit 1 Debug. Enabling the Debugger agent encounters a crash when trying to attach the ScriptDebugServer to a JSGlobalObject. I don't think this patch had made changes here.

> Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
> Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000012
> Exception Note:        EXC_CORPSE_NOTIFY
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0 JSC::Debugger::attach(JSC::JSGlobalObject*) + 464 (JSCellInlines.h:222)
> 1 WebCore::ScriptController::attachDebugger(JSC::Debugger*) + 118 (ScriptController.cpp:328)
> 2 WebCore::Page::setDebugger(JSC::Debugger*) + 67 (Page.cpp:1083)
> 3 WebCore::PageScriptDebugServer::addListener(Inspector::ScriptDebugListener*) + 71 (PageScriptDebugServer.cpp:70)
> 4 Inspector::InspectorDebuggerAgent::enable() + 55 (InspectorDebuggerAgent.cpp:92)
> 5 WebCore::WebDebuggerAgent::enable() + 14 (WebDebuggerAgent.cpp:44)
> 6 WebCore::PageDebuggerAgent::enable() + 14 (PageDebuggerAgent.cpp:66)
> 7 Inspector::DebuggerBackendDispatcher::enable(long, WTF::RefPtr<Inspector::InspectorObject>&&) + 76 (InspectorBackendDispatchers.cpp:2543)
> 8 Inspector::DebuggerBackendDispatcher::dispatch(long, WTF::String const&, WTF::Ref<Inspector::InspectorObject>&&) + 607 (InspectorBackendDispatchers.cpp:2527)
> 9 Inspector::BackendDispatcher::dispatch(WTF::String const&) + 2467 (Ref.h:55)


WebKit 1 Debug. This is a patch made by this patch. After observing a GC there is a crash evaluating JavaScript. Maybe we are doing work too early in Heap::didFinishCollection to be running script now.

> Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
> Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000018
> Exception Note:        EXC_CORPSE_NOTIFY
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   0x0000000101839475 JSC::MarkedBlock::FreeList JSC::MarkedBlock::specializedSweep<(JSC::MarkedBlock::BlockState)3, (JSC::MarkedBlock::SweepMode)1, true>() + 181 (MarkedBlock.cpp:95)
> 1   0x000000010183910d 0x10132f000 + 5284109
> 2   0x0000000101838bbb 0x10132f000 + 5282747
> 3   0x0000000101837e21 JSC::MarkedAllocator::allocateSlowCase(unsigned long) + 225 (MarkedAllocator.cpp:74)
> 4   0x00000001013c9c6f JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 703 (MarkedAllocator.h:94)
> 5   0x00000001036566dc WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) + 284 (ScriptController.cpp:164)
> 6   0x0000000103656919 0x102937000 + 13760793
> 7   0x0000000102ee28d4 WebCore::InspectorClient::doDispatchMessageOnFrontendPage(WebCore::Page*, WTF::String const&) + 356 (InspectorClient.cpp:52)
> 8   0x00000001016c1fcc Inspector::FrontendRouter::sendEvent(WTF::String const&) const + 44 (InspectorFrontendRouter.cpp:90)
> 9   0x00000001016b381d Inspector::HeapFrontendDispatcher::garbageCollected(WTF::RefPtr<Inspector::Protocol::Heap::GarbageCollection>) + 765 (InspectorFrontendDispatchers.cpp:518)
> 10  0x00000001016c295a Inspector::InspectorHeapAgent::didGarbageCollect(JSC::HeapOperation) + 730 (InspectorHeapAgent.cpp:122)
> 11  0x000000010165babc JSC::Heap::didFinishCollection(double) + 236 (Heap.cpp:1351)
> 12  0x000000010165b1cc JSC::Heap::collectImpl(JSC::HeapOperation, void*, void*, int (&) [37]) + 1196 (Heap.cpp:1119)
> 13  0x000000010165aced JSC::Heap::collect(JSC::HeapOperation) + 237 (Heap.cpp:1045)
> 14  0x0000000101651e28 JSC::GCActivityCallback::doWork() + 72 (GCActivityCallback.cpp:81)
> 15  0x00000001016612da JSC::HeapTimer::timerDidFire(__CFRunLoopTimer*, void*) + 186 (HeapTimer.cpp:101)
> 16  0x00007fff989f59f4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
> 17  0x00007fff989f5683 __CFRunLoopDoTimer + 1075
> 18  0x00007fff989f51da __CFRunLoopDoTimers + 298
> 19  0x00007fff989ec6d1 __CFRunLoopRun + 1841
> 20  0x00007fff989ebd38 CFRunLoopRunSpecific + 296
> 21  0x00000001011a9e63 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 2146 (DumpRenderTree.mm:2031)
> 22  0x00000001011a93ca dumpRenderTree(int, char const**) + 2928 (DumpRenderTree.mm:1288)
> 23  0x00000001011aa9da DumpRenderTreeMain(int, char const**) + 1471 (DumpRenderTree.mm:1424)
> 24  0x00007fff9a50d5ad start + 1


Tests were also seeing some funky InspectorFrontendAPI not available, and seemingly only with this patch. I don't know what could be causing that!

Will be investigating.
Comment 17 Joseph Pecoraro 2015-10-02 15:44:56 PDT
> WebKit 1 Debug. This is a patch made by this patch.

Typo: This is a *path* made by this patch.
Comment 18 Joseph Pecoraro 2015-10-02 16:01:39 PDT
Comment on attachment 262344 [details]
[PATCH] For Bots

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

> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:49
> +InspectorHeapAgent::~InspectorHeapAgent()
> +{
> +    ErrorString ignored;
> +    disable(ignored);
> +}

Ahh, this should be in willDestroyFrontendAndBackend instead of ~InspectorHeapAgent. This is likely what is causing the "InspectorFrontendAPI" not available messages if this agent was accidentally left enabled despite being closed and reopening. Which seems to be the case otherwise the ASSERT(m_enabled) below would have triggered.
Comment 19 Joseph Pecoraro 2015-10-14 18:54:10 PDT
Created attachment 263132 [details]
[PATCH] Proposed Fix

This fixes the issues above. The actual diffs are pretty small but I forgot to get a diff of just the changes so I'll call them out here.

1. InspectorHeapAgent::didGarbageCollect triggers event asynchronously (RunLoop::current().dispatch(...))

To avoid the MarkedSpace::sweep ASSERTION we dispatch the garbageCollected events asynchronously to avoid executing any JavaScript between the Garbage Collection and Sweeping phases. Sweeping (MarkedSpace.sweep) has never expected to see anything other than "Marked" MarkedBlocks during a "SweepOnly" sweep. Lets not add that possibility/capability to the sweeping and instead avoid it and run JavaScript later.

This means that garbageCollected events now appear on the frontend after the script completes execution instead of during. This makes the UI a little more consistent during very long running scripts, but can be seen as a bit of a downside as it would be nice to inform the frontend even while scripts are running (e.g. a near infinite loop being traced).

The Heap.gc test needed to be rewritten to allow for gcs between the "HeapAgent.gc()" call and receiving the triggered "Full" Heap.garbageCollected event because this is no longer guaranteed to be synchronous.

2. Disable the InspectorHeapAgent in willDestroyFrontendAndBackend instead of ~InspectorHeapAgent as mentioned before.
Comment 20 BJ Burg 2015-10-15 10:25:04 PDT
Comment on attachment 263132 [details]
[PATCH] Proposed Fix

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

r=me, as second reviewer. Please address nits and stick around for bots when you land this.

> LayoutTests/inspector/heap/garbageCollected.html:38
> +        description: "Evaluating allocation heavy JavaScript should trigger Heap.garbageCollected events.",

'allocation-heavy'

> LayoutTests/inspector/heap/garbageCollected.html:54
> +                    InspectorTest.log("PASS: Should see both a Partial and Full Garbage Collection.");

How deterministic is this? I can imagine this breaking whenever the GC heuristics change.

> LayoutTests/inspector/heap/gc.html:10
> +    InspectorTest.dumpActivityToSystemConsole = true;

Oops, I think this should be removed, right?

> LayoutTests/inspector/heap/gc.html:17
> +            HeapAgent.gc();

have you considered HeapAgent.collectGarbage() ? Do we want to expose full vs partial? Which one does this trigger? Best to document it in the protocol file.

> Source/JavaScriptCore/heap/HeapObserver.h:-26
> -WebInspector.SourceCodeTimelineTreeElement = class SourceCodeTimelineTreeElement extends WebInspector.TimelineRecordTreeElement

This is a very unexpected diff. What happened?

> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:117
> +    double endTime = m_environment.executionStopwatch()->elapsedTime();

Yay.

> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:127
> +        auto collection = Inspector::Protocol::Heap::GarbageCollection::create()

Should we be using the weakThis pattern here? What if the inspector tears down before this is dispatched?

> Source/JavaScriptCore/inspector/protocol/Heap.json:27
> +            "description": "Trigger a full garbage collection."

Oh I see, it is documented here. Still, the name is rather terse (if traditional).

> Source/WebInspectorUI/ChangeLog:9
> +        (Array.prototype.parition):

'partition'

> Source/WebInspectorUI/ChangeLog:45
> +        The GCs bar will be on top of the the Scripts bar. This is particularly useful

'will be on top of'

do you mean overlaid?

> Source/WebInspectorUI/UserInterface/Controllers/HeapManager.js:-26
> -WebInspector.SourceCodeTimelineTreeElement = class SourceCodeTimelineTreeElement extends WebInspector.TimelineRecordTreeElement

I guess your version control thought this was a copy instead of new file. I blame the copyright blocks.

> Source/WebInspectorUI/UserInterface/Models/ScriptTimelineRecord.js:170
> +WebInspector.ScriptTimelineRecord.EventType.displayName = function(eventType, details, includeDetailsInformationInMainTitle)

'includeDetailsInMainTitle' ?  (details are information)

> Source/WebInspectorUI/UserInterface/Views/ScriptTimelineOverviewGraph.js:77
> +        let [gcRecords, nonGCRecords] = this._scriptTimeline.records.partition((x) => x.isGarbageCollection());

beautiful!

> Source/WebInspectorUI/UserInterface/Views/ScriptTimelineView.js:228
> +        var startTime = this.startTime;

'let' ?
Comment 21 Joseph Pecoraro 2015-10-15 11:12:30 PDT
Comment on attachment 263132 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/heap/garbageCollected.html:54
>> +                    InspectorTest.log("PASS: Should see both a Partial and Full Garbage Collection.");
> 
> How deterministic is this? I can imagine this breaking whenever the GC heuristics change.

Periodically trigger GCs does a lot of work repeatedly every 20ms. Yes, if GC heuristics change then this could fail. I think it would be good to have a test for this though, as it could actually catch a GC regression if a test like this changes behavior unexpectedly.

>> LayoutTests/inspector/heap/gc.html:10
>> +    InspectorTest.dumpActivityToSystemConsole = true;
> 
> Oops, I think this should be removed, right?

Oops, yes.

>> Source/JavaScriptCore/heap/HeapObserver.h:-26
>> -WebInspector.SourceCodeTimelineTreeElement = class SourceCodeTimelineTreeElement extends WebInspector.TimelineRecordTreeElement
> 
> This is a very unexpected diff. What happened?

Git thinks a lot of these files were copied from other files...

>> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:127
>> +        auto collection = Inspector::Protocol::Heap::GarbageCollection::create()
> 
> Should we be using the weakThis pattern here? What if the inspector tears down before this is dispatched?

I considered this too unlikely. The RunLoop dispatching looks like it would happen (on all ports) at the end of the current run loop before the next run loop, which I thought would be too early for some kind of teardown. That said, it wouldn't hurt to be safe. I didn't know about the weakThis (createWeakPtr) pattern used elsewhere and that certainly looks simple enough to take advantage of here.

>> Source/WebInspectorUI/ChangeLog:45
>> +        The GCs bar will be on top of the the Scripts bar. This is particularly useful
> 
> 'will be on top of'
> 
> do you mean overlaid?

Yes, I'll reword.
Comment 22 Joseph Pecoraro 2015-10-15 11:46:45 PDT
Created attachment 263175 [details]
[PATCH] For Landing
Comment 23 WebKit Commit Bot 2015-10-15 11:48:19 PDT
Comment on attachment 263175 [details]
[PATCH] For Landing

Rejecting attachment 263175 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 263175, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
le without the binary data in line: "Binary files a/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js and b/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js differ".  Be sure to use the --binary flag when invoking "git diff" with diffs containing binary files. at /Volumes/Data/EWS/WebKit/Tools/Scripts/VCSUtils.pm line 804, <ARGV> line 1162.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 25 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/292205
Comment 24 Joseph Pecoraro 2015-10-15 20:25:39 PDT
WeakPtrFactory doesn't seem to like the multi-threaded nature of JSContext's.
<https://build.webkit.org/builders/Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/10041/steps/webkit-jsc-cloop-test/logs/stdio>

> ASSERTION FAILED: m_boundThread == currentThread()
> /Volumes/Data/slave/yosemite-cloop-debug/build/WebKitBuild/Debug/usr/local/include/wtf/WeakPtr.h(65) : void WTF::WeakReference<Inspector::InspectorHeapAgent>::clear() [T = Inspector::InspectorHeapAgent]
> 1   0x107ef1c10 WTFCrash
> 2   0x107a861ae WTF::WeakReference<Inspector::InspectorHeapAgent>::clear()
> 3   0x107a860f1 WTF::WeakPtrFactory<Inspector::InspectorHeapAgent>::~WeakPtrFactory()
> 4   0x107a85515 WTF::WeakPtrFactory<Inspector::InspectorHeapAgent>::~WeakPtrFactory()
> 5   0x107a831b4 Inspector::InspectorHeapAgent::~InspectorHeapAgent()
...

So I dropped the WeakPtr approach. in a follow-up.
Comment 25 Joseph Pecoraro 2015-10-15 20:27:12 PDT
<http://trac.webkit.org/changeset/191159> - Relanded
<http://trac.webkit.org/changeset/191161> - Removed WeakPtr approach (ASSERTs)
<http://trac.webkit.org/changeset/191162> - Build Fix after dropping WeakPtr
<http://trac.webkit.org/changeset/191164> - EFL Build Fix 1
<http://trac.webkit.org/changeset/191165> - EFL Build Fix 2

Woohoo! First Release bot has succeeded without inspector/heap test issues. I'll wait a bit for Debug bots.
Comment 26 Joseph Pecoraro 2015-10-15 20:55:36 PDT
<http://trac.webkit.org/changeset/191166> - Made inspector/heap/gc.html more reliable