39094 2010-05-13 16:34:55 -0700 Web Inspector: Clearing Breakpoints Too Often 2010-05-16 13:50:26 -0700 1 1 1 Unclassified WebKit Web Inspector (Deprecated) 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 joepeck joepeck bweinstein joepeck keishi pfeldman pmuellr rik timothy yurys oldest_to_newest 225566 0 joepeck 2010-05-13 16:34:55 -0700 Just like workers, breakpoints should be preserved across simple resets. Patch to follow. 225569 1 56033 joepeck 2010-05-13 16:43:38 -0700 Created attachment 56033 [PATCH] Preserve Breakpoints Simple fix. 225570 2 joepeck 2010-05-13 16:46:17 -0700 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. 225575 3 joepeck 2010-05-13 17:03:07 -0700 Committed r59417 M WebCore/ChangeLog M WebCore/inspector/front-end/ScriptsPanel.js r59417 = 5a397efce539fa89f7b448e5b9aecc1ecdf1ca71 (refs/remotes/trunk) http://trac.webkit.org/changeset/59417 Thanks! 225664 4 pfeldman 2010-05-13 22:07:02 -0700 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? 225666 5 joepeck 2010-05-13 22:11:43 -0700 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. 225667 6 joepeck 2010-05-13 22:13:10 -0700 (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. 225670 7 joepeck 2010-05-13 22:23:30 -0700 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. 225671 8 pfeldman 2010-05-13 22:29:45 -0700 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. 225675 9 joepeck 2010-05-13 23:06:30 -0700 > 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. 225681 10 pfeldman 2010-05-14 00:01:50 -0700 > 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. 225684 11 joepeck 2010-05-14 00:27:50 -0700 > 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? 225687 12 joepeck 2010-05-14 00:32:12 -0700 > 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. 226531 13 joepeck 2010-05-16 13:38:12 -0700 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." 226533 14 joepeck 2010-05-16 13:50:26 -0700 Filed: https://bugs.webkit.org/show_bug.cgi?id=39185 56033 2010-05-13 16:43:38 -0700 2010-05-13 16:47:02 -0700 [PATCH] Preserve Breakpoints fix.patch text/plain 1498 joepeck ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg NGY4NDBlOC4uOTBhYmZkMCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAorMjAxMC0wNS0xMyAgSm9zZXBoIFBlY29y YXJvICA8am9lcGVja0B3ZWJraXQub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIFdlYiBJbnNwZWN0b3I6IENsZWFyaW5nIEJyZWFrcG9pbnRzIFRv byBPZnRlbgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MzkwOTQKKworICAgICAgICBNaW5vciByZXNldHMgc2hvdWxkIHByZXNlcnZlIGJvdGggd29ya2Vy cyBhbmQgYnJlYWtwb2ludHMuCisKKyAgICAgICAgKiBpbnNwZWN0b3IvZnJvbnQtZW5kL1Njcmlw dHNQYW5lbC5qczoKKyAgICAgICAgKFdlYkluc3BlY3Rvci5TY3JpcHRzUGFuZWwucHJvdG90eXBl LnJlc2V0KToKKwogMjAxMC0wNS0xMyAgU2ltb24gRnJhc2VyICA8c2ltb24uZnJhc2VyQGFwcGxl LmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBEYXZlIEh5YXR0LgpkaWZmIC0tZ2l0IGEvV2Vi Q29yZS9pbnNwZWN0b3IvZnJvbnQtZW5kL1NjcmlwdHNQYW5lbC5qcyBiL1dlYkNvcmUvaW5zcGVj dG9yL2Zyb250LWVuZC9TY3JpcHRzUGFuZWwuanMKaW5kZXggNjM0NDU1NC4uZDE1MzZjZCAxMDA2 NDQKLS0tIGEvV2ViQ29yZS9pbnNwZWN0b3IvZnJvbnQtZW5kL1NjcmlwdHNQYW5lbC5qcworKysg Yi9XZWJDb3JlL2luc3BlY3Rvci9mcm9udC1lbmQvU2NyaXB0c1BhbmVsLmpzCkBAIC00NzgsNyAr NDc4LDcgQEAgV2ViSW5zcGVjdG9yLlNjcmlwdHNQYW5lbC5wcm90b3R5cGUgPSB7CiAgICAgICAg IHRoaXMucmVzZXQodHJ1ZSk7CiAgICAgfSwKIAotICAgIHJlc2V0OiBmdW5jdGlvbihwcmVzZXJ2 ZVdvcmtlcnMpCisgICAgcmVzZXQ6IGZ1bmN0aW9uKHByZXNlcnZlSXRlbXMpCiAgICAgewogICAg ICAgICB0aGlzLnZpc2libGVWaWV3ID0gbnVsbDsKIApAQCAtNTExLDExICs1MTEsMTIgQEAgV2Vi SW5zcGVjdG9yLlNjcmlwdHNQYW5lbC5wcm90b3R5cGUgPSB7CiAgICAgICAgIH0KIAogICAgICAg ICB0aGlzLl9zb3VyY2VJRE1hcCA9IHt9OwotICAgICAgICAKKwogICAgICAgICB0aGlzLnNpZGVi YXJQYW5lcy53YXRjaEV4cHJlc3Npb25zLnJlZnJlc2hFeHByZXNzaW9ucygpOwotICAgICAgICB0 aGlzLnNpZGViYXJQYW5lcy5icmVha3BvaW50cy5yZXNldCgpOwotICAgICAgICBpZiAoIXByZXNl cnZlV29ya2VycykKKyAgICAgICAgaWYgKCFwcmVzZXJ2ZUl0ZW1zKSB7CisgICAgICAgICAgICB0 aGlzLnNpZGViYXJQYW5lcy5icmVha3BvaW50cy5yZXNldCgpOwogICAgICAgICAgICAgdGhpcy5z aWRlYmFyUGFuZXMud29ya2Vycy5yZXNldCgpOworICAgICAgICB9CiAgICAgfSwKIAogICAgIGdl dCB2aXNpYmxlVmlldygpCg==