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.
<rdar://problem/46472364>
Created attachment 356619 [details] Patch
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
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
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
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
Created attachment 356642 [details] Patch
Created attachment 356687 [details] Patch
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?
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
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
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?
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`.
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(); } }
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.
Created attachment 356772 [details] Patch
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).
Created attachment 357161 [details] Patch
Created attachment 357162 [details] Patch
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
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
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
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
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
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
Created attachment 357181 [details] Patch
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.
(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`.
Created attachment 357187 [details] Patch
(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.
(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!
Created attachment 357203 [details] Patch
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.
Comment on attachment 357203 [details] Patch Clearing flags on attachment: 357203 Committed r239175: <https://trac.webkit.org/changeset/239175>
All reviewed patches have been landed. Closing bug.