Bug 164804 – REGRESSION (r192344): Web Inspector: Turning off Code Coverage or Type Profiler logs an error

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

[Error] Error in getting basic block locations: The VM does not currently have a Control Flow Profiler.
	(anonymous function) (BasicBlockAnnotator.js:61)
	(anonymous function)
	_dispatchResponseToCallback (Connection.js:146)
	_dispatchResponse (Connection.js:116)
	dispatch (Connection.js:69)
	dispatch (InspectorBackend.js:151)
	dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:42)

https://github.com/WebKit/webkit/blob/fa99d39839021e1855d14ac7d05d7771b1bb66a6/Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js#L61

Besides the logged error, everything continues working as expected.

[Radar WebKit Bug Importer]

<rdar://problem/29278028>

[Nikita Vasilyev]

It looks like this.insertAnnotations gets called one last time after clicking on the [c] button.

The timeout is usually successfully canceled by Annotator#clear. It could be a race condition somewhere.

[Nikita Vasilyev]

Steps:
1. Open https://webkit.org/
2. Open Resources tab.
3. Select global.js.
4. Click on the [c] icon to enable Code Coverage.
5. Reload the page.
6. Click on the [c] icon to disable Code Coverage.

Notes:
When reloading a page, Web Inspector may, for a fraction of a second, display some "ghost" JS resource that is different from the one that was selected before page reload.

BasicBlockAnnotator gets created for that resource but, somehow, it doesn't get cleared properly. I'm investigating.

[Nikita Vasilyev]

(In reply to comment #3)
> Notes:
> When reloading a page, Web Inspector may, for a fraction of a second,
> display some "ghost" JS resource that is different from the one that was
> selected before page reload.
> 
> BasicBlockAnnotator gets created for that resource but, somehow, it doesn't
> get cleared properly. I'm investigating.

This hypothesis seems to be correct. I added console.trace("SourceCodeTextEditor %s", sourceCode._url) to SourceCodeTextEditor. I opened https://webkit.org/, selected global.js in the Resources, and reloaded the page. Here's the output:

[Log] Trace: SourceCodeTextEditor https://webkit.org/
	SourceCodeTextEditor (SourceCodeTextEditor.js:30)
	TextResourceContentView (TextResourceContentView.js:34)
	responseContentView (ResourceClusterContentView.js:69:82)
	_showContentViewForIdentifier (ResourceClusterContentView.js:217)
	restoreFromCookie (ResourceClusterContentView.js:139)
	_restoreFromCookie (BackForwardEntry.js:105)
	prepareToShow (BackForwardEntry.js:82)
	_showEntry (ContentViewContainer.js:497)
	showBackForwardEntryForIndex (ContentViewContainer.js:162)
	showContentView (ContentViewContainer.js:138)
	showDefaultContentViewForTreeElement (NavigationSidebarPanel.js:218:83)
	delayedWork (ResourceSidebarPanel.js:271)
	delayedWork

[Log] Trace: SourceCodeTextEditor null
	SourceCodeTextEditor (SourceCodeTextEditor.js:30)
	ScriptContentView (ScriptContentView.js:48)
	createFromRepresentedObject (ContentView.js:55)
	contentViewForRepresentedObject (ContentView.js:173:83)
	showDefaultContentViewForTreeElement (NavigationSidebarPanel.js:213:83)
	_addScript (DebuggerSidebarPanel.js:546)
	_scriptAdded (DebuggerSidebarPanel.js:523)
	dispatch (Object.js:170)
	dispatchEventToListeners (Object.js:177)
	scriptDidParse (DebuggerManager.js:696)
	scriptParsed (DebuggerObserver.js:53)
	dispatchEvent (InspectorBackend.js:287)
	_dispatchEvent (Connection.js:193)
	dispatch (Connection.js:71)
	dispatch (InspectorBackend.js:151)
	dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:42)

[Log] Trace: SourceCodeTextEditor https://webkit.org/wp-content/themes/webkit/scripts/global.js?ver=1.0
	SourceCodeTextEditor (SourceCodeTextEditor.js:30)
	TextResourceContentView (TextResourceContentView.js:34)
	responseContentView (ResourceClusterContentView.js:69:82)
	_showContentViewForIdentifier (ResourceClusterContentView.js:217)
	restoreFromCookie (ResourceClusterContentView.js:139)
	_restoreFromCookie (BackForwardEntry.js:105)
	prepareToShow (BackForwardEntry.js:82)
	_showEntry (ContentViewContainer.js:497)
	showBackForwardEntryForIndex (ContentViewContainer.js:162)
	showContentView (ContentViewContainer.js:138)
	showDefaultContentViewForTreeElement (NavigationSidebarPanel.js:218:83)
	_checkElementsForPendingViewStateCookie (NavigationSidebarPanel.js:776)
	_treeElementAddedOrChanged (NavigationSidebarPanel.js:633)
	dispatch (Object.js:170)
	dispatchEventToListeners (Object.js:177)
	insertChild (TreeOutline.js:227)
	_insertChildTreeElement (FolderizedTreeElement.js:253)
	_addTreeElement (FolderizedTreeElement.js:223)
	addChildForRepresentedObject (FolderizedTreeElement.js:110)
	onpopulate (FrameTreeElement.js:154)
	expand (TreeElement.js:373)
	shouldRefreshChildren (TreeElement.js:180)
	_populateFromNewChildQueue (FolderizedTreeElement.js:187)
	_populateFromNewChildQueue

Besides the expected call for global.js, there're two other calls. showDefaultContentViewForTreeElement showed a resource different from the previously selected one.

[Nikita Vasilyev]

Switching from one resource to another, generally, clears Annotator's timeouts:

[Log] Trace: _clearTimeoutIfNeeded
Script {_listeners: null, _originalRevision: SourceCodeRevision, _currentRevision: SourceCodeRevision, _sourceMaps: null, _formatterSourceMap: null, …}
	_clearTimeoutIfNeeded (Annotator.js:105)
	pause (Annotator.js:54)
	hidden (SourceCodeTextEditor.js:155)
	hidden (TextResourceContentView.js:114)
	prepareToHide (BackForwardEntry.js:95)
	_hideEntry (ContentViewContainer.js:509)
	hidden (ContentViewContainer.js:387)
	hidden (ClusterContentView.js:74)
	prepareToHide (BackForwardEntry.js:95)
	_hideEntry (ContentViewContainer.js:509)
	showBackForwardEntryForIndex (ContentViewContainer.js:161)
	showContentView (ContentViewContainer.js:138)
	showRepresentedObject (ContentBrowserTabContentView.js:185)
	showRepresentedObject (Main.js:1146)
	_treeSelectionDidChange (ResourceSidebarPanel.js:420)
	dispatch (Object.js:170)
	dispatchEventToListeners (Object.js:177)
	select (TreeElement.js:507)
	selectOnMouseDown (TreeElement.js:464)
	treeElementMouseDown (TreeElement.js:276)

This, however, doesn't happen for a resource selected by showDefaultContentViewForTreeElement. It doesn't create a back forward entry and it doesn't call TextResourceContentView#hidden.

[Nikita Vasilyev]

The problem is that we instantiate BasicBlockAnnotator for a script that is never shown.

I'm experimenting with instantiating BasicBlockAnnotator in SourceCodeTextEditor#shown instead of SourceCodeTextEditor#_proceedPopulateWithContent.

[Joseph Pecoraro]

(In reply to comment #6)
> The problem is that we instantiate BasicBlockAnnotator for a script that is
> never shown.
> 
> I'm experimenting with instantiating BasicBlockAnnotator in
> SourceCodeTextEditor#shown instead of
> SourceCodeTextEditor#_proceedPopulateWithContent.

Could you clarify what you mean here? A SourceCodeTextEditor is created, presumably it is shown. If its `shown` is not called, would calling it, and ensuring it gets the normal shown/hidden life cycle be a suitable fix?

If `shown` is not getting called, it seems like just changing where the Annotator is created, might fix this but paste over other issues that would be solved by a proper `shown`/`hidden` life cycle.

[Nikita Vasilyev]

(In reply to comment #6)
> The problem is that we instantiate BasicBlockAnnotator for a script that is
> never shown.
> 
> I'm experimenting with instantiating BasicBlockAnnotator in
> SourceCodeTextEditor#shown instead of
> SourceCodeTextEditor#_proceedPopulateWithContent.

This didn't fix anything. `shown` IS in fact getting called.

The problem is that `hidden` isn't getting called.

In ContentViewContainer#showBackForwardEntryForIndex there's this code:

            if (previousEntry)
                this._hideEntry(previousEntry);

If we don't store a resource as a backForwardEntry, it doesn't get `hidden`.

[Nikita Vasilyev]

(In reply to comment #8)
> (In reply to comment #6)
> > The problem is that we instantiate BasicBlockAnnotator for a script that is
> > never shown.
> > 
> > I'm experimenting with instantiating BasicBlockAnnotator in
> > SourceCodeTextEditor#shown instead of
> > SourceCodeTextEditor#_proceedPopulateWithContent.
> 
> This didn't fix anything. `shown` IS in fact getting called.
> 
> The problem is that `hidden` isn't getting called.
> 
> In ContentViewContainer#showBackForwardEntryForIndex there's this code:
> 
>             if (previousEntry)
>                 this._hideEntry(previousEntry);
> 
> If we don't store a resource as a backForwardEntry, it doesn't get `hidden`.

My assumption was wrong. We do store a resource as a backForwardEntry, but on a ContentViewContainer of an inactive tab. A ContentViewContainer that was never visible. In my case, Debugger tab's ContentViewContainer (I had Resources tab selected). Here's where this ContentViewContainer was instantiated:

    [e-mail]:37:14
    [e-mail]:37:75
    [e-mail]:35:61
    [e-mail]:34:14
    [e-mail]:528:24
    [e-mail]:454:63
    [native code]

[Nikita Vasilyev]

This regressed 12 months ago in https://trac.webkit.org/changeset/192344.
Bug 151149: Web Inspector: blank debugger tab when opening inspector on unvisited website

[Nikita Vasilyev]

Created attachment 295237 [details]
Patch

In the future, we may want to make our view code less prone to errors related to visibility.

[Matt Baker]

Comment on attachment 295237 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295237&action=review

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:475
> +        }

It might be good to break this out into a helper function since it's used identically in two places.

[Matt Baker]

Should this check be performed in NavigationSidebarPanel.prototype.showDefaultContentViewForTreeElement? Potential side-effects should be considered, but it seems that not showing content views when the sidebar's content browser is hidden is the right thing to do.

[Nikita Vasilyev]

Created attachment 295971 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 295971 [details]
Patch

Clearing flags on attachment: 295971

Committed r209257: <http://trac.webkit.org/changeset/209257>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.