Bug 121382 – Web Inspector: TypeError when updating ResourceTreeElement created in strange order

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Brian Burg]

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.

[Radar WebKit Bug Importer]

<rdar://problem/14994748>

[Timothy Hatcher]

Looks like stuff Joe was working on recently for the download button.

[Joseph Pecoraro]

Patch looks good. I'm going to try to reproduce, confirm the patch works, and get it reviewed. Thanks for finding this!

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

Created attachment 211811 [details]
[PATCH] Brian's fix with ChangeLog

[Darin Adler]

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.

[Joseph Pecoraro]

(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".

[Timothy Hatcher]

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.

[WebKit Commit Bot]

Comment on attachment 211811 [details]
[PATCH] Brian's fix with ChangeLog

Clearing flags on attachment: 211811

Committed r155896: <http://trac.webkit.org/changeset/155896>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.