Bug 39094 – Web Inspector: Clearing Breakpoints Too Often

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

[Joseph Pecoraro]

Just like workers, breakpoints should be preserved across simple resets.

Patch to follow.

[Joseph Pecoraro]

Created attachment 56033 [details]
[PATCH] Preserve Breakpoints

Simple fix.

[Joseph Pecoraro]

I'll have reproduction steps up in a minute. I could eventually see adding tests for reset on all panels eventually. I can open a bug for that.

[Joseph Pecoraro]

Committed r59417
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/ScriptsPanel.js
r59417 = 5a397efce539fa89f7b448e5b9aecc1ecdf1ca71 (refs/remotes/trunk)
http://trac.webkit.org/changeset/59417

Thanks!

[Pavel Feldman]

Guys, is there a user scenario for this? My concern is that breakpoints were cleared intentionally and restored from the backend side upon reload based on the breakpoint <-> url mapping. Or is it some other kind of reset you are talking about?

[Joseph Pecoraro]

User Scenario:

  1. User Opens Inspector
  2. Goes to Resources Panel
  3. Sets Breakpoint in Resource
  4. Goes to Scripts Panel (lazy loading will trigger reset())

This only fails if the Scripts Panel wasn't open yet.
And when it fails, its hard to diagnose. The Breakpoint
still appears on the SourceFrame/View however, the
Breakpoints SidebarPane does not show the breakpoint
and the debugger will not actually stop on it!

The example I forgot to put up was very basic html.

  <button id="x">Click Me<p>
  <script>
  document.getElementById('x').addEventListener('click', function() {
    1+1; // <-- Put breakpoint here
  }, false);
  </script>

If you follow the above scenario, you should trigger
the issue.

[Joseph Pecoraro]

