Created attachment 211698 [details] 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.
<rdar://problem/14994748>
Looks like stuff Joe was working on recently for the download button.
Patch looks good. I'm going to try to reproduce, confirm the patch works, and get it reviewed. Thanks for finding this!
(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.
Created attachment 211811 [details] [PATCH] Brian's fix with ChangeLog
Comment on attachment 211811 [details] [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.
(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".
Comment on attachment 211811 [details] [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.
Comment on attachment 211811 [details] [PATCH] Brian's fix with ChangeLog Clearing flags on attachment: 211811 Committed r155896: <http://trac.webkit.org/changeset/155896>
All reviewed patches have been landed. Closing bug.