REGRESSION: Elements tab: Uncaught Exception: No node with given id found

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Created attachment 361211 [details]
[HTML] Test Page

-------
Inspected URL:        file:///Users/mattbaker/Desktop/Test%20pages/delete-multiple-dom-nodes.html
Loading completed:    true
Frontend User Agent:  Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_4) AppleWebKit/605.1.15 (KHTML, like Gecko)

Uncaught Exceptions:
 - No node with given id found (at Connection.js:158:29)
    _dispatchResponseToPromise @ Connection.js:158:29
    _dispatchResponse @ Connection.js:120:44
    dispatch @ Connection.js:70:35
    dispatchMessageFromTarget @ TargetManager.js:101:35
    dispatchMessageFromTarget @ TargetObserver.js:42:51
    dispatchEvent @ InspectorBackend.js:340:42
    _dispatchEvent @ Connection.js:195:32
    dispatch @ Connection.js:72:32
    dispatch @ InspectorBackend.js:178:52
    dispatchNextQueuedMessageFromBackend @ MessageDispatcher.js:42:34
-------

* STEPS TO REPRODUCE
1. Inspect test page
2. Elements tab
3. Select and expand DIV "parent-1"
4. Hit delete
  => Uncaught exception

[Radar WebKit Bug Importer]

<rdar://problem/47828647>

[Matt Baker]

Created attachment 363703 [details]
Patch

[Devin Rousso]

Comment on attachment 363703 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:307
> +        this._treeElementsToRemove.sort((a, b) => getLevel(b) - getLevel(a));

Should we sort before calling `removeSelectedItems`?  It should make performance slightly better since it organizes the array to be bottom-up based on the DOM and we check bottom-up inside `canSelectTreeElement`, meaning that the "lower" nodes are more likely to be earlier on in the array.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:330
> +            return super.canSelectTreeElement(treeElement);

Rather than only using this path if we aren't removing, I think we should flip this around.
```
    if (!super.canSelectTreeElement(treeElement))
        return false;

    let willRemoveAncestorOrSelf = false;
    if (this._treeElementsToRemove) {
        while (treeElement && !willRemoveAncestorOrSelf) {
            willRemoveAncestorOrSelf = this._treeElementsToRemove.includes(treeElement);
            treeElement = treeElement.parent;
        }
    }
    return !willRemoveAncestorOrSelf;
```

[Matt Baker]

Created attachment 363803 [details]
Patch for landing

[Matt Baker]

Comment on attachment 363703 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:307
>> +        this._treeElementsToRemove.sort((a, b) => getLevel(b) - getLevel(a));
> 
> Should we sort before calling `removeSelectedItems`?  It should make performance slightly better since it organizes the array to be bottom-up based on the DOM and we check bottom-up inside `canSelectTreeElement`, meaning that the "lower" nodes are more likely to be earlier on in the array.

The sort would need to go in SelectionController, and this is only needed for the DOMTreeOutline. If this path needs to be optimized in the future I will investigate then.

[WebKit Commit Bot]

Comment on attachment 363803 [details]
Patch for landing

Clearing flags on attachment: 363803

Committed r242577: <https://trac.webkit.org/changeset/242577>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.