Summary: | Web Inspector: DebuggerManager.Event.Resumed introduces test flakiness | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bburg, commit-queue, joepeck, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, ryanhaddad, saam, timothy, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=176071 | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 163230, 164305 | ||||||
Attachments: |
|
Description
Joseph Pecoraro
2016-09-14 00:15:47 PDT
Marked tests as flaky in http://trac.webkit.org/projects/webkit/changeset/206939 One more flaky test that uses DebuggerManager.Event.Resumed https://trac.webkit.org/changeset/207313 *** Bug 164468 has been marked as a duplicate of this bug. *** Created attachment 294186 [details]
[PATCH] Proposed Fix
Comment on attachment 294186 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=294186&action=review > Source/WebCore/inspector/InspectorClient.cpp:51 > + SuspendExceptionScope scope(&frontendPage->inspectorController().vm()); @Mark: I didn't add an extra CatchScope and assertion that the inspector didn't produce an exception because it is possible that the inspector page does produce exceptions. We could add it and try to enforce that requirement, but I didn't think having an ASSERT would be any better then our normal debugging handling (UncaughtExceptionReporter + ability to resume using Web Inspector). With this change I was able to `run-webkit-tests inspector/debugger --time-out-ms=30000 --iterations=5 --force --v` on Release and Debug with no failures (other then 1 pre-existing issue). This was not possible for me before, especially with Debug builds. Comment on attachment 294186 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=294186&action=review r=me, please have mlam or someone glance over JSC parts though. > LayoutTests/ChangeLog:13 > + some tests using new style and best practices. Shouldn't we be unskipping these tests as well, so that they actually run? > LayoutTests/inspector/debugger/break-on-exception.html:56 > + expression: `setTimeout(() => { testCatch(); TestPage.dispatchEventToFrontend("AfterTestFunction")})`, Where is this used in this test? I think we are missing an event listener. > LayoutTests/inspector/debugger/command-line-api-exception-nested-catch.html:78 > + InspectorTest.singleFireEventListener("AfterTestFunction", resolve); ..I see here in this other test we have a listener for it. > LayoutTests/inspector/debugger/csp-exceptions.html:31 > + WebInspector.debuggerManager.resume().then(resolve, reject); Nice. > LayoutTests/inspector/debugger/pause-reason.html:41 > + if (setup) Please propagate 'setup' to the 'setup' parameter, right between 'description' and 'test'. > LayoutTests/inspector/debugger/paused-scopes.html:91 > + WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => { Does this need to unpause, or do we do that automatically? EDIT: oh, the next test assumes it's already paused. I don't like this, but it's hard to do better. > Source/JavaScriptCore/ChangeLog:8 > + Might want to put a few sentences here from the bug to describe problem & solution. > Source/JavaScriptCore/ChangeLog:34 > + All places that do continueProgram should be audited. In error cases, Maybe group this comment also with the methods above where it applies, i.e., continueToLocation. > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:144 > + enum class ShouldDispatchResumed { No, WhenIdle, WhenContinued }; This read a bit awkwardly to be, but I don't have suggestions :| Maybe the member should be m_conditionToDispatchResumed or something. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:-678 > - this._debuggerStepOutButtonItem.enabled = indexOfActiveCallFrame < callFrames.length - 1; I see this FIXME has annoyed you enough times to go ahead and remove it :P Comment on attachment 294186 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=294186&action=review LGTM. > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:520 > + m_frontendDispatcher->resumed(); I think this deserves a comment to explain why you're treating this like a resume instead of a big step. I don't think it is obvious from just reading this code. Comment on attachment 294186 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=294186&action=review >> Source/JavaScriptCore/ChangeLog:8 >> + > > Might want to put a few sentences here from the bug to describe problem & solution. Extra newline. > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:529 > + m_scriptDebugServer.continueProgram(); Nit: extra whitespace at end of this line Comment on attachment 294186 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=294186&action=review > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:948 > + m_frontendDispatcher->resumed(); Nit: extra whitespace at end of line *** Bug 145626 has been marked as a duplicate of this bug. *** |