164334 2016-11-02 13:26:23 -0700 Web Inspector: URL Breakpoints that resolve in multiple workers should only appear in the UI once 2016-11-15 13:38:50 -0800 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 joepeck joepeck bburg commit-queue joepeck mattbaker nvasilyev timothy webkit-bug-importer oldest_to_newest 1247442 0 joepeck 2016-11-02 13:26:23 -0700 Summary: URL Breakpoints that resolve in multiple workers should only appear in the UI once Steps to Reproduce: 1. Inspect <https://nerget.com/rayjs-mt/rayjs.html> 2. With 4 Renderers click "Go!" to spawn workers render 3. Set a breakpoint at render-task.js:14 4. Reload 5. Click "Go!" => Breakpoint resolves in each new worker and shows up 4 times in the UI Notes: - We should refactor and harden Breakpoints by URL more generally. - Clicking the gutter to set a breakpoint almost always (95%+) sets a breakpoint by URL. This is necessary to restore the breakpoint when reloading / navigating the page and hitting the breakpoint. So URL is almost always the right solution. - Any Breakpoint by URL may resolve to MULTIPLE locations - A page with multiple workers loading the same script (worker.js) will resolve the breakpoint to the same location but each tied to the different Worker target. - A page that loads the same URL (script.php) with different comments may resolve the breakpoint to different locations in each. This is rare and admittedly poor use of URLs and so rare that I don't think we should concern ourselves with this. I think the relationship between objects is: DebuggerManager - N Breakpoints Breakpoint (by URL) - 1 Set Location ("URL:line:column") - N Resolved Locations (sourceCodeLocation) - target1:script:line:column - target2:script:line:column - target3:script:line:column Breakpoint (by ScriptIdentifier) - 1 Set Location (target:scriptId:line:column) - At most 1 Resolved Location (sourceCodeLocation) This impacts the current "Breakpoint.resolved" state. It seems "resolved" should move to a computed property based on whether or not there are _any resolved locations_. Likewise the list of _unique locations_ would be better to show in the UI, instead of all the resolved locations on which there may be multiple script:line:column duplicates just in different targets. We could provide a way to enable/disable a breakpoint per-target, but that seems like such a rare case that we shouldn't provide that option. 1247443 1 webkit-bug-importer 2016-11-02 13:26:33 -0700 <rdar://problem/29073523> 1251529 2 joepeck 2016-11-15 11:47:59 -0800 Hmm, so I have a simple fix to address this for now. I do want to cleanup the Model objects a bit though. I'm thinking something like: Breakpoint - identifier : string - userLocation : SourceCodeLocation - resolvedLocations : [SourceCodeLocation] One fortunate thing is that URL breakpoints get the same breakpoint identifier across all targets (it is url:line:column and not tied to a unique counter). So a breakpoint might resolve differently across multiple targets but have the same identifier. 1251530 3 294860 joepeck 2016-11-15 11:49:04 -0800 Created attachment 294860 [PATCH] Proposed Fix 1251552 4 mattbaker 2016-11-15 12:13:11 -0800 Seems related to the change: Regressions: Unexpected timeouts (1) inspector/worker/debugger-pause.html [ Timeout ] 1251555 5 mattbaker 2016-11-15 12:14:14 -0800 (In reply to comment #4) > Seems related to the change: > > Regressions: Unexpected timeouts (1) > inspector/worker/debugger-pause.html [ Timeout ] Actually it shouldn't, since this only changes what tree elements are added. 1251565 6 294860 commit-queue 2016-11-15 12:35:48 -0800 Comment on attachment 294860 [PATCH] Proposed Fix Clearing flags on attachment: 294860 Committed r208746: <http://trac.webkit.org/changeset/208746> 1251566 7 commit-queue 2016-11-15 12:35:52 -0800 All reviewed patches have been landed. Closing bug. 1251598 8 joepeck 2016-11-15 13:38:50 -0800 (In reply to comment #5) > (In reply to comment #4) > > Seems related to the change: > > > > Regressions: Unexpected timeouts (1) > > inspector/worker/debugger-pause.html [ Timeout ] > > Actually it shouldn't, since this only changes what tree elements are added. Addressing this in: bug 164787 <https://webkit.org/b/164787> Web Inspector: inspector/worker/debugger-pause.html fails on WebKit1 294860 2016-11-15 11:49:04 -0800 2016-11-15 12:35:48 -0800 [PATCH] Proposed Fix duplicates-1.patch text/plain 2499 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDIyOWYyNWMuLmJmNjQ5YzIgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkluc3BlY3Rv clVJL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE2LTExLTE1ICBKb3NlcGggUGVjb3Jh cm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogVVJMIEJy ZWFrcG9pbnRzIHRoYXQgcmVzb2x2ZSBpbiBtdWx0aXBsZSB3b3JrZXJzIHNob3VsZCBvbmx5IGFw cGVhciBpbiB0aGUgVUkgb25jZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTY0MzM0CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8yOTA3MzUyMz4KKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIFVzZXJJbnRl cmZhY2UvVmlld3MvRGVidWdnZXJTaWRlYmFyUGFuZWwuanM6CisgICAgICAgIChXZWJJbnNwZWN0 b3IuRGVidWdnZXJTaWRlYmFyUGFuZWwucHJvdG90eXBlLl9hZGRCcmVha3BvaW50KToKKyAgICAg ICAgRG9uJ3QgYWRkIGEgZHVwbGljYXRlIEJyZWFrcG9pbnRUcmVlRWxlbWVudHMgZm9yIHRoZSBz YW1lIEJyZWFrcG9pbnQuCisKIDIwMTYtMTEtMTQgIEpvc2VwaCBQZWNvcmFybyAgPHBlY29yYXJv QGFwcGxlLmNvbT4KIAogICAgICAgICBXZWIgSW5zcGVjdG9yOiBTb3VyY2VDb2RlVGV4dEVkaXRv ciBzaG91bGQgZGlzcGxheSBleGVjdXRpb24gbGluZXMgZm9yIGJhY2tncm91bmQgdGhyZWFkcwpk aWZmIC0tZ2l0IGEvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvRGVi dWdnZXJTaWRlYmFyUGFuZWwuanMgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFj ZS9WaWV3cy9EZWJ1Z2dlclNpZGViYXJQYW5lbC5qcwppbmRleCAyYWE4MGVkLi40ZGEwMTc1IDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9EZWJ1 Z2dlclNpZGViYXJQYW5lbC5qcworKysgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVy ZmFjZS9WaWV3cy9EZWJ1Z2dlclNpZGViYXJQYW5lbC5qcwpAQCAtMzc4LDEwICszNzgsMTMgQEAg V2ViSW5zcGVjdG9yLkRlYnVnZ2VyU2lkZWJhclBhbmVsID0gY2xhc3MgRGVidWdnZXJTaWRlYmFy UGFuZWwgZXh0ZW5kcyBXZWJJbnNwZWMKIAogICAgIF9hZGRCcmVha3BvaW50KGJyZWFrcG9pbnQp CiAgICAgewotICAgICAgICB2YXIgc291cmNlQ29kZSA9IGJyZWFrcG9pbnQuc291cmNlQ29kZUxv Y2F0aW9uLmRpc3BsYXlTb3VyY2VDb2RlOworICAgICAgICBsZXQgc291cmNlQ29kZSA9IGJyZWFr cG9pbnQuc291cmNlQ29kZUxvY2F0aW9uLmRpc3BsYXlTb3VyY2VDb2RlOwogICAgICAgICBpZiAo IXNvdXJjZUNvZGUpCiAgICAgICAgICAgICByZXR1cm4gbnVsbDsKIAorICAgICAgICBpZiAodGhp cy5fYnJlYWtwb2ludHNDb250ZW50VHJlZU91dGxpbmUuZmluZFRyZWVFbGVtZW50KGJyZWFrcG9p bnQpKQorICAgICAgICAgICAgcmV0dXJuOworCiAgICAgICAgIGxldCBwYXJlbnRUcmVlRWxlbWVu dCA9IHRoaXMuX2FkZFRyZWVFbGVtZW50Rm9yU291cmNlQ29kZVRvVHJlZU91dGxpbmUoc291cmNl Q29kZSwgdGhpcy5fYnJlYWtwb2ludHNDb250ZW50VHJlZU91dGxpbmUpOwogCiAgICAgICAgIC8v IE1hcmsgZGlzYWJsZWQgYnJlYWtwb2ludHMgYXMgcmVzb2x2ZWQgaWYgdGhlcmUgaXMgc291cmNl IGNvZGUgbG9hZGVkIHdpdGggdGhhdCBVUkwuCkBAIC0zOTEsNyArMzk0LDcgQEAgV2ViSW5zcGVj dG9yLkRlYnVnZ2VyU2lkZWJhclBhbmVsID0gY2xhc3MgRGVidWdnZXJTaWRlYmFyUGFuZWwgZXh0 ZW5kcyBXZWJJbnNwZWMKICAgICAgICAgaWYgKGJyZWFrcG9pbnQuZGlzYWJsZWQpCiAgICAgICAg ICAgICBicmVha3BvaW50LnJlc29sdmVkID0gdHJ1ZTsKIAotICAgICAgICB2YXIgYnJlYWtwb2lu dFRyZWVFbGVtZW50ID0gbmV3IFdlYkluc3BlY3Rvci5CcmVha3BvaW50VHJlZUVsZW1lbnQoYnJl YWtwb2ludCk7CisgICAgICAgIGxldCBicmVha3BvaW50VHJlZUVsZW1lbnQgPSBuZXcgV2ViSW5z cGVjdG9yLkJyZWFrcG9pbnRUcmVlRWxlbWVudChicmVha3BvaW50KTsKICAgICAgICAgcGFyZW50 VHJlZUVsZW1lbnQuaW5zZXJ0Q2hpbGQoYnJlYWtwb2ludFRyZWVFbGVtZW50LCBpbnNlcnRpb25J bmRleEZvck9iamVjdEluTGlzdFNvcnRlZEJ5RnVuY3Rpb24oYnJlYWtwb2ludFRyZWVFbGVtZW50 LCBwYXJlbnRUcmVlRWxlbWVudC5jaGlsZHJlbiwgdGhpcy5fY29tcGFyZURlYnVnZ2VyVHJlZUVs ZW1lbnRzKSk7CiAgICAgICAgIGlmIChwYXJlbnRUcmVlRWxlbWVudC5jaGlsZHJlbi5sZW5ndGgg PT09IDEpCiAgICAgICAgICAgICBwYXJlbnRUcmVlRWxlbWVudC5leHBhbmQoKTsK