Bug 192388 – Web Inspector: Table selection becomes corrupted when deleting selected cookies

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Summary:
Table selection becomes corrupted when deleting selected cookies.

Steps to Reproduce:
1. Inspector https://daringfireball.net
2. Storage tab > Cookies
3. Select first row, hit delete
  => Cookie removed, next row selected
4. Hit delete

Expected:
  => Cookie removed, next row selected

Actual:
  => The cookie following the selected cookie is removed, and two rows are now selected

Note:
Table adjusts cached row indexes that move as a result of removing rows, but its SelectionController needs to be informed of the change so it can do the same. Instead of calling `didRemoveItem` for each removed index, taking care to enumerate indexes in reverse order, SelectionController should have a `didRemoveItems` method taking an IndexSet. This shields users of SelectionController from having to do the right thing.

[Radar WebKit Bug Importer]

<rdar://problem/46472364>

[Matt Baker]

Created attachment 356619 [details]
Patch

[EWS Watchlist]

Comment on attachment 356619 [details]
Patch

Attachment 356619 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10280853

New failing tests:
inspector/table/table-remove-rows.html

[EWS Watchlist]

Created attachment 356636 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 356619 [details]
Patch

Attachment 356619 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10280784

New failing tests:
inspector/table/table-remove-rows.html

[EWS Watchlist]

Created attachment 356637 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Matt Baker]

Created attachment 356642 [details]
Patch

[Matt Baker]

Created attachment 356687 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 356687 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:14
> +        (WI.SelectionController.prototype.didRemoveItems):
> +        (WI.SelectionController.prototype.didRemoveItem): Deleted.
> +        Replace `didRemoveItem` with a method taking an IndexSet. Calling the
> +        single-index version while iterating over multiple rows in ascending
> +        order is unsafe, a detail best left to the SelectionController.

The bug description was about removing a single item. Is this a drive-by change?

[EWS Watchlist]

Comment on attachment 356687 [details]
Patch

Attachment 356687 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10286551

New failing tests:
webgl/1.0.2/conformance/textures/gl-teximage.html

[EWS Watchlist]

Created attachment 356692 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Nikita Vasilyev]

Comment on attachment 356687 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:228
> +            if (this.hasSelectedItem(index))
> +                this.deselectItem(index);

Could it get called more than once in this loop?
If so, can one didRemoveItems call cause more than one selectionControllerSelectionDidChange?

[Devin Rousso]

Comment on attachment 356687 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:228
>> +                this.deselectItem(index);
> 
> Could it get called more than once in this loop?
> If so, can one didRemoveItems call cause more than one selectionControllerSelectionDidChange?

Along the lines of what Nikita said:

This worries me, because I feel like there's a way for a caller (e.g. `WI.Table`) to remove an item, notify `WI.SelectionController`, who will then tell the caller that an item was removed, only to have the caller then affect something after the removed item.

Table has rows 2 (A) and 3 (B) selected
> row 2 (A) deleted
> row 3 (B) becomes row 2 (B)
> didRemoveItems([2 (A)])
> Table is told to deselect row 2 (A)
> Table deselects row 2 (B)

Tests for cases such as these couldn't hurt :)

I don't want to beat a dead horse, but maybe this would be a good time to revisit the idea of using objects instead of indexes.  I don't think this would be an issue there, as the `WI.SelectionController` would just say to the `WI.Table` "hey you should deselect this object" and `WI.Table` can look and see "oh hey this object isn't there anymore... ignore".

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:343
> -            treeOutline._selectionController.didRemoveItem(childIndex);
> +            treeOutline._selectionController.didRemoveItems(new WI.IndexSet([removedIndex]));

I think we should be doing this inside `_forgetTreeElement`, as I don't see any logic right now that would notify the `WI.SelectionController` when `removeChildren` is called.  Additionally, if I have a parent P and child C selected, but call `removeChild(P)`, I'd also expect C to be removed.

The same is true for `_rememberTreeElement`, as right now only P would trigger a `didInsertItem`.

[Matt Baker]

Comment on attachment 356687 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:343
>> +            treeOutline._selectionController.didRemoveItems(new WI.IndexSet([removedIndex]));
> 
> I think we should be doing this inside `_forgetTreeElement`, as I don't see any logic right now that would notify the `WI.SelectionController` when `removeChildren` is called.  Additionally, if I have a parent P and child C selected, but call `removeChild(P)`, I'd also expect C to be removed.
> 
> The same is true for `_rememberTreeElement`, as right now only P would trigger a `didInsertItem`.

I agree we should move the `didRemoveItems` call into `_forgetTreeElement`. However, this will require a change to `removeChildren`:

removeChildren()
{
    for (let child of this.children) {
        // Deselect, forget, and detach
    }

    this.children = [];    
}

Because this.children isn't updated until after the loop completes, detached elements (lacking parent/sibling references) remain in the tree while `_forgetTreeElement` is called, causing `_indexOfTreeElement` to fail.

To address this we simply need to update the loop as follows:

removeChildren()
{
    while (this.children.length) {
        let child = this.children[0];

        // Deselect, forget, and detach

        this.children.shift();
    }
}

[Matt Baker]

Comment on attachment 356687 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:14
>> +        order is unsafe, a detail best left to the SelectionController.
> 
> The bug description was about removing a single item. Is this a drive-by change?

Not a drive by. The bug was that Table failed to inform its SelectionController about deleted items. The method was updated to take multiple indexes so that Table.prototype._removeRows could do this efficiently and safely.

