Bug 135367 – Web Inspector: ProbeSetDetailsSection should not create live location links for unresolved breakpoints

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

[Joseph Pecoraro]

* SUMMARY
Opening the inspector in local builds I see a lot of asserts trying to make a display string for an non-resolved breakpoint. The Breakpoint's SourceCodeLocation will have a null sourceCode, and produces an assert.

* STEPS TO REPRODUCE
1. Set breakpoints with probes on multiple domains (e.g. bogojoker.com/shell/, nytimes)
2. Load apple.com
3. Open inspector
  => asserts

* DETAILS

console.assert(sourceCode);
[Error] Assertion failed: 
	_locationString (SourceCodeLocation.js, line 294)
	originalLocationString (SourceCodeLocation.js, line 174)
	tooltipString (SourceCodeLocation.js, line 190)
	populateLiveDisplayLocationTooltip (SourceCodeLocation.js, line 243)
	createSourceCodeLocationLink (Main.js, line 1558)
	_probeSetPositionTextOrLink (ProbeSetDetailsSection.js, line 92)
	ProbeSetDetailsSection (ProbeSetDetailsSection.js, line 50)
	_probeSetAdded (ProbeDetailsSidebarPanel.js, line 93)
	dispatch (Object.js, line 141)
	dispatchEventToListeners (Object.js, line 148)
	_probeSetForBreakpoint (ProbeManager.js, line 178)
	(anonymous function) (ProbeManager.js, line 137)
	forEach
	_breakpointActionsChanged (ProbeManager.js, line 129)
	_breakpointAdded (ProbeManager.js, line 98)
	dispatch (Object.js, line 141)
	dispatchEventToListeners (Object.js, line 148)
	addBreakpoint (DebuggerManager.js, line 256)
	restoreBreakpointsSoon (DebuggerManager.js, line 71)
	restoreBreakpointsSoon ([native code], line 0)

[Radar WebKit Bug Importer]

<rdar://problem/17836190>

[Brian Burg]

Created attachment 235708 [details]
Patch

[Timothy Hatcher]

Comment on attachment 235708 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ProbeSetDetailsSection.js:98
> +            var location = breakpoint.sourceCodeLocation;
> +            titleElement = WebInspector.linkifyLocation(breakpoint.url, location.displayLineNumber, location.displayColumnNumber);

This could use a comment. Maybe an assert that breakpoint.sourceCodeLocation.sourceCode is null?

[Joseph Pecoraro]

Comment on attachment 235708 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:9
> +        Regenerate the source code link whenever the breakpoint resolved status changes. If it is
> +        resolved, then we can create a live location link that respects source maps.

Why do we need ProbeSet* objects for every breakpoints, resolved or not?

Is it needed in order to support WebReplay recordings? If these objects only existed for resolved breakpoints, then almost all the changes in this patch could go away. That was my first take on how we would approach this.

[Brian Burg]

(In reply to comment #4)
> (From update of attachment 235708 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235708&action=review
> 
> > Source/WebInspectorUI/ChangeLog:9
> > +        Regenerate the source code link whenever the breakpoint resolved status changes. If it is
> > +        resolved, then we can create a live location link that respects source maps.
> 
> Why do we need ProbeSet* objects for every breakpoints, resolved or not?
> 
> Is it needed in order to support WebReplay recordings? If these objects only existed for resolved breakpoints, then almost all the changes in this patch could go away. That was my first take on how we would approach this.

The view objects (probe section, etc) could be lazily created. The model objects should not be lazily loaded.

It would be nice to not lose samples when the page is reloaded, just like the console. To do this, the probe set needs to stay alive across resolved-unresolved-resolved transitions when reloading. Navigating away could clear the probe samples, but the UI should be clearly delineating when the samples are from a previous load anyway.

There are replay implications. In the replay setting, we want to retain probe sets across multiple executions of the same recording so that users can fill in missing table values in future playbacks. (RemoteObjects for old executions are clearly flushed)

[Brian Burg]

Committed r171819: <http://trac.webkit.org/changeset/171819>