Bug 135396 – Web Inspector: breakpoints are always speculatively resolved when restored from local storage

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Brian Burg]

A long-time quirk/optimization in the frontend is that we immediately set a breakpoint as resolved if the breakpoint was successfully set in the backend. The purpose of this is to make clicking in the gutter immediately produce a resolved breakpoint, rather than waiting for several round trips (perceived as lag).

However, the logic is wrong such that any breakpoint that is set via DebuggerManager.addBreakpoint will be speculatively resolved, even if its corresponding source code has never been loaded. This can cause problems for later code that assumes a resolved breakpoint also has a valid sourceCodeLocation.sourceCode.

[Radar WebKit Bug Importer]

<rdar://problem/17848753>

[Brian Burg]

Created attachment 235707 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 235707 [details]
Patch

Clearing flags on attachment: 235707

Committed r171784: <http://trac.webkit.org/changeset/171784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 235707 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:257
> +        function speculativelyResolveBreakpoint(breakpoint) {
> +            breakpoint.resolved = true;
> +        }
> +
>          if (!breakpoint.disabled)
> -            this._setBreakpoint(breakpoint);
> +            this._setBreakpoint(breakpoint, shouldSpeculativelyResolve ? speculativelyResolveBreakpoint.bind(null, breakpoint) : null);

I think we can improve this.

The _setBreakpoint callback should be passed the breakpoint, so that we aren't creating all this bound functions.

    function speculativelyResolveBreakpoint(breakpoint) {
        breakpoint.resolved = true;
    }

    this._setBreakpoint(breakpoint, shouldSpeculativelyResolve ? speculativelyResolveBrekapoint : null);

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:548
> +            for (var location of locations)
> +                this.breakpointResolved(breakpointIdentifier, location);
>  
>              if (typeof callback === "function")
>                  callback();

And here, callback would pass the breakpoint:

    callback(breakpoint);

[Brian Burg]

(In reply to comment #5)
> (From update of attachment 235707 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235707&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:257
> > +        function speculativelyResolveBreakpoint(breakpoint) {
> > +            breakpoint.resolved = true;
> > +        }
> > +
> >          if (!breakpoint.disabled)
> > -            this._setBreakpoint(breakpoint);
> > +            this._setBreakpoint(breakpoint, shouldSpeculativelyResolve ? speculativelyResolveBreakpoint.bind(null, breakpoint) : null);
> 
> I think we can improve this.
> 
> The _setBreakpoint callback should be passed the breakpoint, so that we aren't creating all this bound functions.
> 
>     function speculativelyResolveBreakpoint(breakpoint) {
>         breakpoint.resolved = true;
>     }
> 
>     this._setBreakpoint(breakpoint, shouldSpeculativelyResolve ? speculativelyResolveBrekapoint : null);
> 
> > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:548
> > +            for (var location of locations)
> > +                this.breakpointResolved(breakpointIdentifier, location);
> >  
> >              if (typeof callback === "function")
> >                  callback();
> 
> And here, callback would pass the breakpoint:
> 
>     callback(breakpoint);

I was going to go full-blown promises here since this code doesn't handle errors well, but I'm trying to restrain myself from allocation and command optimizations until we have some useful profiling data.