Bug 234575

Summary: Uncaught Exception: Cannot step over because debugger is not paused
Product: WebKit Reporter: BJ Burg <bburg>
Component: Web InspectorAssignee: Patrick Angle <pangle>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, ews-watchlist, hi, inspector-bugzilla-changes, pangle, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
URL: about:blank
Attachments:
Description Flags
Patch v1.0
none
Patch v1.1
none
Patch v1.2 none

Description BJ Burg 2021-12-21 14:11:02 PST
Uncaught Exception in Web Inspector.

Steps to Reproduce:
0. Using an engineering build, launch Safari
1. Go to about:blank
2. Use keyboard shortcuts for stepping, for example Command-'

Uncaught Exceptions:
-----------------------
 - Cannot step over because debugger is not paused. (at DebuggerManager.js:597:44)
    stepOver @ DebuggerManager.js:597:44
    ? @ Main.js:1558:32
    _handleKeyDown @ KeyboardShortcut.js:82:42
-----------------------

Notes:
Inspected URL:        about:blank
Loading completed:    true
Frontend User Agent:  Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko)
Comment 1 Radar WebKit Bug Importer 2021-12-21 14:11:53 PST
<rdar://problem/86783769>
Comment 2 Patrick Angle 2022-01-06 16:00:18 PST
Created attachment 448543 [details]
Patch v1.0
Comment 3 Devin Rousso 2022-01-06 16:09:39 PST
Comment on attachment 448543 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:1556
> +        InspectorFrontendHost.beep();

I kinda feel like instead of beeping, maybe we should just disable the keyboard shortcut when not paused.  Our documentation even says "When paused" <https://webkit.org/web-inspector/keyboard-shortcuts/#sources-tab> so I don't think it's that crazy of an idea.  I'd probably look for where `WI.DebuggerManager.Event.Paused` and `WI.DebuggerManager.Event.Resumed` are dispatched and add `WI.stepOverKeyboardShortcut.disabled = !this.paused;` for each keyboard shortcut (and their alternates).

NIT: if you do wanna keep this, I'd make this into an early return
Comment 4 Patrick Angle 2022-01-06 16:46:25 PST
Created attachment 448546 [details]
Patch v1.1
Comment 5 Devin Rousso 2022-01-06 17:51:41 PST
Comment on attachment 448546 [details]
Patch v1.1

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

r=me with one thing to fix :)

> Source/WebInspectorUI/UserInterface/Base/Main.js:1687
> +    WI.stepOverKeyboardShortcut.disabled = !WI.debuggerManager.paused;

NIT: Can we `let paused = WI.debuggerManager.paused;` to avoid the repeated logic?  Alternatively, you could make `WI._updateDebuggerKeyboardShortcuts` take a parameter `paused` and hardcode the value at each callsite (e.g. we know that the debugger is not paused inside `WI._debuggerDidResume` so there's kinda no reason to re-check `WI.debuggerManager.paused`, tho it would be a nice `console.assert` just to be sure).

> Source/WebInspectorUI/UserInterface/Base/Main.js:1690
> +    WI.pauseOrResumeAlternateKeyboardShortcut.disabled = !WI.debuggerManager.paused;

I dont think this should ever be disabled since it works both when and not paused.
Comment 6 Patrick Angle 2022-01-06 21:33:01 PST
Created attachment 448560 [details]
Patch v1.2
Comment 7 EWS 2022-01-07 12:22:10 PST
Committed r287776 (245836@main): <https://commits.webkit.org/245836@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448560 [details].