Bug 151876 – Web Inspector: when a marked-dirty subview is attached to a parent View, dirtyDescendantsCount gets out of sync

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

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.

[Radar WebKit Bug Importer]

<rdar://problem/23763582>

[Blaze Burg]

Created attachment 266658 [details]
[Video] steps to repro

[Matt Baker]

Created attachment 266667 [details]
[Patch] Proposed Fix

[Blaze Burg]

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.

[Matt Baker]

(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.

[Matt Baker]

Created attachment 266675 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 266675 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 266675

Committed r193486: <http://trac.webkit.org/changeset/193486>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 151904

[Joseph Pecoraro]

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).

[Matt Baker]

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.

[Matt Baker]

Created attachment 266770 [details]
[Patch] Proposed Fix

[Blaze Burg]

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

[Matt Baker]

Created attachment 267050 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 267050 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 267050

Committed r193872: <http://trac.webkit.org/changeset/193872>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.