Bug 48858 – Web Inspector: native breakpoints aren't hit on navigation

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

[Pavel Podivilov]

Web Inspector: native breakpoints aren't hit on navigation

[Pavel Podivilov]

Created attachment 72709 [details]
Patch.

[Pavel Feldman]

Comment on attachment 72709 [details]
Patch.

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

I see two changes here: first makes front-end responsible for assigning breakpoint ids, second makes them survive commit load. Two questions:
- is the first part really related to the fix,
- i don't see where the breakpoint maps get into the state cookie which makes me think that it won't stop upon renderer (inspector controller) change.

> WebCore/inspector/InspectorController.cpp:730
> +            m_debuggerAgent->setPauseWhenAttached();

Should this be called in reuseFrontend?

> WebCore/inspector/InspectorDOMAgent.cpp:782
> +    m_breakpointToId.remove(breakpoint);

clean up m_idToBreakpoint here as well?

[Pavel Podivilov]

(In reply to comment #2)
> (From update of attachment 72709 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72709&action=review
> 
> I see two changes here: first makes front-end responsible for assigning breakpoint ids, second makes them survive commit load. Two questions:
> - is the first part really related to the fix,
It is related, because without this change, closing front-end while breakpoints are restored, may result in inconsistent state.

> - i don't see where the breakpoint maps get into the state cookie which makes me think that it won't stop upon renderer (inspector controller) change.
Breakpoints are set by front-end while debugger is paused on the first script.

> 
> > WebCore/inspector/InspectorController.cpp:730
> > +            m_debuggerAgent->setPauseWhenAttached();
> 
> Should this be called in reuseFrontend?
I think calling this in didCommitLoad should be enough.

> 
> > WebCore/inspector/InspectorDOMAgent.cpp:782
> > +    m_breakpointToId.remove(breakpoint);
> 
> clean up m_idToBreakpoint here as well?
Call to m_idToBreakpoint.take above removes the breakpoint from m_idToBreakpoint.

[Pavel Podivilov]

Created attachment 72829 [details]
Patch.

setNativeBreakpoint should return breakpointId as discussed offline.

[Pavel Feldman]

Comment on attachment 72829 [details]
Patch.

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

> WebCore/inspector/InspectorDOMAgent.cpp:761
> +void InspectorDOMAgent::setDOMBreakpoint(const String& breakpointId, long nodeId, long type)

I like the way it used to be better. Dom agent can manage dom breakpoint handles (i.e. return then and delete by them). And if there already is a breakpoint of a type, we could return illegal handle / report error.

> WebCore/inspector/InspectorDebuggerAgent.cpp:160
> +    if (m_scheduledPauseEventType != type)

This gets complex. Could you provide an example where we hit this guard? Setting r- until you provide explanation.

> WebCore/inspector/front-end/DOMAgent.js:382
> +            WebInspector.breakpointManager.setCanRestoreDOMBreakpoints();

I like the previous name more.

[Pavel Podivilov]

Created attachment 74222 [details]
Comments addressed.

[Pavel Podivilov]

(In reply to comment #5)
> (From update of attachment 72829 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72829&action=review
> 
> > WebCore/inspector/InspectorDOMAgent.cpp:761
> > +void InspectorDOMAgent::setDOMBreakpoint(const String& breakpointId, long nodeId, long type)
> 
> I like the way it used to be better. Dom agent can manage dom breakpoint handles (i.e. return then and delete by them). And if there already is a breakpoint of a type, we could return illegal handle / report error.

Done.

> 
> > WebCore/inspector/InspectorDebuggerAgent.cpp:160
> > +    if (m_scheduledPauseEventType != type)
> 
> This gets complex. Could you provide an example where we hit this guard? Setting r- until you provide explanation.

The idea is to ignore schedulePause/cancelPause called from InspectorInstrumentation when "pause on attach" or "pause on next JS statement" is already scheduled to avoid unwanted pause cancellation.

> 
> > WebCore/inspector/front-end/DOMAgent.js:382
> > +            WebInspector.breakpointManager.setCanRestoreDOMBreakpoints();
> 
> I like the previous name more.

setCanRestoreDOMBreakpoints -> documentSet.

[Pavel Podivilov]

Created attachment 75912 [details]
Patch.

Use backend cookie to restore sticky breakpoints on navigation instead of pausing debugger on attach.

[Yury Semikhatsky]

Comment on attachment 75912 [details]
Patch.

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

> WebCore/inspector/InspectorState.cpp:52
> +    registerObject(stickyBreakpoints, (const char*)0, (const char*)0);

Let's introduce a constant for (const char*)0 or better pass String() since the method accepts const String&

> WebCore/inspector/front-end/BreakpointManager.js:69
> +        var stickyId = this._setNativeBreakpoint("DOM", !disabled, { path: path, domEventType: domEventType });

Please extract constants for native breakpoint types.

> WebCore/inspector/front-end/BreakpointManager.js:159
> +        InspectorBackend.setNativeBreakpoint(data, this.stickyBreakpointRestored.bind(this, stickyId));

The callback method should be called didSetBreakpoint or didSetNativeBreakpoint.

> WebCore/inspector/front-end/BreakpointManager.js:169
> +    stickyBreakpointRestored: function(stickyId, backendId)

Can you explain why we need two types of ids: sticky and backend ones? It seems that the front-end can assign ids for all the breakpoints when it loads them from settings/they are being added and the back-end can reuse the same ids.

Also from the FE point of view stickyId is just an id for a breakpoint that is valid during current debug session only so it's not clear why it's sticky.

> WebCore/inspector/front-end/BreakpointManager.js:172
> +        if (!breakpoint || !breakpoint.enabled || breakpoint._backendId) {

What's the scenario when you need to remove breakpoint due to the last condition(breakpoint._backendId)?

> WebCore/inspector/front-end/BreakpointManager.js:261
> +            if (breakpoint.type === "EventListener")

Why not create these views after they have been restored on the back-end and the front-end receives stickyBreakpointRestored notification?

[Pavel Podivilov]

Created attachment 76399 [details]
Patch.

[Eric Seidel (no email)]

Attachment 76399 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7028066

[Yury Semikhatsky]

Comment on attachment 76399 [details]
Patch.

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

> WebCore/inspector/front-end/BreakpointManager.js:36
> +        this._stickyBreakpoints[projectId] = this._validateBreakpoints(breakpoints[projectId]);

Why do we need this validation? No one else has access to the storage.

> WebCore/inspector/front-end/BreakpointManager.js:193
> +            if (breakpoint.type === WebInspector.BreakpointManager.NativeBreakpointTypes.EventListener)

Why is there no branch for DOM breakpoints?

[Pavel Podivilov]

Committed r74103: <http://trac.webkit.org/changeset/74103>

[Pavel Podivilov]

(In reply to comment #12)
> (From update of attachment 76399 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76399&action=review
> 
> > WebCore/inspector/front-end/BreakpointManager.js:36
> > +        this._stickyBreakpoints[projectId] = this._validateBreakpoints(breakpoints[projectId]);
> 
> Why do we need this validation? No one else has access to the storage.

Storage format may change in future.

> 
> > WebCore/inspector/front-end/BreakpointManager.js:193
> > +            if (breakpoint.type === WebInspector.BreakpointManager.NativeBreakpointTypes.EventListener)
> 
> Why is there no branch for DOM breakpoints?

DOM breakpoints are restored after pushing nodes by path to frontend.