View layout state is getting corrupted by clearing descendent child counts for a child view's layout being canceled while that view is being laid out.
Created attachment 458562 [details] Patch v1.0
<rdar://problem/92529233>
Comment on attachment 458562 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=458562&action=review > Source/WebInspectorUI/UserInterface/Views/View.js:276 > + this._isInLayout = true; something about this feels wrong/icky to me instead of having another member variable, is there any way to introspect the `_dirtyDescendantsCount` _after_ we've `layout`? i guess i'm not entirely sure what the order of events is for this bug. im not sure i see where/how "Cancelling an in-progress layout" happens
(In reply to Devin Rousso from comment #3) > Comment on attachment 458562 [details] > Patch v1.0 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458562&action=review > > > Source/WebInspectorUI/UserInterface/Views/View.js:276 > > + this._isInLayout = true; > > something about this feels wrong/icky to me > > instead of having another member variable, is there any way to introspect > the `_dirtyDescendantsCount` _after_ we've `layout`? i guess i'm not > entirely sure what the order of events is for this bug. im not sure i see > where/how "Cancelling an in-progress layout" happens This was tricky to work out while I was debugging, but hopefully this helps explain my findings more clearly... Each view has two member variables that track whether we need to be interested in actually laying it out (since all layout starts from the root view). These are `_dirty`, which tracks if the view itself is dirty and needs `layout()` and related functions called, and `_dirtyDescendantsCount`, which tracks how many children of this view are dirty and need layout called. In `WI.View._visitViewTreeForLayout`: during layout if the root view is not `_dirty` but has a non-zero `_dirtyDescendantsCount`, each subview is has the same `_dirty`/`_dirtyDescendantsCount` check performed until we reach a view that is marked as dirty. Once we've identifier a dirty view, we don't check its descendents since the view and all of its content will have `layout()` called. Right before we actual lay out a view, its `_dirty` flag is set to false and its `_dirtyDescendantsCount` is set to zero, since we know it and all of its subviews will be laid out soon. This also allows a subview to mark a superview as needing layout as a result of its own layout (for example, a NavigationItem does this to its parent NavigationBar when it is hidden/shown, since it may mean that more/fewer items have to be collapsed). After performing layout on the dirty view, each subview (and in turn their subviews...) are laid out, which includes the initial step of setting `_dirty` to false and `_dirtyDescendantsCount` to zero. The bug here comes when laying out a view causes itself or one of its subviews to call WI.View.prototype.cancelLayout, which in this bug happens with the following stack trace: cancelLayout (View.js:182) updateLayout (View.js:156) _positionHeaderViews (DataGrid.js:1077) _updateHeaderAndScrollbar (DataGrid.js:948) sizeDidChange (DataGrid.js:942) _layoutSubtree (View.js:287) _layoutSubtree (View.js:309) _layoutSubtree (View.js:309) _layoutSubtree (View.js:309) _layoutSubtree (View.js:309) _visitViewTreeForLayout (View.js:400) --- requestAnimationFrame --- _scheduleLayoutForView (View.js:361) `cancelLayout()` is turn calls `WI.View._cancelScheduledLayoutForView()`, and this is where the bug actually occurs. A count of cancelled layouts is calculated to be the number of dirty descendents (plus one if the view itself was `_dirty`. We then walk the ancestor chain back to the root view and reduce each ancestors `_dirtyDescendantsCount` by the count of cancelled layouts. This logic is sound for views not in middle of being laid out, but in this case we are cancelling a layout that is already in progress which has some unfortunate consequences. Firstly, the remaining subviews will still be laid out anyways and have their `_dirty` flag set to false and their `_dirtyDescendantsCount` set to zero as part of their being laid out. Secondly, parent views have already consumed the `_dirtyDescendantsCount` as part of their layout, which means that we are subtracting some number of dirty descendants from zero. The Math.max(0, ...) smells pretty bad here since it means we are removing a count of more dirty subviews than we believe we have. Thirdly, and most critical to this bug, we may have, as part of laying out views, needed to perform work that invalidated some part of the view hierarchy. In this case, it means that a parent view that has already laid out may have a non-zero `_dirtyDescendantsCount` to reflect that some part of the view hierarchy below it needs additional layout, which will have been scheduled. If we zero out the `_dirtyDescendantsCount`, it means that the dirty descendant may never be reached in `WI.View._visitViewTreeForLayout`, leaving it stuck as `_dirty`, but with no way to reach it until something like a dirtying of the root view (e.g. from a resize of the window) occurs. In this case, that inconsistent state with an "unreachable" (in terms of _dirtyDescendantsCount) dirty view means that the view is stuck `_dirty`, which means even future calls to `needsLayout()` will early-return without walking the ancestor tree to increment their `_dirtyDescendantsCount`s.
Created attachment 458687 [details] Patch v1.1 - Improved accounting approach, removed cancelLayout
Comment on attachment 458687 [details] Patch v1.1 - Improved accounting approach, removed cancelLayout View in context: https://bugs.webkit.org/attachment.cgi?id=458687&action=review > Source/WebInspectorUI/UserInterface/Views/View.js:244 > + console.assert(this._parentView || !(this._isDirty || this._dirtyDescendantsCount)); I think this assertion may be triggered if `view.needsLayout()` and then `parentView.removeSubview(view)`. Style: i think we usually prefer applying the `!` to the inside of the `(` `)` so there's less mental math ``` console.assert(this._parentView || (!this._isDirty && !this._dirtyDescendantsCount)); ``` > Source/WebInspectorUI/UserInterface/Views/View.js:266 > + function recursivelySetNotDirty(view) { NIT: we might want to do this with an array and a `while` just in case we do ever get a deep enough hierarchy that this would cause a "stack too deep" error > Source/WebInspectorUI/UserInterface/Views/View.js:371 > + view._setDirty(true); Hmm, do we need to do this before the `if` above? Otherwise, will `_dirty` ever be set if `needsLayout()` is called on something not yet attached to the root? And in that case, what would happen if the parent of the detached tree is then added to the root? Would we actually crawl the entire tree, or only until the parent of the previously detached tree? Adding something new to _any_ tree should probably mark it as being `_dirty` (and maybe propagate the `_dirtyDescendantsCount` up to the root).
Comment on attachment 458687 [details] Patch v1.1 - Improved accounting approach, removed cancelLayout View in context: https://bugs.webkit.org/attachment.cgi?id=458687&action=review >> Source/WebInspectorUI/UserInterface/Views/View.js:244 >> + console.assert(this._parentView || !(this._isDirty || this._dirtyDescendantsCount)); > > I think this assertion may be triggered if `view.needsLayout()` and then `parentView.removeSubview(view)`. > > Style: i think we usually prefer applying the `!` to the inside of the `(` `)` so there's less mental math > ``` > console.assert(this._parentView || (!this._isDirty && !this._dirtyDescendantsCount)); > ``` This assertion verifies that the view either: has a parent view before moving to the new parent -or- is not dirty in any way (we no longer mark detached trees as dirty as that would be redundant, see :368-371 and my response below). In your example `view` starts with a parent (from which you then remove it), and therefor will not trigger this assertion. This assertion exists to ensure that we aren't needlessly setting the dirty flag/dirtying the subtree before a view is attached. >> Source/WebInspectorUI/UserInterface/Views/View.js:266 >> + function recursivelySetNotDirty(view) { > > NIT: we might want to do this with an array and a `while` just in case we do ever get a deep enough hierarchy that this would cause a "stack too deep" error Good point. >> Source/WebInspectorUI/UserInterface/Views/View.js:371 >> + view._setDirty(true); > > Hmm, do we need to do this before the `if` above? Otherwise, will `_dirty` ever be set if `needsLayout()` is called on something not yet attached to the root? > > And in that case, what would happen if the parent of the detached tree is then added to the root? Would we actually crawl the entire tree, or only until the parent of the previously detached tree? > > Adding something new to _any_ tree should probably mark it as being `_dirty` (and maybe propagate the `_dirtyDescendantsCount` up to the root). This is handled in `_didMoveToParent`, which will call `_scheduleLayoutForView` once the view is actually attached, which means the entire subtree being attached will get laid out at that time. Marking views within that subtree as dirty before it is attached would be redundant. This works because once we encounter a dirty view during layout, all of its subviews are also laid out.
Created attachment 458693 [details] Patch v1.2
Comment on attachment 458693 [details] Patch v1.2 View in context: https://bugs.webkit.org/attachment.cgi?id=458693&action=review r=me, nice fix =D > Source/WebInspectorUI/ChangeLog:29 > + `_dirtyDescendantsCount` and at what times relative to layout. would be awesome to have some tests for this (e.g. calling `updateLayout` inside the `layout` of a parent) > Source/WebInspectorUI/ChangeLog:31 > + The only time in this patch that we do not do a +1/-1 adjustment to the layout count is in the special case of would be awesome to have some tests for this (e.g. ensuring that the accounting is done correctly if a dirty `WI.View` is removed) > Source/WebInspectorUI/ChangeLog:52 > + - A special case of recursively calling `_setDirty(false)` that is able to optimize away the need to walk the Aside: Reading this made me think a bit, and im pretty sure there's actually another unrelated bug. Nowhere in `WI.View` do we check to see if when adding new subviews that the given subview is not already a subview somewhere else. We probably should add a `console.assert(!view.parentView)` somewhere inside `WI.View.prototype.insertSubviewBefore` to catch this (or just add logic to automatically call `view.parentView.removeSubview(view)` in this case). > Source/WebInspectorUI/UserInterface/Views/View.js:251 > + this._setSelfAndDescendantsAttachedToRoot(isAttachedToRoot); It seems like both `_setSelfAndDescendantsNotDirty` and `_setSelfAndDescendantsAttachedToRoot` crawl down the entire view hierarchy underneath the `parentView`. Can we perhaps combine those two methods into a single function (or inlined here) so that we only have to crawl down once? > Source/WebInspectorUI/UserInterface/Views/View.js:268 > + let view = views.shift(); NIT: rather than call `shift()`, which is pretty expensive, can we instead just keep an index (e.g. `for (let i = 0; i < views.length; ++i)`)? We might wanna do that in `WI.View._visitViewTreeForLayout` too 😅
Created attachment 458714 [details] Patch v1.3
Committed r293727 (250215@main): <https://commits.webkit.org/250215@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458714 [details].