When attaching subviews, View should be checking incoming views for dirty flag and updating dirtyDescendantsCount if necessary (and scheduling a layout). Right now in this case, the view is marked as dirty but will never be scheduled because the dirty flag is set but the parent doesn't think there's any work to do. I added a workaround in NewTabContentView, you can uncomment it to see this bug in action.
<rdar://problem/23763582>
Created attachment 266658 [details] [Video] steps to repro
Created attachment 266667 [details] [Patch] Proposed Fix
Comment on attachment 266667 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=266667&action=review r=me > Source/WebInspectorUI/UserInterface/Views/View.js:185 > + let isDescendedFromRoot = false; Rename: isDescendantOfRoot > Source/WebInspectorUI/UserInterface/Views/View.js:196 > + // If the view is not a descendant of the root view, switch to a synchronous layout. "If the view is not attached to the main view tree, ..." > Source/WebInspectorUI/UserInterface/Views/View.js:200 > + parentView._dirtyDescendantsCount--; I can't imagine when this will do anything, since by definition its parents will be synchronously laid out too.
(In reply to comment #4) > Comment on attachment 266667 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=266667&action=review > > r=me > > > Source/WebInspectorUI/UserInterface/Views/View.js:185 > > + let isDescendedFromRoot = false; > > Rename: isDescendantOfRoot > > > Source/WebInspectorUI/UserInterface/Views/View.js:196 > > + // If the view is not a descendant of the root view, switch to a synchronous layout. > > "If the view is not attached to the main view tree, ..." > > > Source/WebInspectorUI/UserInterface/Views/View.js:200 > > + parentView._dirtyDescendantsCount--; > > I can't imagine when this will do anything, since by definition its parents > will be synchronously laid out too. It's parents will not be laid out. The result will be a subtree that has no dirty views, but which has non-zero dirty descendant counts. This has no side effects, since once the subtree is attached to the root, the counts will be reset during the next layout pass. The intent here is just to make sure the numbers are correct in order to avoid any confusion while debugging future view issues.
Created attachment 266675 [details] [Patch] Proposed Fix
Comment on attachment 266675 [details] [Patch] Proposed Fix Clearing flags on attachment: 266675 Committed r193486: <http://trac.webkit.org/changeset/193486>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 151904
Comment on attachment 266675 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=266675&action=review > Source/WebInspectorUI/UserInterface/Views/View.js:204 > + view._layoutSubtree(); What we were seeing in the Timeline infinite Recursion was: > [Error] RangeError: Maximum call stack size exceeded. > layout - TimelineOverview > _layoutSubtree > _scheduleLayoutForView > needsLayout > scrollStartTime > revealMarker > layout - TimelineOverview > ... We probably should not force a synchronous layout when detached, and instead defer the layout until it gets attached (added a subview).
The correct solution is to defer layouts for detached subviews, as noted. However, the infinite recursion should never have been possible. Investigating the problem uncovered a fragile pattern used in many timeline classes (and elsewhere). See https://bugs.webkit.org/show_bug.cgi?id=151910.
Created attachment 266770 [details] [Patch] Proposed Fix
Comment on attachment 266770 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=266770&action=review r=me > Source/WebInspectorUI/ChangeLog:20 > + Notify the view when it becomes, or is no longer, descended from the root. 'the root view' > Source/WebInspectorUI/ChangeLog:22 > + Notify the view when it's added to, or removed from, a parent. 'a parent view' > Source/WebInspectorUI/UserInterface/Views/View.js:187 > + let pendingLayouts = this._dirtyDescendantsCount; Nit: pendingLayoutsCount > Source/WebInspectorUI/UserInterface/Views/View.js:230 > + if (!view._isAttachedToRoot) { I would add a comment here: "Don't schedule layout of the view unless it is a descendant of the root view. When it moves to a rooted view tree, force an initial layout." > Source/WebInspectorUI/UserInterface/Views/View.js:243 > let cancelledLayouts = view._dirtyDescendantsCount; Nit: cancelledLayoutsCount
Created attachment 267050 [details] [Patch] Proposed Fix
Comment on attachment 267050 [details] [Patch] Proposed Fix Clearing flags on attachment: 267050 Committed r193872: <http://trac.webkit.org/changeset/193872>