Bug 151876 - Web Inspector: when a marked-dirty subview is attached to a parent View, dirtyDescendantsCount gets out of sync
Summary: Web Inspector: when a marked-dirty subview is attached to a parent View, dirt...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on: 151904
Blocks:
  Show dependency treegraph
 
Reported: 2015-12-04 11:08 PST by BJ Burg
Modified: 2015-12-09 15:07 PST (History)
8 users (show)

See Also:


Attachments
[Video] steps to repro (17.72 MB, video/mp4)
2015-12-04 13:58 PST, BJ Burg
no flags Details
[Patch] Proposed Fix (3.31 KB, patch)
2015-12-04 15:32 PST, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (3.30 KB, patch)
2015-12-04 15:59 PST, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (8.22 KB, patch)
2015-12-07 04:10 PST, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (8.69 KB, patch)
2015-12-09 14:54 PST, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2015-12-04 11:08:45 PST
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.
Comment 1 Radar WebKit Bug Importer 2015-12-04 11:09:05 PST
<rdar://problem/23763582>
Comment 2 BJ Burg 2015-12-04 13:58:32 PST
Created attachment 266658 [details]
[Video] steps to repro
Comment 3 Matt Baker 2015-12-04 15:32:41 PST
Created attachment 266667 [details]
[Patch] Proposed Fix
Comment 4 BJ Burg 2015-12-04 15:42:15 PST
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.
Comment 5 Matt Baker 2015-12-04 15:54:17 PST
(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.
Comment 6 Matt Baker 2015-12-04 15:59:54 PST
Created attachment 266675 [details]
[Patch] Proposed Fix
Comment 7 WebKit Commit Bot 2015-12-04 16:12:39 PST
Comment on attachment 266675 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 266675

Committed r193486: <http://trac.webkit.org/changeset/193486>
Comment 8 WebKit Commit Bot 2015-12-04 16:12:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Commit Bot 2015-12-04 19:22:25 PST
Re-opened since this is blocked by bug 151904
Comment 10 Joseph Pecoraro 2015-12-04 19:39:32 PST
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).
Comment 11 Matt Baker 2015-12-04 22:01:38 PST
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.
Comment 12 Matt Baker 2015-12-07 04:10:19 PST
Created attachment 266770 [details]
[Patch] Proposed Fix
Comment 13 BJ Burg 2015-12-09 14:47:43 PST
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
Comment 14 Matt Baker 2015-12-09 14:54:31 PST
Created attachment 267050 [details]
[Patch] Proposed Fix
Comment 15 WebKit Commit Bot 2015-12-09 15:07:32 PST
Comment on attachment 267050 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 267050

Committed r193872: <http://trac.webkit.org/changeset/193872>
Comment 16 WebKit Commit Bot 2015-12-09 15:07:36 PST
All reviewed patches have been landed.  Closing bug.