Bug 135367 - Web Inspector: ProbeSetDetailsSection should not create live location links for unresolved breakpoints
Summary: Web Inspector: ProbeSetDetailsSection should not create live location links f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on: 135396
Blocks:
  Show dependency treegraph
 
Reported: 2014-07-28 16:27 PDT by Joseph Pecoraro
Modified: 2014-07-30 14:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.05 KB, patch)
2014-07-29 16:06 PDT, Brian Burg
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-07-28 16:27:56 PDT
* 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)
Comment 1 Radar WebKit Bug Importer 2014-07-28 16:28:41 PDT
<rdar://problem/17836190>
Comment 2 Brian Burg 2014-07-29 16:06:49 PDT
Created attachment 235708 [details]
Patch
Comment 3 Timothy Hatcher 2014-07-29 16:34:07 PDT
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?
Comment 4 Joseph Pecoraro 2014-07-29 21:19:51 PDT
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.
Comment 5 Brian Burg 2014-07-30 09:46:41 PDT
(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)
Comment 6 Brian Burg 2014-07-30 14:47:09 PDT
Committed r171819: <http://trac.webkit.org/changeset/171819>