WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
165633
Web Inspector: Need some limit on Async Call Stacks for async loops (rAF loops)
https://bugs.webkit.org/show_bug.cgi?id=165633
Summary
Web Inspector: Need some limit on Async Call Stacks for async loops (rAF loops)
Joseph Pecoraro
Reported
2016-12-08 15:53:40 PST
Created
attachment 296579
[details]
[IMAGE] Large Async Call Stacks Due to Async Loop Summary: Need some limit on Async Call Stacks for async loops (rAF loops) For rAF loops the Async Call Stacks get too large. This produces a difficult to use / understand UI. Steps to Reproduce: 1. Inspect
http://tesseract.projectnaptha.com
2. Wait a few seconds 3. Click Debugger Pause button => Main Thread has a very very large call stack with rAFs
Attachments
[IMAGE] Large Async Call Stacks Due to Async Loop
(294.18 KB, image/png)
2016-12-08 15:53 PST
,
Joseph Pecoraro
no flags
Details
[Image] Proposed UI
(252.48 KB, image/png)
2017-01-10 16:50 PST
,
Matt Baker
no flags
Details
Patch
(35.85 KB, patch)
2017-01-25 22:34 PST
,
Matt Baker
no flags
Details
Formatted Diff
Diff
Patch
(90.99 KB, patch)
2017-01-27 15:28 PST
,
Matt Baker
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews114 for mac-elcapitan
(1.69 MB, application/zip)
2017-01-27 16:42 PST
,
Build Bot
no flags
Details
Patch
(91.22 KB, patch)
2017-01-27 17:18 PST
,
Matt Baker
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews113 for mac-elcapitan
(1.68 MB, application/zip)
2017-01-27 18:34 PST
,
Build Bot
no flags
Details
Patch
(90.67 KB, patch)
2017-01-27 22:56 PST
,
Matt Baker
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews116 for mac-elcapitan
(1.68 MB, application/zip)
2017-01-28 00:11 PST
,
Build Bot
no flags
Details
Patch for landing
(91.09 KB, patch)
2017-01-28 13:01 PST
,
Matt Baker
no flags
Details
Formatted Diff
Diff
Patch for landing
(91.04 KB, patch)
2017-01-28 13:02 PST
,
Matt Baker
no flags
Details
Formatted Diff
Diff
Patch for landing
(90.78 KB, patch)
2017-01-30 13:22 PST
,
Matt Baker
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2016-12-19 11:41:28 PST
<
rdar://problem/29738502
>
Matt Baker
Comment 2
2017-01-10 16:50:04 PST
Created
attachment 298525
[details]
[Image] Proposed UI The UI should have some kind of indication that the call stack has been truncated. Xcode indicates the presence of elided call frames with a dashed separator (and a gap in the call frame indices). The scenario for the Inspector is somewhat different, but we can take a cue from Xcode's UI. The attached screenshot shows a truncated call stack*, and two proposals for indicating that frames have been dropped. * Stack trace limit was set to a small number to get the desired output
Matt Baker
Comment 3
2017-01-10 16:51:22 PST
(In reply to
comment #2
)
> The scenario for the Inspector is somewhat different, but we can take a cue > from Xcode's UI. The attached screenshot shows a truncated call stack*, and > two proposals for indicating that frames have been dropped.
I'm leaning toward the dimmed function icon.
Joseph Pecoraro
Comment 4
2017-01-10 17:25:35 PST
(In reply to
comment #3
)
> (In reply to
comment #2
) > > The scenario for the Inspector is somewhat different, but we can take a cue > > from Xcode's UI. The attached screenshot shows a truncated call stack*, and > > two proposals for indicating that frames have been dropped. > > I'm leaning toward the dimmed function icon.
Doesn't that already mean something for Tail Deleted Frames?
Matt Baker
Comment 5
2017-01-10 18:32:20 PST
(In reply to
comment #4
)
> (In reply to
comment #3
) > > (In reply to
comment #2
) > > > The scenario for the Inspector is somewhat different, but we can take a cue > > > from Xcode's UI. The attached screenshot shows a truncated call stack*, and > > > two proposals for indicating that frames have been dropped. > > > > I'm leaning toward the dimmed function icon. > > Doesn't that already mean something for Tail Deleted Frames?
Tail deleted frames use a gray function icon (TailDeletedFunction.svg).
Blaze Burg
Comment 6
2017-01-17 10:25:18 PST
It looks strange that the async horizontal rule goes all the way across but the elided frames rule is indented. Can you make it go the full width too? How do either of the rules look when we have worker contexts in the sidebar? I would prefer the UIString "(Earlier frames unavailable)" or something. 'Removed' seems like the wrong word. Truncated or Elided might be okay.
Matt Baker
Comment 7
2017-01-25 22:34:32 PST
Created
attachment 299794
[details]
Patch
Joseph Pecoraro
Comment 8
2017-01-26 00:51:36 PST
Comment on
attachment 299794
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=299794&action=review
> Source/JavaScriptCore/ChangeLog:30 > + A node is either `pending`, `active`, `dispatched`, or `canceled`, and > + all nodes start out as pending. After a callback for a pending node is
Split into two sentences at the "and".
> Source/JavaScriptCore/ChangeLog:32 > + (e.g. setInterval), in which case it remains pending. Once a nodes is no
Typo: "a nodes" => "a node"
> Source/JavaScriptCore/ChangeLog:62 > + * JavaScriptCore.xcodeproj/project.pbxproj: > + * inspector/AsyncStackTrace.cpp: Added.
The new file needs to be added to CMakeLists.txt
> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:8811 > 0F0B839D14BCF46600885B4F /* LLIntThunks.h in Headers */, > + 6A38CFAA1E32B5AB0060206F /* AsyncStackTrace.h in Headers */, > 142E3139134FF0A600AFADB5 /* Local.h in Headers */,
You should probably run: ./Tools/Scripts/sort-Xcode-project-file Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj We should all get in the habit of doing that more often.
> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:47 > +}
We should add a destructor with: ASSERT(!m_childCount)
> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:69 > + ASSERT(m_state != State::Dispatched);
I think a better ASSERT would be ASSERT(Active || Canceled). I don't think we can be Pending or Dispatched here.
> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:74 > + if (!m_singleShot) { > + m_state = State::Pending; > + return; > + }
This should not reset to Pending if the current state is Canceled.
> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:84 > + ASSERT(m_state != State::Canceled);
I'm not sure you can assert this. What happens in this test case: let id = setTimeout(() => { clearTimeout(id); clearTimeout(id); }); I think we would get here twice. The first will cancel and the second will get here when cancelled.
> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:161 > + if (!parentNode->m_parent) {
You don't need to clone up the entire tree here. You only need to clone up until maxDepth, this may be cloning more than is necessary.
> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:166 > + currentNode->m_parent = AsyncStackTrace::create(parentNode->m_callStack, true, parentNode->m_parent);
We should be decrementing the m_childCount of the first locked ancestor because we are creating a parallel tree. I know I can come up with a bad scenario if you want me to.
> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:182 > + m_parent = nullptr;
Am I understanding this correctly: as soon as this happens an entire line of parents with 1 ref would reach 0 refs and be destructed? So in the loop above, m_childCount accounting may make a bunch of nodes get to zero, and this line causes a chain dealloc of them all?
> Source/JavaScriptCore/inspector/AsyncStackTrace.h:61 > + bool isTruncated() const;
This has no implementation, you can remove.
> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:265 > + if (m_currentAsyncCallIdentifier && m_currentAsyncCallIdentifier.value() == identifier) > + return;
So if I understand this correctly. - setTimeout, cancelTimeout => not current will remove - setTimeout, willDispatch, cancelTimeout => current will not remove => didDispatch => remove - setInterval, cancelInterval => not active will remove - setInterval, willDispatch, didDispatch => not single shot so will not remove - setInterval, willDispatch, cancelInterval => current will not remove => didDispatch => should remove (BUG) The last case has an issue: - setInterval - Creates AsyncStackTrace (Interval, 1234) => Pending - willDispatch - willDispatchAsyncCall Updates AsyncStackTrace (Interval, 1234) => Active - Current (Interval, 1234) - cancelInterval - didCancelAsyncCall Updates AsyncStackTrace (Interval, 1234) => Canceled - Not removed since it is current - didDispatch - didDispatchAsyncCall Updates AsyncStackTrace (Interval, 1234) => Pending - Not removed since it is pending, but it should have been removed Looks like didDispatchAsyncCall should NOT reset the state from Canceled to Pending. That would leak until page navigation or inspector closes.
> Source/JavaScriptCore/inspector/protocol/Console.json:43 > + { "name": "truncated", "type": "boolean", "optional": true, "description": "Whether one or more frames have been truncated from the bottom of the stack." },
r- because there needs to be a test for protocol changes. Also, I think you will also need to include this in: Source/WebInspectorUI/Versions/Inspector-iOS-10.3.json And rebuild the backend commands for it: Source/WebInspectorUI/Scripts/update-LegacyInspectorBackendCommands.rb
> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.css:56 > + border-top: dashed 1px var(--border-color);
I think this should be 0.5px (hairline) for Retina. I believe that is our usual style.
> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.css:64 > + margin-left: 0;
I'm surprised this would be needed.
Matt Baker
Comment 9
2017-01-26 14:34:40 PST
Comment on
attachment 299794
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=299794&action=review
>> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:161 >> + if (!parentNode->m_parent) { > > You don't need to clone up the entire tree here. You only need to clone up until maxDepth, this may be cloning more than is necessary.
Since lastUnlockedAncestor != newStackTraceRoot we should break out once the next parent is the new root. Setting the truncate flag, etc, can be done outside the loop.
>> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:182 >> + m_parent = nullptr; > > Am I understanding this correctly: as soon as this happens an entire line of parents with 1 ref would reach 0 refs and be destructed? So in the loop above, m_childCount accounting may make a bunch of nodes get to zero, and this line causes a chain dealloc of them all?
Correct! Child counts must be updated by hand, but branches without a pending descendent clean up after themselves.
>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:265 >> + return; > > So if I understand this correctly. > > - setTimeout, cancelTimeout => not current will remove > - setTimeout, willDispatch, cancelTimeout => current will not remove => didDispatch => remove > - setInterval, cancelInterval => not active will remove > - setInterval, willDispatch, didDispatch => not single shot so will not remove > - setInterval, willDispatch, cancelInterval => current will not remove => didDispatch => should remove (BUG) > > The last case has an issue: > > - setInterval > - Creates AsyncStackTrace (Interval, 1234) => Pending > - willDispatch > - willDispatchAsyncCall Updates AsyncStackTrace (Interval, 1234) => Active > - Current (Interval, 1234) > - cancelInterval > - didCancelAsyncCall Updates AsyncStackTrace (Interval, 1234) => Canceled > - Not removed since it is current > - didDispatch > - didDispatchAsyncCall Updates AsyncStackTrace (Interval, 1234) => Pending > - Not removed since it is pending, but it should have been removed > > Looks like didDispatchAsyncCall should NOT reset the state from Canceled to Pending. That would leak until page navigation or inspector closes.
Great catch!
>> Source/JavaScriptCore/inspector/protocol/Console.json:43 >> + { "name": "truncated", "type": "boolean", "optional": true, "description": "Whether one or more frames have been truncated from the bottom of the stack." }, > > r- because there needs to be a test for protocol changes. > > Also, I think you will also need to include this in: > > Source/WebInspectorUI/Versions/Inspector-iOS-10.3.json > > And rebuild the backend commands for it: > > Source/WebInspectorUI/Scripts/update-LegacyInspectorBackendCommands.rb
The only protocol change was the addition of the truncated property to Console.StackTrace, which doesn't impact InspectorBackendCommands.
Joseph Pecoraro
Comment 10
2017-01-26 19:43:20 PST
Comment on
attachment 299794
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=299794&action=review
>>> Source/JavaScriptCore/inspector/protocol/Console.json:43 >>> + { "name": "truncated", "type": "boolean", "optional": true, "description": "Whether one or more frames have been truncated from the bottom of the stack." }, >> >> r- because there needs to be a test for protocol changes. >> >> Also, I think you will also need to include this in: >> >> Source/WebInspectorUI/Versions/Inspector-iOS-10.3.json >> >> And rebuild the backend commands for it: >> >> Source/WebInspectorUI/Scripts/update-LegacyInspectorBackendCommands.rb > > The only protocol change was the addition of the truncated property to Console.StackTrace, which doesn't impact InspectorBackendCommands.
Any change to a protocol json file modifies an InspectorBackendCommands. Your change here modifies the generated InspectorBackendCommands, which gets copied into UserInterface at build time. I'm saying that you will need to edit the Inspector-iOS-10.3.json and when you do you will need to update its generated UserInterface/Protocol/Legacy/10.3/InspectorBackendCommands.js. The above script will do that.
Matt Baker
Comment 11
2017-01-27 15:28:12 PST
Created
attachment 299968
[details]
Patch
Build Bot
Comment 12
2017-01-27 16:42:47 PST
Comment on
attachment 299968
[details]
Patch
Attachment 299968
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/2960532
New failing tests: inspector/debugger/breakpoint-action-eval.html
Build Bot
Comment 13
2017-01-27 16:42:50 PST
Created
attachment 299979
[details]
Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Matt Baker
Comment 14
2017-01-27 16:46:30 PST
Fixing! Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010db35227 WTFCrash + 39 (Assertions.cpp:323) 1 com.apple.JavaScriptCore 0x000000010caffd09 Inspector::AsyncStackTrace::~AsyncStackTrace() + 73 (AsyncStackTrace.cpp:51) 2 com.apple.JavaScriptCore 0x000000010caffd65 Inspector::AsyncStackTrace::~AsyncStackTrace() + 21 (AsyncStackTrace.cpp:52) 3 com.apple.JavaScriptCore 0x000000010cb043d9 WTF::RefCounted<Inspector::AsyncStackTrace>::deref() const + 73 (RefCounted.h:145) 4 com.apple.JavaScriptCore 0x000000010cb04384 void WTF::derefIfNotNull<Inspector::AsyncStackTrace>(Inspector::AsyncStackTrace*) + 52 (PassRefPtr.h:41) ...
Matt Baker
Comment 15
2017-01-27 17:01:14 PST
(In reply to
comment #14
)
> Fixing! > > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread > 0 com.apple.JavaScriptCore 0x000000010db35227 WTFCrash + 39 > (Assertions.cpp:323) > 1 com.apple.JavaScriptCore 0x000000010caffd09 > Inspector::AsyncStackTrace::~AsyncStackTrace() + 73 (AsyncStackTrace.cpp:51) > 2 com.apple.JavaScriptCore 0x000000010caffd65 > Inspector::AsyncStackTrace::~AsyncStackTrace() + 21 (AsyncStackTrace.cpp:52) > 3 com.apple.JavaScriptCore 0x000000010cb043d9 > WTF::RefCounted<Inspector::AsyncStackTrace>::deref() const + 73 > (RefCounted.h:145) > 4 com.apple.JavaScriptCore 0x000000010cb04384 void > WTF::derefIfNotNull<Inspector::AsyncStackTrace>(Inspector::AsyncStackTrace*) > + 52 (PassRefPtr.h:41) > ...
It seems like this could be the result of InspectorDebuggerAgent tearing down a non-empty pending stack traces map.
Matt Baker
Comment 16
2017-01-27 17:18:29 PST
Created
attachment 299983
[details]
Patch
Build Bot
Comment 17
2017-01-27 18:34:03 PST
Comment on
attachment 299983
[details]
Patch
Attachment 299983
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/2961036
New failing tests: inspector/debugger/breakpoint-action-eval.html
Build Bot
Comment 18
2017-01-27 18:34:06 PST
Created
attachment 299989
[details]
Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Matt Baker
Comment 19
2017-01-27 19:04:30 PST
(In reply to
comment #18
)
> Created
attachment 299989
[details]
> Archive of layout-test-results from ews113 for mac-elcapitan > > The attached test failures were seen while running run-webkit-tests on the > mac-debug-ews. > Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
🤔
Matt Baker
Comment 20
2017-01-27 22:56:48 PST
Created
attachment 299999
[details]
Patch
Build Bot
Comment 21
2017-01-28 00:11:42 PST
Comment on
attachment 299999
[details]
Patch
Attachment 299999
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/2962295
New failing tests: inspector/debugger/breakpoint-action-eval.html
Build Bot
Comment 22
2017-01-28 00:11:45 PST
Created
attachment 300006
[details]
Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Joseph Pecoraro
Comment 23
2017-01-28 01:18:37 PST
Comment on
attachment 299999
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=299999&action=review
r=me
> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:177 > + // Decrement the child count of the first locked ancestor now that itafter removing its subtree.
Typo: "now that itafter".
> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1144 > + for (auto& entry : m_pendingAsyncCalls) { > + auto& stackTrace = entry.value; > + if (stackTrace->isPending()) > + stackTrace->didCancelAsyncCall(); > + }
I feel like this is unnecessary. With m_pendingAsyncCalls.clear() all of the "leaf" nodes will be dereferenced, causing all of their chains to be dereferenced. Why do we care to cancel things? (I could see maybe doing this if there were some Assertions in an ~AsyncStackTrace but that doesn't exist).
> LayoutTests/inspector/debugger/async-stack-trace-expected.txt:82 > +Set Debugger.asyncStackTraceDepth equal to 4 > +PASS: Non-root StackTrace should not be truncated. > +PASS: Non-root StackTrace should not be truncated. > +PASS: StackTrace root should be truncated. > +PASS: StackTrace should be truncated to the nearest call stack. > +0: [F] pauseThenFinishTest > +1: [F] repeat > +2: --- requestAnimationFrame --- > +3: [F] repeat > +4: --- requestAnimationFrame --- > +5: [F] repeat > +6: --- truncated ---
Interesting. This feels off-by-one, but I don't mind that much. The depth is 4, but we have (1) pauseThenFinishTest, (2) repeat, (3) rAF, (4) repeat, (5) rAF, (6) repeat, (7) truncated. Seems we could have stopped here at frame (5).
Joseph Pecoraro
Comment 24
2017-01-28 01:24:05 PST
Comment on
attachment 299999
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=299999&action=review
> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:52 > +AsyncStackTrace::~AsyncStackTrace() > +{ > + ASSERT(!m_childCount); > +}
Wow I totally missed the constructor despite looking for it... Yes this ASSERT is triggering because of: 18 com.apple.JavaScriptCore 0x0000000105ad54de Inspector::InspectorDebuggerAgent::clearAsyncStackTraceData() + 270 (InspectorDebuggerAgent.cpp:1147) And that makes sense, we are deferring the leaf nodes in a random order, and we are not removing them. Which is fine because we really do just want to release all the data and we don't need to update the internal numbers. But if you want to keep debug code, you could probably do: #ifndef NDEBUG if (m_parent) remove(); ASSERT(!m_childCount); #endif Which I think may work and could catch other accounting issues. But it might not be worth it. What do you think?
Matt Baker
Comment 25
2017-01-28 12:20:01 PST
Comment on
attachment 299999
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=299999&action=review
>> LayoutTests/inspector/debugger/async-stack-trace-expected.txt:82 >> +6: --- truncated --- > > Interesting. This feels off-by-one, but I don't mind that much. The depth is 4, but we have (1) pauseThenFinishTest, (2) repeat, (3) rAF, (4) repeat, (5) rAF, (6) repeat, (7) truncated. Seems we could have stopped here at frame (5).
1) The first two frames belong to the non-asynchronous (live) call stack, and don't count toward the async stack depth budget. 2) Frame 6 is just an indicated that the trace was truncated, not a frame. I'll improve the call stack logging to make this clear.
Matt Baker
Comment 26
2017-01-28 12:25:57 PST
Comment on
attachment 299999
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=299999&action=review
>> Source/JavaScriptCore/inspector/AsyncStackTrace.cpp:52 >> +} > > Wow I totally missed the constructor despite looking for it... Yes this ASSERT is triggering because of: > > 18 com.apple.JavaScriptCore 0x0000000105ad54de Inspector::InspectorDebuggerAgent::clearAsyncStackTraceData() + 270 (InspectorDebuggerAgent.cpp:1147) > > And that makes sense, we are deferring the leaf nodes in a random order, and we are not removing them. Which is fine because we really do just want to release all the data and we don't need to update the internal numbers. > > But if you want to keep debug code, you could probably do: > > #ifndef NDEBUG > if (m_parent) > remove(); > ASSERT(!m_childCount); > #endif > > Which I think may work and could catch other accounting issues. But it might not be worth it. > > What do you think?
I think the advantage of having InspectorDebuggerAgent::clearAsyncStackTraceData do the remove (via didCancelAsyncStackTrace) and leaving the assert in place is that we then catch any situations where nodes are erroneously left in the tree, or the tree structure is in a bad state somehow.
Matt Baker
Comment 27
2017-01-28 12:30:25 PST
After a little more thought I think this is fine. We don't have any context for why the trace is being destroyed in this case, but the tree structure is still sanity checked by the ASSERT which is seems sufficient.
Matt Baker
Comment 28
2017-01-28 13:01:02 PST
Created
attachment 300049
[details]
Patch for landing
Matt Baker
Comment 29
2017-01-28 13:02:34 PST
Created
attachment 300051
[details]
Patch for landing
WebKit Commit Bot
Comment 30
2017-01-28 17:03:44 PST
Comment on
attachment 300051
[details]
Patch for landing Clearing flags on attachment: 300051 Committed
r211345
: <
http://trac.webkit.org/changeset/211345
>
WebKit Commit Bot
Comment 31
2017-01-28 17:03:49 PST
All reviewed patches have been landed. Closing bug.
Ryan Haddad
Comment 32
2017-01-30 10:07:58 PST
The LayoutTest edited with this change is failing an assertion: inspector/debugger/async-stack-trace.html
https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r211365%20(10686)/results.html
ASSERTION FAILED: !m_childCount /Volumes/Data/slave/elcapitan-debug/build/Source/JavaScriptCore/inspector/AsyncStackTrace.cpp(53) : Inspector::AsyncStackTrace::~AsyncStackTrace() 1 0x114093410 WTFCrash 2 0x113059724 Inspector::AsyncStackTrace::~AsyncStackTrace() 3 0x113059835 Inspector::AsyncStackTrace::~AsyncStackTrace() 4 0x11305ddd9 WTF::RefCounted<Inspector::AsyncStackTrace>::deref() const 5 0x11305dd84 void WTF::derefIfNotNull<Inspector::AsyncStackTrace>(Inspector::AsyncStackTrace*) 6 0x11305a78b WTF::RefPtr<Inspector::AsyncStackTrace>::operator=(std::nullptr_t) 7 0x113059815 Inspector::AsyncStackTrace::remove() 8 0x1130596ea Inspector::AsyncStackTrace::~AsyncStackTrace() 9 0x113059835 Inspector::AsyncStackTrace::~AsyncStackTrace() 10 0x11305ddd9 WTF::RefCounted<Inspector::AsyncStackTrace>::deref() const 11 0x11305dd84 void WTF::derefIfNotNull<Inspector::AsyncStackTrace>(Inspector::AsyncStackTrace*) 12 0x11305a78b WTF::RefPtr<Inspector::AsyncStackTrace>::operator=(std::nullptr_t) 13 0x113059815 Inspector::AsyncStackTrace::remove() 14 0x113059c2f Inspector::AsyncStackTrace::didDispatchAsyncCall() 15 0x1139b9f72 Inspector::InspectorDebuggerAgent::didDispatchAsyncCall() 16 0x1179d2fc8 WebCore::InspectorInstrumentation::didFireAnimationFrameImpl(WebCore::InspectorInstrumentationCookie const&) 17 0x118e6c9f8 WebCore::InspectorInstrumentation::didFireAnimationFrame(WebCore::InspectorInstrumentationCookie const&) 18 0x118e6c22f WebCore::ScriptedAnimationController::serviceScriptedAnimations(double) 19 0x118e6c361 WebCore::ScriptedAnimationController::displayRefreshFired() 20 0x117088a84 WebCore::DisplayRefreshMonitorClient::fireDisplayRefreshIfNeeded() 21 0x1170853f9 WebCore::DisplayRefreshMonitor::displayDidRefresh() 22 0x11708531d WebCore::DisplayRefreshMonitor::handleDisplayRefreshedNotificationOnMainThread(void*) 23 0x117089573 WebCore::DisplayRefreshMonitorMac::displayLinkFired()::$_0::operator()() const 24 0x1170894ac WTF::Function<void ()>::CallableWrapper<WebCore::DisplayRefreshMonitorMac::displayLinkFired()::$_0>::call() 25 0x1140c8283 WTF::Function<void ()>::operator()() const 26 0x1140e41cb WTF::RunLoop::performWork() 27 0x1140e5a74 WTF::RunLoop::performWork(void*) 28 0x7fff982e57e1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 29 0x7fff982c4f1c __CFRunLoopDoSources0 30 0x7fff982c443f __CFRunLoopRun 31 0x7fff982c3e38 CFRunLoopRunSpecific
Ryan Haddad
Comment 33
2017-01-30 12:09:54 PST
Reverted
r211345
for reason: The LayoutTest for this change is failing an assertion. Committed
r211381
: <
http://trac.webkit.org/changeset/211381
>
Matt Baker
Comment 34
2017-01-30 12:19:50 PST
Fixing.
Matt Baker
Comment 35
2017-01-30 13:22:16 PST
Created
attachment 300137
[details]
Patch for landing
WebKit Commit Bot
Comment 36
2017-01-30 14:02:30 PST
Comment on
attachment 300137
[details]
Patch for landing Clearing flags on attachment: 300137 Committed
r211385
: <
http://trac.webkit.org/changeset/211385
>
WebKit Commit Bot
Comment 37
2017-01-30 14:02:37 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug