Bug 216067 – Web Inspector: Uncaught Exception: Missing node for given nodeId

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

[Simon Fraser (smfr)]

Uncaught Exception in Web Inspector.

Steps to Reproduce:
1. What were you doing? Include setup or other preparations to reproduce the exception.
2. Include explicit, accurate, and minimal steps taken. Do not include extraneous or irrelevant steps.
3. What did you expect to have happen? What actually happened?

Uncaught Exceptions:
-----------------------
 - Missing node for given nodeId (at Connection.js:162:29)
    _dispatchResponseToPromise @ Connection.js:162:29
    _dispatchResponse @ Connection.js:124:44
    dispatch @ Connection.js:77:35
    dispatchMessageFromTarget @ TargetManager.js:176:39
    dispatchMessageFromTarget @ TargetObserver.js:47:51
    _dispatchEvent @ Connection.js:210:26
    dispatch @ Connection.js:79:32
    dispatch @ InspectorBackend.js:232:52
    ? @ MessageDispatcher.js:42:34
-----------------------

Notes:
Inspected URL:        https://www.facebook.com/
Loading completed:    true
Frontend User Agent:  Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/605.1.15 (KHTML, like Gecko)

Release MiniBrowser at r266337.

Deleted a node in inspector while inspecting facebook.com stream page.

[Nikita Vasilyev]

I couldn't reproduce it on facebook.com home feed page (when logged in). What is a facebook.com stream page? Is deleting any node throws an exception?

[Nikita Vasilyev]

Never mind, I just reproduced this on facebook.com home feed running ToT WebKit build.

[Radar WebKit Bug Importer]

<rdar://problem/68520144>

[Nikita Vasilyev]

Bisecting. I've reproduced the bug on r248445 (Aug 8, 2019) and I haven't gone further back. It doesn't seem like a recent regression. It is possible though that something has changed recently that makes this error resurface more frequently.

I'm stopping bisecting for now. It's tricky to bisect because the facebook.com home feed is different every time it takes several nodes to remove before the error happens. I'll try to make a reduction page.

[Nikita Vasilyev]

Reduction:
data:text/html;,<div></div>text

Steps:
1. Inspect <div>
2. Press Delete key

Notes:
Exception aside, everything works as expected. After deleting <div>, its parent gets selected.

[Nikita Vasilyev]

Created attachment 408492 [details]
Patch

There are so many places in our codebase where we omit `catch` on promises. When they fail, the uncaught exceptions are often quite useless :(

[Devin Rousso]

Comment on attachment 408492 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:9
> +        Handle rejected promise. `WI.RemoteObject.resolveNode` is expected to fail when the domNode is destroyed.

Why is it that `resolveNode` is even getting called when a node is removed?  I would think that when the node is removed we would automatically select a different node that we know will exist (i.e. not a child), meaning that we shouldn't encounter any errors.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:271
> +            if (domNode.destroyed)
> +                return;

Should this also be done in the other callbacks too?

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:273
> +            console.error("Cannot resolve node.", errorMessage, domNode);

Please use `console.assert(false, ...)` so that this is stripped from shipped software.

If you really want to include error info, please use `WI.reportInternalError` so that if/when this is hit the uncaught exception view is shown, making it much more likely for an engineer to file a bug about it.

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #7)
> Comment on attachment 408492 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408492&action=review
> 
> > Source/WebInspectorUI/ChangeLog:9
> > +        Handle rejected promise. `WI.RemoteObject.resolveNode` is expected to fail when the domNode is destroyed.
> 
> Why is it that `resolveNode` is even getting called when a node is removed? 
> I would think that when the node is removed we would automatically select a
> different node that we know will exist (i.e. not a child), meaning that we
> shouldn't encounter any errors.

This's a good question, indeed.

DOMNodeDetailsSidebarPanel.prototype._refreshProperties is called with a stale `this.domNode`:

    WI.RemoteObject.resolveNode
    --- async ---
    _refreshProperties (DOMNodeDetailsSidebarPanel.js:262)
    layout (DOMNodeDetailsSidebarPanel.js:169)
    _layoutSubtree (View.js:293)
    updateLayout (View.js:159)
    updateLayoutIfNeeded (View.js:167)
    shown (SidebarPanel.js:100)
    selectedSidebarPanel (Sidebar.js:138)
    removeSidebarPanel (Sidebar.js:101)
    showDetailsSidebarPanels (ContentBrowserTabContentView.js:199)
    _contentBrowserCurrentRepresentedObjectsDidChange (ContentBrowserTabContentView.js:323)
    dispatch (Object.js:165)
    dispatchEventToListeners (Object.js:172)
    (anonymous function) (ContentBrowser.js:96)
    _execute (Debouncer.js:132)
    (anonymous function) (Debouncer.js:75)

A combination of removeSidebarPanel (Sidebar.js:101) and showDetailsSidebarPanels (ContentBrowserTabContentView.js:199)
logic causes the selected sidebar to change before setting a new domNode. It should be possible to avoid unnecessary
`resolveNode` call but it looks like this change is going to be fairly involved.

[Nikita Vasilyev]

Created attachment 408560 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 408492 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:271
>> +                return;
> 
> Should this also be done in the other callbacks too?

I removed this from my latest patch as it shouldn't attempt to resolve destroyed nodes any more.

[Devin Rousso]

Comment on attachment 408560 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:180
> +        let selectedSidebarPanelCandidate = null;

NIT: we normally use naming like `sidebarPanelToSelect`

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:265
> +        }).catch((errorMessage) => {

NIT: we normally just use `error`

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:270
> +            console.assert(false, "Cannot resolve node.", errorMessage, domNode);

Are you still able to reliably hit this?

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:88
> +    removeSidebarPanel(sidebarPanelOrIdentifierOrIndex, suppressSelectionChange)

rather than have this as an optional argument, please make this into an `options = {}` so that if we need to add additional optional arguments in the future it's not nearly as difficult

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:99
> +        if (this._selectedSidebarPanel === sidebarPanel && !suppressSelectionChange) {

NIT: I'd put the `!suppressSelectionChange` first so that it avoids the extra work if true

[Nikita Vasilyev]

Comment on attachment 408560 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:270
>> +            console.assert(false, "Cannot resolve node.", errorMessage, domNode);
> 
> Are you still able to reliably hit this?

No. However, defining catch statements for promises helps greatly with debugging. I'd like to keep this one.

[Nikita Vasilyev]

Created attachment 408561 [details]
Patch

[Devin Rousso]

Comment on attachment 408561 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:196
> -                    WI.detailsSidebar.selectedSidebarPanel = sidebarPanel;
> +                    sidebarPanelToSelect = sidebarPanel;

Thinking about it a bit more, I think the old logic here is fine as the logic inside `WI.SidebarPanel.prototype.removeSidebarPanel` only runs if the given `WI.SidebarPanel` is in fact selected, meaning that if this code runs the given `WI.SidebarPanel` will never be selected.  As such, I think we can leave this as is.

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:200
> +                WI.detailsSidebar.removeSidebarPanel(sidebarPanel, {suppressSelectionChange: true});

I think this now means that when a sidebar panel is removed, we always select the first sidebar panel.  Do we want that?  Should we move some of the logic inside `WI.SidebarPanel.prototype.removeSidebarPanel` to here so that we can try to determine the closest remaining `WI.SidebarPanel` once we're done removing all the ones that can no longer `inspect`?

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:268
> +            if (this.domNode !== domNode)
> +                return;

Should this also be done in the other callbacks too?

> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:102
> +        if (!options.suppressSelectionChange && this._selectedSidebarPanel === sidebarPanel) {
> +            let index = this._sidebarPanels.indexOf(sidebarPanel);
>              this.selectedSidebarPanel = this._sidebarPanels[index - 1] || this._sidebarPanels[index + 1] || null;
>          }

Looking at all the callsites, do we even need to have any of this code?  Every caller already has logic to set `selectedSidebarPanel` in the same function that they call `removeSidebarPanel`.

[Nikita Vasilyev]

Comment on attachment 408561 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:196
>> +                    sidebarPanelToSelect = sidebarPanel;
> 
> Thinking about it a bit more, I think the old logic here is fine as the logic inside `WI.SidebarPanel.prototype.removeSidebarPanel` only runs if the given `WI.SidebarPanel` is in fact selected, meaning that if this code runs the given `WI.SidebarPanel` will never be selected.  As such, I think we can leave this as is.

While you're correct, the goal of this change was to make the code easier to follow. When `WI.detailsSidebar.selectedSidebarPanel = ...` is outside of the loop, it's abundantly clear that this's going to run only once.

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:268
>> +                return;
> 
> Should this also be done in the other callbacks too?

The early return is already present in the `then` callback (line 239). By "other callbacks", did you mean in every single WI.RemoteObject.resolveNode?..

>> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:102
>>          }
> 
> Looking at all the callsites, do we even need to have any of this code?  Every caller already has logic to set `selectedSidebarPanel` in the same function that they call `removeSidebarPanel`.

Good point, I'll remove it.
Besides, I think the concept of selecting a neighboring panel is a bit odd — I can't think of a case in Web Inspector when it's more useful than simply selecting the 1st panel.

[Nikita Vasilyev]

Created attachment 409303 [details]
Patch

[Nikita Vasilyev]

Created attachment 409304 [details]
Patch

[Devin Rousso]

Comment on attachment 409304 [details]
Patch

r=me

[EWS]

Committed r267379: <https://trac.webkit.org/changeset/267379>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409304 [details].