135396 2014-07-29 15:30:45 -0700 Web Inspector: breakpoints are always speculatively resolved when restored from local storage 2014-07-30 09:50:07 -0700 1 1 1 Unclassified WebKit Web Inspector 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 135367 1 burg burg commit-queue graouts joepeck timothy webkit-bug-importer oldest_to_newest 1025566 0 burg 2014-07-29 15:30:45 -0700 A long-time quirk/optimization in the frontend is that we immediately set a breakpoint as resolved if the breakpoint was successfully set in the backend. The purpose of this is to make clicking in the gutter immediately produce a resolved breakpoint, rather than waiting for several round trips (perceived as lag). However, the logic is wrong such that any breakpoint that is set via DebuggerManager.addBreakpoint will be speculatively resolved, even if its corresponding source code has never been loaded. This can cause problems for later code that assumes a resolved breakpoint also has a valid sourceCodeLocation.sourceCode. 1025568 1 webkit-bug-importer 2014-07-29 15:33:12 -0700 <rdar://problem/17848753> 1025587 2 235707 burg 2014-07-29 15:56:49 -0700 Created attachment 235707 Patch 1025640 3 235707 commit-queue 2014-07-29 20:49:11 -0700 Comment on attachment 235707 Patch Clearing flags on attachment: 235707 Committed r171784: <http://trac.webkit.org/changeset/171784> 1025641 4 commit-queue 2014-07-29 20:49:14 -0700 All reviewed patches have been landed. Closing bug. 1025658 5 235707 joepeck 2014-07-29 21:24:55 -0700 Comment on attachment 235707 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235707&action=review > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:257 > + function speculativelyResolveBreakpoint(breakpoint) { > + breakpoint.resolved = true; > + } > + > if (!breakpoint.disabled) > - this._setBreakpoint(breakpoint); > + this._setBreakpoint(breakpoint, shouldSpeculativelyResolve ? speculativelyResolveBreakpoint.bind(null, breakpoint) : null); I think we can improve this. The _setBreakpoint callback should be passed the breakpoint, so that we aren't creating all this bound functions. function speculativelyResolveBreakpoint(breakpoint) { breakpoint.resolved = true; } this._setBreakpoint(breakpoint, shouldSpeculativelyResolve ? speculativelyResolveBrekapoint : null); > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:548 > + for (var location of locations) > + this.breakpointResolved(breakpointIdentifier, location); > > if (typeof callback === "function") > callback(); And here, callback would pass the breakpoint: callback(breakpoint); 1025726 6 burg 2014-07-30 09:50:07 -0700 (In reply to comment #5) > (From update of attachment 235707 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=235707&action=review > > > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:257 > > + function speculativelyResolveBreakpoint(breakpoint) { > > + breakpoint.resolved = true; > > + } > > + > > if (!breakpoint.disabled) > > - this._setBreakpoint(breakpoint); > > + this._setBreakpoint(breakpoint, shouldSpeculativelyResolve ? speculativelyResolveBreakpoint.bind(null, breakpoint) : null); > > I think we can improve this. > > The _setBreakpoint callback should be passed the breakpoint, so that we aren't creating all this bound functions. > > function speculativelyResolveBreakpoint(breakpoint) { > breakpoint.resolved = true; > } > > this._setBreakpoint(breakpoint, shouldSpeculativelyResolve ? speculativelyResolveBrekapoint : null); > > > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:548 > > + for (var location of locations) > > + this.breakpointResolved(breakpointIdentifier, location); > > > > if (typeof callback === "function") > > callback(); > > And here, callback would pass the breakpoint: > > callback(breakpoint); I was going to go full-blown promises here since this code doesn't handle errors well, but I'm trying to restrain myself from allocation and command optimizations until we have some useful profiling data. 235707 2014-07-29 15:56:49 -0700 2014-07-29 21:24:55 -0700 Patch bug-135396-20140729155610.patch text/plain 5989 burg U3VidmVyc2lvbiBSZXZpc2lvbjogMTcxNzExCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCA3 NTdkODJmY2Y2MDUwODNjNDQ3ZDVmMmFhMGEwMTcxY2ZjZjNjMTIyLi4xZGJhMDc1NTU0NDg3YWQ5 NDg4YjU1ZTY1YTEzZDgwNjZhMWM3NGQwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwzMiBAQAorMjAxNC0wNy0yOSAgQnJpYW4gSi4gQnVyZyAgPGJ1cmdAY3Mud2FzaGluZ3Rv bi5lZHU+CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogYnJlYWtwb2ludHMgYXJlIGFsd2F5cyBz cGVjdWxhdGl2ZWx5IHJlc29sdmVkIHdoZW4gcmVzdG9yZWQgZnJvbSBsb2NhbCBzdG9yYWdlCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzUzOTYKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBBIGxvbmdzdGFu ZGluZyBxdWlyay9vcHRpbWl6YXRpb24gaW4gdGhlIGZyb250ZW5kIGlzIHRoYXQgd2UgaW1tZWRp YXRlbHkgc2V0IGEgYnJlYWtwb2ludAorICAgICAgICBhcyByZXNvbHZlZCBpZiB0aGUgYnJlYWtw b2ludCB3YXMgc3VjY2Vzc2Z1bGx5IHNldCBpbiB0aGUgYmFja2VuZC4gVGhpcyBlbnN1cmVzIHRo YXQgY2xpY2tpbmcgaW4KKyAgICAgICAgdGhlIGd1dHRlciBpbW1lZGlhdGVseSBwcm9kdWNlcyBh IHJlc29sdmVkIGJyZWFrcG9pbnQgd2l0aCBvbmx5IG9uZSByb3VuZC10cmlwLgorCisgICAgICAg IEhvd2V2ZXIsIG5vdCBhbGwgYnJlYWtwb2ludHMgc2hvdWxkIGJlIHNwZWN1bGF0aXZlbHkgcmVz b2x2ZWQsIGJlY2F1c2UgdGhlIGNvcnJlc3BvbmRpbmcgcmVzb3VyY2UKKyAgICAgICAgbWF5IG5v dCBiZSBsb2FkZWQgeWV0LiBUaGlzIHNpdHVhdGlvbiBjYXVzZXMgcHJvYmxlbXMgZm9yIGNvZGUg dGhhdCBhc3N1bWVzIGEgcmVzb2x2ZWQgYnJlYWtwb2ludAorICAgICAgICBhbHNvIGhhcyBhIHZh bGlkIHNvdXJjZUNvZGVMb2NhdGlvbi5zb3VyY2VDb2RlLgorCisgICAgICAgICogVXNlckludGVy ZmFjZS9Db250cm9sbGVycy9EZWJ1Z2dlck1hbmFnZXIuanM6CisgICAgICAgIChXZWJJbnNwZWN0 b3IuRGVidWdnZXJNYW5hZ2VyLnJlc3RvcmVCcmVha3BvaW50c1Nvb24pOiBEb24ndCBsZWFrIHRo ZSB2YXJpYWJsZSB0byBnbG9iYWwgc2NvcGUuCisgICAgICAgIChXZWJJbnNwZWN0b3IuRGVidWdn ZXJNYW5hZ2VyKToKKyAgICAgICAgKFdlYkluc3BlY3Rvci5EZWJ1Z2dlck1hbmFnZXIucHJvdG90 eXBlLnNwZWN1bGF0aXZlbHlSZXNvbHZlQnJlYWtwb2ludCk6CisgICAgICAgIChXZWJJbnNwZWN0 b3IuRGVidWdnZXJNYW5hZ2VyLnByb3RvdHlwZS5hZGRCcmVha3BvaW50KTogU3BlY3VsYXRpdmVs eSByZXNvbHZlIGhlcmUgaWYgcmVxdWVzdGVkCisgICAgICAgIHVzaW5nIHRoZSBzdWNjZXNzIGNh bGxiYWNrIHBhc3NlZCB0byBfc2V0QnJlYWtwb2ludC4KKworICAgICAgICAoV2ViSW5zcGVjdG9y LkRlYnVnZ2VyTWFuYWdlci5wcm90b3R5cGUuZGlkU2V0QnJlYWtwb2ludCk6IEVtaXQgc2ltdWxh dGVkCisgICAgICAgIERlYnVnZ2VyLmJyZWFrcG9pbnRSZXNvbHZlZCBldmVudHMgc2luY2UgdGhl eSBhcmUgb25seSBzZW50IGJ5IHRoZSBiYWNrZW5kIHdoZW4gYSBzY3JpcHQgaXMgcGFyc2VkLgor CisgICAgICAgIChXZWJJbnNwZWN0b3IuRGVidWdnZXJNYW5hZ2VyLnByb3RvdHlwZS5fc2V0QnJl YWtwb2ludCk6CisgICAgICAgICogVXNlckludGVyZmFjZS9WaWV3cy9Tb3VyY2VDb2RlVGV4dEVk aXRvci5qczoKKyAgICAgICAgKFdlYkluc3BlY3Rvci5Tb3VyY2VDb2RlVGV4dEVkaXRvci5wcm90 b3R5cGUudGV4dEVkaXRvckJyZWFrcG9pbnRBZGRlZCk6IFJlcXVlc3Qgc3BlY3VsYXRpdmUgcmVz b2x2ZS4KKwogMjAxNC0wNy0yOCAgSm9zZXBoIFBlY29yYXJvICA8cGVjb3Jhcm9AYXBwbGUuY29t PgogCiAgICAgICAgIFdlYiBJbnNwZWN0b3I6IERpc2FibGUgQ29weSBSb3cgaW4gVGltZWxpbmVz IERhdGFHcmlkcywgaXQgZG9lcyBub3QgY3VycmVudGx5IHByb3ZpZGUgdmFsdWUKZGlmZiAtLWdp dCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL0NvbnRyb2xsZXJzL0RlYnVn Z2VyTWFuYWdlci5qcyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL0NvbnRy b2xsZXJzL0RlYnVnZ2VyTWFuYWdlci5qcwppbmRleCBlNjVhYWMxYjViMzNkZWQ1ZjczMTZiYzFi ZDJjMWRlODM4OTAxMzc0Li5kYmQxZjFkNGQ0N2IwZWRkMWI5ZGQ0M2M4ODFhNGI0ZmIzM2JmOTg3 IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9Db250cm9s bGVycy9EZWJ1Z2dlck1hbmFnZXIuanMKKysrIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJ bnRlcmZhY2UvQ29udHJvbGxlcnMvRGVidWdnZXJNYW5hZ2VyLmpzCkBAIC02Nyw3ICs2Nyw3IEBA IFdlYkluc3BlY3Rvci5EZWJ1Z2dlck1hbmFnZXIgPSBmdW5jdGlvbigpCiAgICAgdGhpcy5fdXBk YXRlQnJlYWtPbkV4Y2VwdGlvbnNTdGF0ZSgpOwogCiAgICAgZnVuY3Rpb24gcmVzdG9yZUJyZWFr cG9pbnRzU29vbigpIHsKLSAgICAgICAgZm9yIChjb29raWUgb2YgdGhpcy5fYnJlYWtwb2ludHNT ZXR0aW5nLnZhbHVlKQorICAgICAgICBmb3IgKHZhciBjb29raWUgb2YgdGhpcy5fYnJlYWtwb2lu dHNTZXR0aW5nLnZhbHVlKQogICAgICAgICAgICAgdGhpcy5hZGRCcmVha3BvaW50KG5ldyBXZWJJ bnNwZWN0b3IuQnJlYWtwb2ludChjb29raWUpKTsKICAgICB9CiAKQEAgLTIyNyw3ICsyMjcsNyBA QCBXZWJJbnNwZWN0b3IuRGVidWdnZXJNYW5hZ2VyLnByb3RvdHlwZSA9IHsKICAgICAgICAgRGVi dWdnZXJBZ2VudC5jb250aW51ZVRvTG9jYXRpb24oe3NjcmlwdElkOiBzY3JpcHRJZGVudGlmaWVy LCBsaW5lTnVtYmVyOiBsaW5lTnVtYmVyLCBjb2x1bW5OdW1iZXI6IGNvbHVtbk51bWJlcn0pOwog ICAgIH0sCiAKLSAgICBhZGRCcmVha3BvaW50OiBmdW5jdGlvbihicmVha3BvaW50LCBza2lwRXZl bnREaXNwYXRjaCkKKyAgICBhZGRCcmVha3BvaW50OiBmdW5jdGlvbihicmVha3BvaW50LCBza2lw RXZlbnREaXNwYXRjaCwgc2hvdWxkU3BlY3VsYXRpdmVseVJlc29sdmUpCiAgICAgewogICAgICAg ICBjb25zb2xlLmFzc2VydChicmVha3BvaW50IGluc3RhbmNlb2YgV2ViSW5zcGVjdG9yLkJyZWFr cG9pbnQsICJCYWQgYXJndW1lbnQgdG8gRGVidWdnZXJNYW5nZXIuYWRkQnJlYWtwb2ludDogIiwg YnJlYWtwb2ludCk7CiAgICAgICAgIGlmICghYnJlYWtwb2ludCkKQEAgLTI0OSw4ICsyNDksMTIg QEAgV2ViSW5zcGVjdG9yLkRlYnVnZ2VyTWFuYWdlci5wcm90b3R5cGUgPSB7CiAKICAgICAgICAg dGhpcy5fYnJlYWtwb2ludHMucHVzaChicmVha3BvaW50KTsKIAorICAgICAgICBmdW5jdGlvbiBz cGVjdWxhdGl2ZWx5UmVzb2x2ZUJyZWFrcG9pbnQoYnJlYWtwb2ludCkgeworICAgICAgICAgICAg YnJlYWtwb2ludC5yZXNvbHZlZCA9IHRydWU7CisgICAgICAgIH0KKwogICAgICAgICBpZiAoIWJy ZWFrcG9pbnQuZGlzYWJsZWQpCi0gICAgICAgICAgICB0aGlzLl9zZXRCcmVha3BvaW50KGJyZWFr cG9pbnQpOworICAgICAgICAgICAgdGhpcy5fc2V0QnJlYWtwb2ludChicmVha3BvaW50LCBzaG91 bGRTcGVjdWxhdGl2ZWx5UmVzb2x2ZSA/IHNwZWN1bGF0aXZlbHlSZXNvbHZlQnJlYWtwb2ludC5i aW5kKG51bGwsIGJyZWFrcG9pbnQpIDogbnVsbCk7CiAKICAgICAgICAgaWYgKCFza2lwRXZlbnRE aXNwYXRjaCkKICAgICAgICAgICAgIHRoaXMuZGlzcGF0Y2hFdmVudFRvTGlzdGVuZXJzKFdlYklu c3BlY3Rvci5EZWJ1Z2dlck1hbmFnZXIuRXZlbnQuQnJlYWtwb2ludEFkZGVkLCB7YnJlYWtwb2lu dDogYnJlYWtwb2ludH0pOwpAQCAtNTI0LDcgKzUyOCw3IEBAIFdlYkluc3BlY3Rvci5EZWJ1Z2dl ck1hbmFnZXIucHJvdG90eXBlID0gewogICAgICAgICAvLyBhIG11bHRpLXN0ZXAgcHJvY2VzcyBm b3IgdGhlIHVzZXIgdGhhdCBjYW4gYmUgY29uZnVzaW5nLgogICAgICAgICB0aGlzLmJyZWFrcG9p bnRzRW5hYmxlZCA9IHRydWU7CiAKLSAgICAgICAgZnVuY3Rpb24gZGlkU2V0QnJlYWtwb2ludChl cnJvciwgYnJlYWtwb2ludElkZW50aWZpZXIpCisgICAgICAgIGZ1bmN0aW9uIGRpZFNldEJyZWFr cG9pbnQoZXJyb3IsIGJyZWFrcG9pbnRJZGVudGlmaWVyLCBsb2NhdGlvbnMpCiAgICAgICAgIHsK ICAgICAgICAgICAgIGlmIChlcnJvcikKICAgICAgICAgICAgICAgICByZXR1cm47CkBAIC01MzIs NyArNTM2LDEzIEBAIFdlYkluc3BlY3Rvci5EZWJ1Z2dlck1hbmFnZXIucHJvdG90eXBlID0gewog ICAgICAgICAgICAgdGhpcy5fYnJlYWtwb2ludElkTWFwW2JyZWFrcG9pbnRJZGVudGlmaWVyXSA9 IGJyZWFrcG9pbnQ7CiAKICAgICAgICAgICAgIGJyZWFrcG9pbnQuaWRlbnRpZmllciA9IGJyZWFr cG9pbnRJZGVudGlmaWVyOwotICAgICAgICAgICAgYnJlYWtwb2ludC5yZXNvbHZlZCA9IHRydWU7 CisKKyAgICAgICAgICAgIC8vIERlYnVnZ2VyLnNldEJyZWFrcG9pbnQgcmV0dXJucyBhIHNpbmds ZSBsb2NhdGlvbi4KKyAgICAgICAgICAgIGlmICghKGxvY2F0aW9ucyBpbnN0YW5jZW9mIEFycmF5 KSkKKyAgICAgICAgICAgICAgICBsb2NhdGlvbnMgPSBbbG9jYXRpb25zXTsKKworICAgICAgICAg ICAgZm9yICh2YXIgbG9jYXRpb24gb2YgbG9jYXRpb25zKQorICAgICAgICAgICAgICAgIHRoaXMu YnJlYWtwb2ludFJlc29sdmVkKGJyZWFrcG9pbnRJZGVudGlmaWVyLCBsb2NhdGlvbik7CiAKICAg ICAgICAgICAgIGlmICh0eXBlb2YgY2FsbGJhY2sgPT09ICJmdW5jdGlvbiIpCiAgICAgICAgICAg ICAgICAgY2FsbGJhY2soKTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2Vy SW50ZXJmYWNlL1ZpZXdzL1NvdXJjZUNvZGVUZXh0RWRpdG9yLmpzIGIvU291cmNlL1dlYkluc3Bl Y3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvU291cmNlQ29kZVRleHRFZGl0b3IuanMKaW5kZXgg ZmY1OWJmOWY5YWQyYjU4MmNmZGFiNDhkNWI3OGI0ZDFmMjhhMDI1My4uYTlmYjcxZDMxMDUyZDUy NmEzNDExNzNhZGY1ZTljMzk3ODY5YjRiMyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3Rv clVJL1VzZXJJbnRlcmZhY2UvVmlld3MvU291cmNlQ29kZVRleHRFZGl0b3IuanMKKysrIGIvU291 cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvU291cmNlQ29kZVRleHRFZGl0 b3IuanMKQEAgLTgzOSw3ICs4MzksMTAgQEAgV2ViSW5zcGVjdG9yLlNvdXJjZUNvZGVUZXh0RWRp dG9yLnByb3RvdHlwZSA9IHsKICAgICAgICAgdGhpcy5fYWRkQnJlYWtwb2ludFdpdGhFZGl0b3JM aW5lSW5mbyhicmVha3BvaW50LCBsaW5lSW5mbyk7CiAKICAgICAgICAgdGhpcy5faWdub3JlQnJl YWtwb2ludEFkZGVkQnJlYWtwb2ludCA9IGJyZWFrcG9pbnQ7Ci0gICAgICAgIFdlYkluc3BlY3Rv ci5kZWJ1Z2dlck1hbmFnZXIuYWRkQnJlYWtwb2ludChicmVha3BvaW50KTsKKworICAgICAgICB2 YXIgc2hvdWxkU2tpcEV2ZW50RGlzcGF0Y2ggPSBmYWxzZTsKKyAgICAgICAgdmFyIHNob3VsZFNw ZWN1bGF0aXZlbHlSZXNvbHZlQnJlYWtwb2ludCA9IHRydWU7CisgICAgICAgIFdlYkluc3BlY3Rv ci5kZWJ1Z2dlck1hbmFnZXIuYWRkQnJlYWtwb2ludChicmVha3BvaW50LCBzaG91bGRTa2lwRXZl bnREaXNwYXRjaCwgc2hvdWxkU3BlY3VsYXRpdmVseVJlc29sdmVCcmVha3BvaW50KTsKICAgICAg ICAgZGVsZXRlIHRoaXMuX2lnbm9yZUJyZWFrcG9pbnRBZGRlZEJyZWFrcG9pbnQ7CiAKICAgICAg ICAgLy8gUmV0dXJuIHRoZSBtb3JlIGFjY3VyYXRlIGxvY2F0aW9uIGFuZCBicmVha3BvaW50IGlu Zm8uCg==