Bug 30659 – Web Inspector: Leftover Breakpoints in the Sidebar Pane

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Created attachment 41631 [details]
[IMAGE] Orphaned Breakpoints in Sidebar

I came across this situation where a Breakpoint shows up in the Sidebar, but there is no Tag in the UI.  There appears to be no way to remove this sidebar entry.  It is reproducible, and thus you can have many garbage breakpoints.  I don't know if its just the UI or if there are really orphaned breakpoints somewhere else.

Steps to Reproduce:
1. Open the inspector on a page
2. Add a breakpoint to a line (it can be in either active/disabled states)
3. Refresh the page in the background
4. Go back to the Inspector
5. Remove the breakpoint by clicking the tag enough times

Current Behavior:
The sidebar entry remains but is in an unusable state.  Checking/Unchecking does nothing. It does not jump to the source code when clicked.

Expected Behavior:
When the breakpoint was removed the sidebar entry should have been removed. (In Step 5)

[Joseph Pecoraro]

The Breakpoints Sidebar was added in:
http://trac.webkit.org/changeset/46402

[Patrick Mueller]

I have a fix for this that I haven't fully investigated yet.  In ScriptsPanel.addScript(), I changed the logic to not just update the sourceID for existing breakpoints, but to delete the old ones, and add new ones.  Code now looks like this:

        if (sourceURL in this._breakpointsURLMap && sourceID) {
            var breakpoints = this._breakpointsURLMap[sourceURL];
            var breakpointsLength = breakpoints.length;
            for (var i = 0; i < breakpointsLength; ++i) {
                var breakpoint = breakpoints[i];
                var enabled = breakpoint.enabled;
                this.removeBreakpoint(breakpoint);
                
                if (startingLine <= breakpoint.line) {
                    breakpoint = new WebInspector.Breakpoint(breakpoint.url, breakpoint.line, sourceID, breakpoint.condition);
                    this.addBreakpoint(breakpoint);
                    breakpoint.enabled = enabled;
                }
            }
        }

Fixes the problem for me.

Looking at the code, it appears there are bits of breakpoint goop all over the place; it would be nice to have this a little more centralized, somehow.  I would have expected most of the breakpoint logic to actually be in BreakPoint.js, but in fact that class pretty much just handles breakpoint state, and none of the interesting behavior.

[Patrick Mueller]

Created attachment 42520 [details]
proposed patch 2009/11/04 - a

The problem is that the sidebar wasn't being cleaned up - still pointed to the sourceID of the original script (sourceIDs for script change on reload).

Easiest thing to do is to call ScriptPanel's removeBreakpoint() on the old one, change the sourceID of the breakpoint to the new sourceID, then call addBreakpoint().   It's a bit overkill, as I >think< all that really needs to be done is to change the sidebar's sourceID.  But that's a bit reach-y.  And untested.

As I mentioned in a previous comment, I think a lot of this breakpoint stuff can be consolidated somehow; too much function spread over too many files.  This bug was an example of the problem of the spread.

I need to get reloading of breakpoint's in named eval()'s working (no simple fix for that).  Thinking a refactoring at that point might be appropriate.  Patching this one for now would be great because this is a bug a lot of people will hit.

[Timothy Hatcher]

Comment on attachment 42520 [details]
proposed patch 2009/11/04 - a

I agree, we should clean up this breakpoint logic and get more of it shared and in a nice state.

[WebKit Commit Bot]

Comment on attachment 42520 [details]
proposed patch 2009/11/04 - a

Clearing flags on attachment: 42520

Committed r50529: <http://trac.webkit.org/changeset/50529>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.