150579 2015-10-26 16:18:56 -0700 Web Inspector: WebInspector.Object can dispatch constructor-level events multiple times 2015-10-26 17:26:39 -0700 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 mattbaker mattbaker bburg commit-queue graouts joepeck mattbaker nvasilyev timothy webkit-bug-importer oldest_to_newest 1136682 0 mattbaker 2015-10-26 16:18:56 -0700 * SUMMARY WebInspector.Object can dispatch constructor-level events multiple times. As Object.dispatchEventToListeners walks the prototype chain looking for listeners on each constructor, it should check for listeners that are a direct property of the constructor, rather than a simple 'in' test which will pick up properties inherited down the chain. * EXAMPLE Assuming the following class hierarchy: C -> B -> A -> WebInspector.Object and an object Foo listening for events on class A: class Foo() { constructor() { A.addEventListener(A.Event.SomeEvent, this._handleSomeEvent, this); } _handleSomeEvent(event) { ... } } If an object of type C fires A.Event.SomeEvent, Foo._handleSomeEvent will be called three times, once for each constructor in the prototype chain. The expectation is that it will be called just once. * NOTE This has real performance implications in the Inspector UI, as constructor event handlers often perform layout updates. For example, ContentBrowser calls NavigationBar.udpateLayout (which is extremely expensive) in response to WebInspector.ContentView.Event.SelectionPathComponentsDidChange, which it listens for on the ContentView constructor. Selecting a DOM node in the Elements tab currently generates *six* calls to NavigationBar.updateLayout (three for each deselect/select in the DOM tree content view). Fixing this issue will reduce the number to just two. 1136683 1 webkit-bug-importer 2015-10-26 16:19:23 -0700 <rdar://problem/23267238> 1136685 2 264093 mattbaker 2015-10-26 16:26:45 -0700 Created attachment 264093 [Patch] Proposed Fix 1136687 3 264093 timothy 2015-10-26 16:34:05 -0700 Comment on attachment 264093 [Patch] Proposed Fix Nice find! We might be able to keep this code as-is and just remove the loop where we walk the prototype chain and just do: dispatch(this.constructor); I think that will do the same thing, and just take advantage of JS's inheritance. I'm not sure why I didn't think of that before, unless this wasn't broken until we switched to classes with proper inheritance. 1136690 4 mattbaker 2015-10-26 16:38:52 -0700 (In reply to comment #3) > Comment on attachment 264093 [details] > [Patch] Proposed Fix > > Nice find! > > We might be able to keep this code as-is and just remove the loop where we > walk the prototype chain and just do: dispatch(this.constructor); That would be a nice simplification. I'll try it out. > I think that will do the same thing, and just take advantage of JS's > inheritance. I'm not sure why I didn't think of that before, unless this > wasn't broken until we switched to classes with proper inheritance. Interesting... 1136691 5 264093 timothy 2015-10-26 16:39:51 -0700 Comment on attachment 264093 [Patch] Proposed Fix Nevermind! That approach wouldn't work, this is the correct fix. My idea wont allow proper bubbling to all the classes. It would only catch event listeners registered on the concrete class of the instance, not the superclasses 1136717 6 264093 commit-queue 2015-10-26 17:26:35 -0700 Comment on attachment 264093 [Patch] Proposed Fix Clearing flags on attachment: 264093 Committed r191615: <http://trac.webkit.org/changeset/191615> 1136718 7 commit-queue 2015-10-26 17:26:39 -0700 All reviewed patches have been landed. Closing bug. 264093 2015-10-26 16:26:45 -0700 2015-10-26 17:26:35 -0700 [Patch] Proposed Fix bug-150579-20151026162557.patch text/plain 2353 mattbaker U3VidmVyc2lvbiBSZXZpc2lvbjogMTkxNjA0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCA2 MTlkMzljOGFiYTkyZTlhZjBlNmFkNWQwNzEwYTA1Mjg5OGQ5ZDU0Li45ODc5NjY4YWIwZDVhYzY5 ODljNWU5YTFhZjk2ZTE1YmI0NDZlY2RkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOSBAQAorMjAxNS0xMC0yNiAgTWF0dCBCYWtlciAgPG1hdHRiYWtlckBhcHBsZS5jb20+ CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogV2ViSW5zcGVjdG9yLk9iamVjdCBjYW4gZGlzcGF0 Y2ggY29uc3RydWN0b3ItbGV2ZWwgZXZlbnRzIG11bHRpcGxlIHRpbWVzCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTA1NzkKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBVc2UgYGhhc093blByb3BlcnR5YCB3 aGVuIGNoZWNraW5nIGZvciBjb25zdHJ1Y3RvciBldmVudCBsaXN0ZW5lcnMgd2hlbiB3YWxraW5n IHRoZQorICAgICAgICBwcm90b3R5cGUgY2hhaW4uIFRoaXMgcHJldmVudHMgbGlzdGVuZXJzIHJl Z2lzdGVyZWQgd2l0aCBhIGJhc2UgY2xhc3MgY29uc3RydWN0b3IKKyAgICAgICAgZnJvbSBiZWlu ZyBkaXNwYXRjaGVkIG11bHRpcGxlIHRpbWVzIGJ5IGdldHRpbmcgcGlja2VkIHVwIGhpZ2hlciBp biB0aGUgcHJvdG90eXBlIGNoYWluLgorCisgICAgICAgICogVXNlckludGVyZmFjZS9CYXNlL09i amVjdC5qczoKKyAgICAgICAgKFdlYkluc3BlY3Rvci5PYmplY3QucHJvdG90eXBlLmRpc3BhdGNo RXZlbnRUb0xpc3RlbmVycy5kaXNwYXRjaCk6CisgICAgICAgIChXZWJJbnNwZWN0b3IuT2JqZWN0 LnByb3RvdHlwZS5kaXNwYXRjaEV2ZW50VG9MaXN0ZW5lcnMpOgorICAgICAgICAoV2ViSW5zcGVj dG9yLk9iamVjdCk6CisKIDIwMTUtMTAtMjQgIE5pa2l0YSBWYXNpbHlldiAgPG52YXNpbHlldkBh cHBsZS5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgcm9sbGluZyBvdXQgcjE5MTQxOS4KZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL0Jhc2UvT2JqZWN0 LmpzIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvQmFzZS9PYmplY3QuanMK aW5kZXggMjJhMzk4M2UzNTNhYjY1ZWQ3ZGU0MWIxMzcxZWRkYzhlNDM1ZTIyZS4uOTc5NTY5ZTZm YWYzNjBhMzQwZWY4M2I1NzlmYmU0MDFhNzhjZjM3NCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYklu c3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvQmFzZS9PYmplY3QuanMKKysrIGIvU291cmNlL1dlYklu c3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvQmFzZS9PYmplY3QuanMKQEAgLTE0MCwxMSArMTQwLDE1 IEBAIFdlYkluc3BlY3Rvci5PYmplY3QgPSBjbGFzcyBXZWJJbnNwZWN0b3JPYmplY3QKIAogICAg ICAgICBmdW5jdGlvbiBkaXNwYXRjaChvYmplY3QpCiAgICAgICAgIHsKLSAgICAgICAgICAgIGlm ICghb2JqZWN0IHx8ICFvYmplY3QuX2xpc3RlbmVycyB8fCAhb2JqZWN0Ll9saXN0ZW5lcnNbZXZl bnRUeXBlXSB8fCBldmVudC5fc3RvcHBlZFByb3BhZ2F0aW9uKQorICAgICAgICAgICAgaWYgKCFv YmplY3QgfHwgIW9iamVjdC5oYXNPd25Qcm9wZXJ0eSgiX2xpc3RlbmVycyIpIHx8IGV2ZW50Ll9z dG9wcGVkUHJvcGFnYXRpb24pCisgICAgICAgICAgICAgICAgcmV0dXJuOworCisgICAgICAgICAg ICBsZXQgbGlzdGVuZXJzRm9yVGhpc0V2ZW50ID0gb2JqZWN0Ll9saXN0ZW5lcnNbZXZlbnRUeXBl XTsKKyAgICAgICAgICAgIGlmICghbGlzdGVuZXJzRm9yVGhpc0V2ZW50KQogICAgICAgICAgICAg ICAgIHJldHVybjsKIAogICAgICAgICAgICAgLy8gTWFrZSBhIGNvcHkgd2l0aCBzbGljZSBzbyBt dXRhdGlvbnMgZHVyaW5nIHRoZSBsb29wIGRvZXNuJ3QgYWZmZWN0IHVzLgotICAgICAgICAgICAg dmFyIGxpc3RlbmVyc0ZvclRoaXNFdmVudCA9IG9iamVjdC5fbGlzdGVuZXJzW2V2ZW50VHlwZV0u c2xpY2UoMCk7CisgICAgICAgICAgICBsaXN0ZW5lcnNGb3JUaGlzRXZlbnQgPSBsaXN0ZW5lcnNG b3JUaGlzRXZlbnQuc2xpY2UoMCk7CiAKICAgICAgICAgICAgIC8vIEl0ZXJhdGUgb3ZlciB0aGUg bGlzdGVuZXJzIGFuZCBjYWxsIHRoZW0uIFN0b3AgaWYgc3RvcFByb3BhZ2F0aW9uIGlzIGNhbGxl ZC4KICAgICAgICAgICAgIGZvciAodmFyIGkgPSAwOyBpIDwgbGlzdGVuZXJzRm9yVGhpc0V2ZW50 Lmxlbmd0aDsgKytpKSB7Cg==