121382 2013-09-15 03:17:29 -0700 Web Inspector: TypeError when updating ResourceTreeElement created in strange order 2013-09-16 13:23:13 -0700 1 1 1 Unclassified WebKit Web Inspector 528+ (Nightly build) All All RESOLVED FIXED http://www.pixastic.com/labs/digg_attack/ InRadar P2 Normal --- 0 burg webkit-unassigned commit-queue graouts joepeck timothy webkit-bug-importer oldest_to_newest 929404 0 211698 burg 2013-09-15 03:17:29 -0700 Created attachment 211698 trivial fix for missing instanceof check Repro: Go to any web page with the inspector open. Then, navigate to http://www.pixastic.com/labs/digg_attack/. The inspector will encounter a TypeError. [Error] TypeError: undefined is not a function (evaluating 'this.updateStatusForMainFrame()') _updateStatus (ResourceTreeElement.js, line 188) dispatch (Object.js, line 180) dispatchEventToListeners (Object.js, line 187) markAsFinished (Resource.js, line 593) resourceRequestDidFinishLoading (FrameResourceManager.js, line 320) loadingFinished (NetworkObserver.js, line 58) dispatch (InspectorBackend.js, line 262) dispatchNextQueuedMessageFromBackend (Main.js, line 298) (anonymous function) Analysis: It appears this happens only on navigating to this page, not on reloading it. When reloaded, a ResourceTreeElement for the main frame resource is created (triggered by adding breakpoints, IIRC), then a FrameTreeElement for the main frame resource is created later. In the navigation case, the instantiation order is reversed for some reason; since the main frame is already established by this later time, the guard inside ResourceTreeElement._updateStatus succeeds despite the constructor being incorrect. Fix: I was able to avoid this error by adding an instanceof check to the guard. A trivial patch (with some previously missed code cleanup) is attached; I'm busy this weekend and won't have time to update my integration checkout, rebase, make changelog, etc until later this week. Feel free to commit it, or not. 929405 1 webkit-bug-importer 2013-09-15 03:17:42 -0700 <rdar://problem/14994748> 929475 2 timothy 2013-09-15 11:48:41 -0700 Looks like stuff Joe was working on recently for the download button. 929823 3 joepeck 2013-09-16 11:13:57 -0700 Patch looks good. I'm going to try to reproduce, confirm the patch works, and get it reviewed. Thanks for finding this! 929833 4 joepeck 2013-09-16 11:38:29 -0700 (In reply to comment #3) > Patch looks good. I'm going to try to reproduce, confirm the patch works, and get it reviewed. Thanks for finding this! I wasn't able to reproduce the original uncaught exception. But the patch is good regardless. 929834 5 211811 joepeck 2013-09-16 11:41:26 -0700 Created attachment 211811 [PATCH] Brian's fix with ChangeLog 929848 6 211811 darin 2013-09-16 11:57:38 -0700 Comment on attachment 211811 [PATCH] Brian's fix with ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=211811&action=review > Source/WebInspectorUI/UserInterface/ResourceTreeElement.js:-175 > - _updateStatusWithMainFrameButtons: function() > - { > - if (this._reloadButton) { > - this.status = this._reloadButton; > - return; > - } > - > - if (!this._loadingMainFrameButtons) { > - this._loadingMainFrameButtons = true; > - var tooltip = WebInspector.UIString("Reload page (%s)\nReload ignoring cache (%s)").format(WebInspector._reloadPageKeyboardShortcut.displayName, WebInspector._reloadPageIgnoringCacheKeyboardShortcut.displayName); > - wrappedSVGDocument("Images/Reload.svg", "reload-button", tooltip, function(svgDocument) { > - this._reloadButton = svgDocument; > - this._reloadButton.addEventListener("click", this._reloadPageClicked); > - this.status = this._reloadButton; > - delete this._loadingMainFrameButtons; > - }.bind(this)); > - } > - }, Change log doesn’t mentioned this deletion. 929851 7 joepeck 2013-09-16 12:02:58 -0700 (In reply to comment #6) > (From update of attachment 211811 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211811&action=review > > > Source/WebInspectorUI/UserInterface/ResourceTreeElement.js:-175 > > - _updateStatusWithMainFrameButtons: function() > > - { > > - if (this._reloadButton) { > > - this.status = this._reloadButton; > > - return; > > - } > > - > > - if (!this._loadingMainFrameButtons) { > > - this._loadingMainFrameButtons = true; > > - var tooltip = WebInspector.UIString("Reload page (%s)\nReload ignoring cache (%s)").format(WebInspector._reloadPageKeyboardShortcut.displayName, WebInspector._reloadPageIgnoringCacheKeyboardShortcut.displayName); > > - wrappedSVGDocument("Images/Reload.svg", "reload-button", tooltip, function(svgDocument) { > > - this._reloadButton = svgDocument; > > - this._reloadButton.addEventListener("click", this._reloadPageClicked); > > - this.status = this._reloadButton; > > - delete this._loadingMainFrameButtons; > > - }.bind(this)); > > - } > > - }, > > Change log doesn’t mentioned this deletion. I guess that is a prepare-ChangeLog issue. This function is unused code that should have been removed when we switched to "updateStatusForMainFrame". 929886 8 211811 timothy 2013-09-16 12:59:44 -0700 Comment on attachment 211811 [PATCH] Brian's fix with ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=211811&action=review >>> Source/WebInspectorUI/UserInterface/ResourceTreeElement.js:-175 >>> - }, >> >> Change log doesn’t mentioned this deletion. > > I guess that is a prepare-ChangeLog issue. This function is unused code that should have been removed when we switched to "updateStatusForMainFrame". prepare-ChangeLog has never called out removed functions. That is something you need to manual do. 929896 9 211811 commit-queue 2013-09-16 13:23:11 -0700 Comment on attachment 211811 [PATCH] Brian's fix with ChangeLog Clearing flags on attachment: 211811 Committed r155896: <http://trac.webkit.org/changeset/155896> 929897 10 commit-queue 2013-09-16 13:23:13 -0700 All reviewed patches have been landed. Closing bug. 211698 2013-09-15 03:17:29 -0700 2013-09-15 03:17:29 -0700 trivial fix for missing instanceof check fix-instanceof.patch text/plain 2414 burg ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1Jlc291cmNl VHJlZUVsZW1lbnQuanMgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9SZXNv dXJjZVRyZWVFbGVtZW50LmpzCmluZGV4IDAxNzU0MjYuLjJjMzhmMzUgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1Jlc291cmNlVHJlZUVsZW1lbnQuanMK KysrIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvUmVzb3VyY2VUcmVlRWxl bWVudC5qcwpAQCAtMTUxLDQ0ICsxNTEsMjUgQEAgV2ViSW5zcGVjdG9yLlJlc291cmNlVHJlZUVs ZW1lbnQucHJvdG90eXBlID0gewogCiAgICAgICAgIGlmIChvbGRNYWluVGl0bGUgIT09IHRoaXMu bWFpblRpdGxlKQogICAgICAgICAgICAgdGhpcy5jYWxsRmlyc3RBbmNlc3RvckZ1bmN0aW9uKCJk ZXNjZW5kYW50UmVzb3VyY2VUcmVlRWxlbWVudE1haW5UaXRsZURpZENoYW5nZSIsIFt0aGlzLCBv bGRNYWluVGl0bGVdKTsKICAgICB9LAogCiAgICAgLy8gUHJpdmF0ZQogCi0gICAgX3VwZGF0ZVN0 YXR1c1dpdGhNYWluRnJhbWVCdXR0b25zOiBmdW5jdGlvbigpCi0gICAgewotICAgICAgICBpZiAo dGhpcy5fcmVsb2FkQnV0dG9uKSB7Ci0gICAgICAgICAgICB0aGlzLnN0YXR1cyA9IHRoaXMuX3Jl bG9hZEJ1dHRvbjsKLSAgICAgICAgICAgIHJldHVybjsKLSAgICAgICAgfQotCi0gICAgICAgIGlm ICghdGhpcy5fbG9hZGluZ01haW5GcmFtZUJ1dHRvbnMpIHsKLSAgICAgICAgICAgIHRoaXMuX2xv YWRpbmdNYWluRnJhbWVCdXR0b25zID0gdHJ1ZTsKLSAgICAgICAgICAgIHZhciB0b29sdGlwID0g V2ViSW5zcGVjdG9yLlVJU3RyaW5nKCJSZWxvYWQgcGFnZSAoJXMpXG5SZWxvYWQgaWdub3Jpbmcg Y2FjaGUgKCVzKSIpLmZvcm1hdChXZWJJbnNwZWN0b3IuX3JlbG9hZFBhZ2VLZXlib2FyZFNob3J0 Y3V0LmRpc3BsYXlOYW1lLCBXZWJJbnNwZWN0b3IuX3JlbG9hZFBhZ2VJZ25vcmluZ0NhY2hlS2V5 Ym9hcmRTaG9ydGN1dC5kaXNwbGF5TmFtZSk7Ci0gICAgICAgICAgICB3cmFwcGVkU1ZHRG9jdW1l bnQoIkltYWdlcy9SZWxvYWQuc3ZnIiwgInJlbG9hZC1idXR0b24iLCB0b29sdGlwLCBmdW5jdGlv bihzdmdEb2N1bWVudCkgewotICAgICAgICAgICAgICAgIHRoaXMuX3JlbG9hZEJ1dHRvbiA9IHN2 Z0RvY3VtZW50OwotICAgICAgICAgICAgICAgIHRoaXMuX3JlbG9hZEJ1dHRvbi5hZGRFdmVudExp c3RlbmVyKCJjbGljayIsIHRoaXMuX3JlbG9hZFBhZ2VDbGlja2VkKTsKLSAgICAgICAgICAgICAg ICB0aGlzLnN0YXR1cyA9IHRoaXMuX3JlbG9hZEJ1dHRvbjsKLSAgICAgICAgICAgICAgICBkZWxl dGUgdGhpcy5fbG9hZGluZ01haW5GcmFtZUJ1dHRvbnM7Ci0gICAgICAgICAgICB9LmJpbmQodGhp cykpOwotICAgICAgICB9Ci0gICAgfSwKLQogICAgIF91cGRhdGVTdGF0dXM6IGZ1bmN0aW9uKCkK ICAgICB7CiAgICAgICAgIGlmICh0aGlzLl9yZXNvdXJjZS5mYWlsZWQpCiAgICAgICAgICAgICB0 aGlzLmFkZENsYXNzTmFtZShXZWJJbnNwZWN0b3IuUmVzb3VyY2VUcmVlRWxlbWVudC5GYWlsZWRT dHlsZUNsYXNzTmFtZSk7CiAgICAgICAgIGVsc2UKICAgICAgICAgICAgIHRoaXMucmVtb3ZlQ2xh c3NOYW1lKFdlYkluc3BlY3Rvci5SZXNvdXJjZVRyZWVFbGVtZW50LkZhaWxlZFN0eWxlQ2xhc3NO YW1lKTsKIAogICAgICAgICBpZiAodGhpcy5fcmVzb3VyY2UuZmluaXNoZWQgfHwgdGhpcy5fcmVz b3VyY2UuZmFpbGVkKSB7CiAgICAgICAgICAgICAvLyBSZW1vdmUgdGhlIHNwaW5uZXIgYW5kIHJl cGxhY2Ugd2l0aCBhIHJlbG9hZCBidXR0b24gaW4gY2FzZSBpdCdzIHRoZSBtYWluIGZyYW1lJ3Mg bWFpbiByZXNvdXJjZS4KICAgICAgICAgICAgIHZhciBmcmFtZSA9IHRoaXMuX3Jlc291cmNlLnBh cmVudEZyYW1lOwotICAgICAgICAgICAgaWYgKHRoaXMuX3Jlc291cmNlLmlzTWFpblJlc291cmNl KCkgJiYgZnJhbWUgJiYgZnJhbWUuaXNNYWluRnJhbWUoKSkKKyAgICAgICAgICAgIGlmICh0aGlz Ll9yZXNvdXJjZS5pc01haW5SZXNvdXJjZSgpICYmIGZyYW1lICYmIGZyYW1lLmlzTWFpbkZyYW1l KCkgJiYgdGhpcyBpbnN0YW5jZW9mIFdlYkluc3BlY3Rvci5GcmFtZVRyZWVFbGVtZW50KQogICAg ICAgICAgICAgICAgIHRoaXMudXBkYXRlU3RhdHVzRm9yTWFpbkZyYW1lKCk7CiAgICAgICAgICAg ICBlbHNlCiAgICAgICAgICAgICAgICAgdGhpcy5zdGF0dXMgPSBudWxsOwogICAgICAgICB9IGVs c2UgewogICAgICAgICAgICAgdmFyIHNwaW5uZXIgPSBuZXcgV2ViSW5zcGVjdG9yLkluZGV0ZXJt aW5hdGVQcm9ncmVzc1NwaW5uZXI7CiAgICAgICAgICAgICB0aGlzLnN0YXR1cyA9IHNwaW5uZXIu ZWxlbWVudDsKICAgICAgICAgfQo= 211811 2013-09-16 11:41:26 -0700 2013-09-16 13:23:11 -0700 [PATCH] Brian's fix with ChangeLog brrian.patch text/plain 2736 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDVlZGY3MWIuLjIwODUxNGEgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkluc3BlY3Rv clVJL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEzLTA5LTE2ICBKb3NlcGggUGVjb3Jh cm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogVHlwZUVy cm9yIHdoZW4gdXBkYXRpbmcgUmVzb3VyY2VUcmVlRWxlbWVudCBjcmVhdGVkIGluIHN0cmFuZ2Ug b3JkZXIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEy MTM4MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFBh dGNoIGJ5IEJyaWFuIEJ1cmcuIEZpeCBhbiB1bmNhdWdodCBleGNlcHRpb24gdGhhdCBjYW4gaGFw cGVuIG9uIG5hdmlnYXRpb25zLgorCisgICAgICAgICogVXNlckludGVyZmFjZS9SZXNvdXJjZVRy ZWVFbGVtZW50LmpzOgorICAgICAgICAoV2ViSW5zcGVjdG9yLlJlc291cmNlVHJlZUVsZW1lbnQu cHJvdG90eXBlLl91cGRhdGVTdGF0dXMpOgorCiAyMDEzLTA5LTEzICBHdXN0YXZvIE5vcm9uaGEg U2lsdmEgIDxndXN0YXZvLm5vcm9uaGFAY29sbGFib3JhLmNvbT4KIAogICAgICAgICBbR1RLXSBN b3ZlIHRvIHRoZSBuZXcgd2ViIGluc3BlY3RvcgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkluc3Bl Y3RvclVJL1VzZXJJbnRlcmZhY2UvUmVzb3VyY2VUcmVlRWxlbWVudC5qcyBiL1NvdXJjZS9XZWJJ bnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1Jlc291cmNlVHJlZUVsZW1lbnQuanMKaW5kZXggMDE3 NTQyNi4uMmMzOGYzNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRl cmZhY2UvUmVzb3VyY2VUcmVlRWxlbWVudC5qcworKysgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkv VXNlckludGVyZmFjZS9SZXNvdXJjZVRyZWVFbGVtZW50LmpzCkBAIC0xNTUsMjUgKzE1NSw2IEBA IFdlYkluc3BlY3Rvci5SZXNvdXJjZVRyZWVFbGVtZW50LnByb3RvdHlwZSA9IHsKIAogICAgIC8v IFByaXZhdGUKIAotICAgIF91cGRhdGVTdGF0dXNXaXRoTWFpbkZyYW1lQnV0dG9uczogZnVuY3Rp b24oKQotICAgIHsKLSAgICAgICAgaWYgKHRoaXMuX3JlbG9hZEJ1dHRvbikgewotICAgICAgICAg ICAgdGhpcy5zdGF0dXMgPSB0aGlzLl9yZWxvYWRCdXR0b247Ci0gICAgICAgICAgICByZXR1cm47 Ci0gICAgICAgIH0KLQotICAgICAgICBpZiAoIXRoaXMuX2xvYWRpbmdNYWluRnJhbWVCdXR0b25z KSB7Ci0gICAgICAgICAgICB0aGlzLl9sb2FkaW5nTWFpbkZyYW1lQnV0dG9ucyA9IHRydWU7Ci0g ICAgICAgICAgICB2YXIgdG9vbHRpcCA9IFdlYkluc3BlY3Rvci5VSVN0cmluZygiUmVsb2FkIHBh Z2UgKCVzKVxuUmVsb2FkIGlnbm9yaW5nIGNhY2hlICglcykiKS5mb3JtYXQoV2ViSW5zcGVjdG9y Ll9yZWxvYWRQYWdlS2V5Ym9hcmRTaG9ydGN1dC5kaXNwbGF5TmFtZSwgV2ViSW5zcGVjdG9yLl9y ZWxvYWRQYWdlSWdub3JpbmdDYWNoZUtleWJvYXJkU2hvcnRjdXQuZGlzcGxheU5hbWUpOwotICAg ICAgICAgICAgd3JhcHBlZFNWR0RvY3VtZW50KCJJbWFnZXMvUmVsb2FkLnN2ZyIsICJyZWxvYWQt YnV0dG9uIiwgdG9vbHRpcCwgZnVuY3Rpb24oc3ZnRG9jdW1lbnQpIHsKLSAgICAgICAgICAgICAg ICB0aGlzLl9yZWxvYWRCdXR0b24gPSBzdmdEb2N1bWVudDsKLSAgICAgICAgICAgICAgICB0aGlz Ll9yZWxvYWRCdXR0b24uYWRkRXZlbnRMaXN0ZW5lcigiY2xpY2siLCB0aGlzLl9yZWxvYWRQYWdl Q2xpY2tlZCk7Ci0gICAgICAgICAgICAgICAgdGhpcy5zdGF0dXMgPSB0aGlzLl9yZWxvYWRCdXR0 b247Ci0gICAgICAgICAgICAgICAgZGVsZXRlIHRoaXMuX2xvYWRpbmdNYWluRnJhbWVCdXR0b25z OwotICAgICAgICAgICAgfS5iaW5kKHRoaXMpKTsKLSAgICAgICAgfQotICAgIH0sCi0KICAgICBf dXBkYXRlU3RhdHVzOiBmdW5jdGlvbigpCiAgICAgewogICAgICAgICBpZiAodGhpcy5fcmVzb3Vy Y2UuZmFpbGVkKQpAQCAtMTg0LDcgKzE2NSw3IEBAIFdlYkluc3BlY3Rvci5SZXNvdXJjZVRyZWVF bGVtZW50LnByb3RvdHlwZSA9IHsKICAgICAgICAgaWYgKHRoaXMuX3Jlc291cmNlLmZpbmlzaGVk IHx8IHRoaXMuX3Jlc291cmNlLmZhaWxlZCkgewogICAgICAgICAgICAgLy8gUmVtb3ZlIHRoZSBz cGlubmVyIGFuZCByZXBsYWNlIHdpdGggYSByZWxvYWQgYnV0dG9uIGluIGNhc2UgaXQncyB0aGUg bWFpbiBmcmFtZSdzIG1haW4gcmVzb3VyY2UuCiAgICAgICAgICAgICB2YXIgZnJhbWUgPSB0aGlz Ll9yZXNvdXJjZS5wYXJlbnRGcmFtZTsKLSAgICAgICAgICAgIGlmICh0aGlzLl9yZXNvdXJjZS5p c01haW5SZXNvdXJjZSgpICYmIGZyYW1lICYmIGZyYW1lLmlzTWFpbkZyYW1lKCkpCisgICAgICAg ICAgICBpZiAodGhpcy5fcmVzb3VyY2UuaXNNYWluUmVzb3VyY2UoKSAmJiBmcmFtZSAmJiBmcmFt ZS5pc01haW5GcmFtZSgpICYmIHRoaXMgaW5zdGFuY2VvZiBXZWJJbnNwZWN0b3IuRnJhbWVUcmVl RWxlbWVudCkKICAgICAgICAgICAgICAgICB0aGlzLnVwZGF0ZVN0YXR1c0Zvck1haW5GcmFtZSgp OwogICAgICAgICAgICAgZWxzZQogICAgICAgICAgICAgICAgIHRoaXMuc3RhdHVzID0gbnVsbDsK