[Webkit-unassigned] [Bug 82566] RenderTextControlSingleLine::scrollWidth/Height/Left/Top should not call back to DOM tree

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=82566





--- Comment #20 from Tien-Ren Chen <trchen at chromium.org>  2012-05-21 18:04:31 PST ---
(In reply to comment #19)
> (From update of attachment 137437 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137437&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Calling to Element::scrollWidth/scrolHeight/scrollLeft/scrollTop/
> > +        setScrollLeft/setScrollTop will call
> > +        Document::updateLayoutIgnorePendingStyleSheets, which can trigger
> > +        unexpected layout update.
> 
> How is that possible? When we're calling scrollWidth on RenderTextControlSingleLine via Element::scrollWidth(),
> we should have already called updateLayoutIgnorePendingStylesheets().
> The call to updateLayoutIgnorePendingStyleSheets() here should be no-op. Who is the caller?
> We need to fix that caller to call updateLayoutIgnorePendingStyleSheets first. r- because this is a wrong fix.

scrollWidth() is a virtual function. Anything that calls renderBox->scrollWidth() can potentially go to RenderTextControlSingleLine::scrollWidth().

Why is it a wrong fix when there is no layout test fails? Why query innerTextElement()->scrollWidth() from the DOM tree when there is the cached value in the render tree?

> Do we have a crash report / stack trace for this?

Unfortunately this crash currently only exists in downstream Chrome for Android builds.
http://b.corp.google.com/issue?id=6101477
http://crash.corp.google.com/reportdetail?reportid=098fe31738dfc304
http://b.corp.google.com/issue?id=6237967
http://crash.corp.google.com/reportdetail?reportid=7facd3bd586667d4

The two crashes are very similar in nature. We have a RenderLayer::updateNonFastScrollableRegion function in downstream that collects all scrollable region in a RenderLayer, so the compositor thread will know when to delegate scrolling events to the WebKit thread. The function traverse through the render tree and query RenderBox::canBeScrolledAndHasScrollableArea() which will call scrollWidth(). Then unexpected layout screwed up the tree traversal.

We delayed the call until very last moment of a compositor commit, even after we called WebViewImpl::layout() for a full layout update, but layout can still happen unexpectedly.

As far as I know layout is an iterative operation. The render tree can take multiple layouts to reach to a fixed point. Means that it is virtually impossible to safely traverse the render tree while querying geometries from the RenderObjects, if you allow a RenderObject to re-layout anytime as it wishes.

> > Source/WebCore/ChangeLog:11
> > +        Reviewed by NOBODY (OOPS!).
> 
> This line should appear before the long description.
> 
> > Source/WebCore/ChangeLog:13
> > +        No new tests. No change in behavior.
> 
> There definitely is a behavior change. Please revise.

Okay, we'll find a way to test this, but before that let's determine what is the correct fix.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.