>>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:228
>>> +                this.deselectItem(index);
>> 
>> Could it get called more than once in this loop?
>> If so, can one didRemoveItems call cause more than one selectionControllerSelectionDidChange?
> 
> Along the lines of what Nikita said:
> 
> This worries me, because I feel like there's a way for a caller (e.g. `WI.Table`) to remove an item, notify `WI.SelectionController`, who will then tell the caller that an item was removed, only to have the caller then affect something after the removed item.
> 
> Table has rows 2 (A) and 3 (B) selected

In practice TreeOutline deselects items before removing, so I don't think this will occur. That said, it is worth considering since this creates the possibility for reentrancy where before there was none. Even if `didRemoveItems` was guaranteed to cause, at most, one selectionControllerSelectionDidChange, this is still a problem.

The simplest solution is to not call selectionControllerSelectionDidChange inside of `didRemoveItems`. In practice this should be fine, since TreeOutline deselects items before removing.

[Matt Baker]

Created attachment 356772 [details]
Patch

[Devin Rousso]

Comment on attachment 356772 [details]
Patch

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

r-, as I think there is another issue with this.

If [1, 2, 3] are selected, and both [1, 2] are removed simultaneously, I'd expect that [3] becomes [1].  This can happen if we had a DOM tree like this:

    <0>
        <1>
            <2></2>
        </1>
    </0>
    <3></3>

If <0> gets removed from the DOM tree via JavaScript, <1> and <2> would get removed from the DOM tree without the frontend/user actually explicitly calling delete.

I'm pretty sure that right now, because we call `_forgetTreeElement` on the parent and _then_ the child, we'd hit the assertion that we couldn't get an index for <1> and <2> since they'd already have been removed from the `WI.TreeOutline`.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:409
> +        // Update the SelectionController before attaching the TreeElement.
> +        // Attaching the TreeElement can cause it to become selected, and
> +        // added to the SelectionController.

NIT: is this comment strictly necessary, or is it mainly for the benefit of future programming, like as a "gotcha"?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:410
> +        let index = this._indexOfTreeElement(element) || 0;

Should we even be calling `didInsertItem` if we're unable to get an index?  You already assert inside `_indexOfTreeElement`, so this seems a bit counter to that expectation.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:422
> +            this._selectionController.didRemoveItems(new WI.IndexSet([index]));

Ditto (>410).

[Matt Baker]

Created attachment 357161 [details]
Patch

[Matt Baker]

Created attachment 357162 [details]
Patch

[EWS Watchlist]

Comment on attachment 357162 [details]
Patch

Attachment 357162 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10373138

New failing tests:
inspector/table/table-remove-rows.html

[EWS Watchlist]

Created attachment 357168 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 357162 [details]
Patch

Attachment 357162 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10373160

New failing tests:
inspector/table/table-remove-rows.html

[EWS Watchlist]

Created attachment 357169 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 357162 [details]
Patch

Attachment 357162 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10373119

New failing tests:
inspector/table/table-remove-rows.html

[EWS Watchlist]

Created attachment 357175 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Matt Baker]

Created attachment 357181 [details]
Patch

[Matt Baker]

Comment on attachment 356772 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:410
>> +        let index = this._indexOfTreeElement(element) || 0;
> 
> Should we even be calling `didInsertItem` if we're unable to get an index?  You already assert inside `_indexOfTreeElement`, so this seems a bit counter to that expectation.

Now that we call this in _rememberTreeElement we should always be able to find an index.

[Matt Baker]

(In reply to Devin Rousso from comment #17)
> Comment on attachment 356772 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356772&action=review
> 
> r-, as I think there is another issue with this.
> 
> If [1, 2, 3] are selected, and both [1, 2] are removed simultaneously, I'd
> expect that [3] becomes [1].  This can happen if we had a DOM tree like this:
> 
>     <0>
>         <1>
>             <2></2>
>         </1>
>     </0>
>     <3></3>
> 
> If <0> gets removed from the DOM tree via JavaScript, <1> and <2> would get
> removed from the DOM tree without the frontend/user actually explicitly
> calling delete.
> 
> I'm pretty sure that right now, because we call `_forgetTreeElement` on the
> parent and _then_ the child, we'd hit the assertion that we couldn't get an
> index for <1> and <2> since they'd already have been removed from the
> `WI.TreeOutline`.

This is fixed by changed to TreeOutline `removeChildren` and `removeChildAtIndex`.

[Matt Baker]

Created attachment 357187 [details]
Patch

[Devin Rousso]

(In reply to Matt Baker from comment #28)
> This is fixed by changed to TreeOutline `removeChildren` and `removeChildAtIndex`.
If that's the case, we should have a test for it.

Also, it seems like your changes to the "inspector/table/table-remove-rows.html" test got lost across patches.  That test seems to still be relevant, so we should add it back.

[Matt Baker]

(In reply to Devin Rousso from comment #30)
> (In reply to Matt Baker from comment #28)
> > This is fixed by changed to TreeOutline `removeChildren` and `removeChildAtIndex`.
> If that's the case, we should have a test for it.

I can look into writing DOMTreeOutline tests when I work on https://webkit.org/b/192044. It should be possible to write layout tests for DOMTreeOutline selection that exercise DOMAgent.removeNode and DOMTreeUpdater.

> Also, it seems like your changes to the
> "inspector/table/table-remove-rows.html" test got lost across patches.  That
> test seems to still be relevant, so we should add it back.

Oops!

[Matt Baker]

Created attachment 357203 [details]
Patch

[Devin Rousso]

Comment on attachment 357203 [details]
Patch

rs=me, please don't forget to write tests in <https://webkit.org/b/192044>.  Indexes seems quite brittle, so I'd like to know for sure what our limitations/expectations are.

[WebKit Commit Bot]

Comment on attachment 357203 [details]
Patch

Clearing flags on attachment: 357203

Committed r239175: <https://trac.webkit.org/changeset/239175>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.