(In reply to comment #5)
Another common Scenario:
 
  1. User Opens Inspector
  2. Goes to Resources Panel
  3. Sets Breakpoint in Resource
  4. Triggers action causing Breakpoint
  5. Debugger Automatically Enabled => triggers reset() destroys breakpoint

Also possible to show with the test case in the prev.
comment's test code.

[Joseph Pecoraro]

Arg. It looks like you might be right. Although I did
test with refreshing the page on my test case, I just
tested on another website and I'm seeing duplicate
breakpoint entires after refresh. I have to leave the
office now, but I can take a look at this when I get home.

[Pavel Feldman]

Why I think this scenario is complex:
- When a breakpoint is set, it is being added into the 'm_stickyBreakpoints' list in inspector controller by url and line.
- When debugging gets enabled, it adds a listener to jsc debugger and recompiles the scripts
- If scripts have matching url, frontend's restoredBreakpoint is called and the breakpoint should get propagated to the front-end.

So something is wrong with the order of cleanups here, but cleaning might still be necessary.

Reason underlying behavior is complex:
Imagine you make a reload and you have a breakpoint in the <script> tag of main resource. You won't have a chance to restore this breakpoint from the front-end since it is asynchronous. Front-end only knows about your page after main resource has been loaded, but we are actually supposed to stop on a breakpoint before that. So we restore these breakpoints synchronously in the inspector controller upon parse. 

So I guess the problem might be with the order of debuggerWasEnabled and restoredBreakpoint. It is worth checking at least. Scenario that I think might fail and is worth testing is whether disabling and further enabling scripts panel creates dupe breakpoints.

[Joseph Pecoraro]

> So I guess the problem might be with the order of debuggerWasEnabled
> and restoredBreakpoint. It is worth checking at least. Scenario that I think
> might fail and is worth testing is whether disabling and further enabling
> scripts panel creates dupe breakpoints.

My patch seems to work fine with these scenarios. Where I was hitting
multiple breakpoints is in fact an issue that exists in the nightly and
is unchanged by this patch. You hint at why this could be happening.

Set a breakpoint on a minified line, such as line 8 of google.com and
disable/enable debugging or refresh the page. Many duplicates emerge.
This might even be desired behavior but with a poor UI. If you think
it is another bug we should open up another bugzilla bug.

[Pavel Feldman]

> My patch seems to work fine with these scenarios. Where I was hitting
> multiple breakpoints is in fact an issue that exists in the nightly and
> is unchanged by this patch. You hint at why this could be happening.
> 

Although the user-visible behavior is now correct, I still have reservations wrt fix. What happens does not meet my expectations (the whole story with restoring breakpoints). They should have been 'restored' and hence get into the scripts panel after enabling. So there is another bug preventing this from happening.

What I don't like is that script debug server's breakpoints map is being populated even when debugger is disabled. Then, upon enabling, dupe breakpoints are likely to be created. Is this happening? So I'd say that the fix is simply short-sighted.

This area is especially fragile since we have two different debugger implementations (jsc and v8). They use same front-end, so we need to keep them aligned. A fix that is working for one, may break the other unless we keep a picture of how debugger should interact with the front-end in mind. I did not check v8 yet, but I think this fix might be breaking it.

[Joseph Pecoraro]

> Although the user-visible behavior is now correct, I still have
> reservations wrt fix. What happens does not meet my expectations
> (the whole story with restoring breakpoints).

The bug in both of my user scenarios was front-end only.
Both times ScriptsPanel.reset(true) was getting called, delegating
down to the Script's panel's breakpoints.reset(). This clears the
visible breakpoints, but makes no effort to actually remove the
breakpoints. Now, in cases where the the user adds breakpoints
in the UI, those breakpoints are not removed from the UI
during common operations:

  1. selecting the Scripts panel
  2. hitting a breakpoint

The BreakpointsSidebarPane seems to prevent against
obvious UI duplicates (I haven't debugged this to see
what the breakpoint id's are when the duplicates appear)
but I find it unlikely that in case 1 there would be an issue.

    // Inside addBreakpoint:
    if (this.breakpoints[breakpoint.id])
        return;

The same basic stuff applies to case 2. I haven't
investigated deeply, but debuggerWasEnabled() calls
the reset(true), removing breakpoints from the UI.
Why when the debugger was enabled would we
want to remove the breakpoints we just potentially
set? =)

So I think this fix is the right fix for the user-visible side.
In that neither of these cases do we want the UI visible
breakpoints to be removed.


> They should have been 'restored' and hence get into the
> scripts panel after enabling. So there is another bug
> preventing this from happening.

Its not that they weren't getting into the scripts panel,
its that they were getting removed, and the above patch
fixed that. Am I missing something again?




> What I don't like is that script debug server's breakpoints
> map is being populated even when debugger is disabled.
> Then, upon enabling, dupe breakpoints are likely to be
> created. Is this happening? So I'd say that the fix is
> simply short-sighted.

So you do not want to allow the user to set a breakpoint
unless they have enabled the Debugger in the Scripts Panel?

Where would dupes come from by enabling the debugger?
The breakpoints that are set are already set, why would the
act of enabling the panel cause new breakpoints to be set?


> This area is especially fragile since we have two different
> debugger implementations (jsc and v8). They use same
> front-end, so we need to keep them aligned. A fix that
> is working for one, may break the other unless we keep
> a picture of how debugger should interact with the
> front-end in mind. I did not check v8 yet, but I think this
> fix might be breaking it.

I'm sorry. I should have waited for some feedback from the
Chrome team on this. However, I still maintain that my
changes are merely cosmetic.

Could you test this in Chrome and let me know if it produces
bad behavior?

[Joseph Pecoraro]

> However, I still maintain that my changes are merely cosmetic.

Well, considering earlier that I said: "Breakpoints SidebarPane does
not show the breakpoint and the debugger will not actually stop on it!"
this may not be the case.

[Joseph Pecoraro]

It is still the case where when you set a breakpoint before the
debugger is enabled at all that the breakpoint will still show in
the SourceFrame (and now it will show in the BreakpointsSidebarPane)
but not actually have been registered in the backend, so it won't
actually break. I think that may have been what Pavel was talking
about. I wil file a bug on this.

This fix makes it the blue breakpoints on source lines match the
breakpoints in the BreakpointsSidebarPane. And I feel that is the
desired behavior. But apparently these breakpoints aren't really
getting registered until the ScriptsPanel (lazy?) loads the debugger.
That should probably happen automatically when the user sets a
breakpoint and had previously set their preference to Always allow
scripts debugging. Because clearly that is the user's intent.

Otherwise we should probably visually show "this breakpoint won't
work yet because the Debugger isn't enabled."

[Joseph Pecoraro]

Filed: https://bugs.webkit.org/show_bug.cgi?id=39185