Bug 74111 – Some overlay scrollbar API calls in ScrollAnimatorMac can lead to an assertion in RenderBox::mapAbsoluteToLocalPoint

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

https://bugs.webkit.org/show_bug.cgi?id=73348 Fixed an extremely reproducible case of this bug, but it is still possible to encounter the assertion. (Sample backtrace below.) Basically, if we call into AppKit to update overlay scrollbar information during layout, then AppKit may call back into WebKit while a layout is still happening via our delegates, and do stuff that we would rather not happen during layout. Sam, Simon, and I discussed this extensively yesterday, and we think that a good way to fix this would be to add zero-delay timer for all of the AppKit calls that can be called during layout. Specifically, that would be the calls in:

ScrollAnimatorMac::notifyPositionChanged()
ScrollAnimatorMac::contentsResized()
…and possibly the add/remove scrollbar functions. (Those functions can definitely be called during a layout, but I don't think the AppKit-related work they do is troublesome.)

See also bug 71049.

[Ion Rosca]

(In reply to comment #1)
> See also bug 81607.

[Ion Rosca]

Created attachment 144815 [details]
Fix + manual test that asserts

I've moved notifyPositionChanged to a zero-delay timer as Beth has suggested.
This assert cannot be fired when running layout tests, however  the html I added as manual test should crash at load time on a debug lion build.

[Simon Fraser (smfr)]

Comment on attachment 144815 [details]
Fix + manual test that asserts

View in context: https://bugs.webkit.org/attachment.cgi?id=144815&action=review

> ManualTests/scrollbar-crash-on-hide-scrolled-area.html:5
> +    <meta charset="utf-8"/>
> +    <title>QuickStart</title>

Neither seem necessary

> ManualTests/scrollbar-crash-on-hide-scrolled-area.html:33
> +    <br /><br /><br />Manual repro: scroll down and click on <b>Next step</b><br /><br /><br /><br /><br /><br />
> +    <br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br />
> +    <br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br />
> +    <br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br />
> +    <br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br />
> +    <br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br />
> +    <br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br />

Ick. Can this testase be simplified, maybe by just adding css height to an element?

[Ion Rosca]

There are some layout test failures regarding scrollbars. I'll try to figure out what the problem is and I'll come up with another patch.

[Ion Rosca]

Created attachment 145022 [details]
Fix 2 + manual test that asserts

[Simon Fraser (smfr)]

Comment on attachment 145022 [details]
Fix 2 + manual test that asserts

View in context: https://bugs.webkit.org/attachment.cgi?id=145022&action=review

> ManualTests/scrollbar-crash-on-hide-scrolled-area.html:33
> +    <a href="#" class=â"nextStepButton" onclick="document.getElementById('toHide').style.display='none'">âNext stepâ</a>â

There are still some non-ascii characters here. The testcase could be cleaned further.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1239
> +void ScrollAnimatorMac::startNotifyContentAreaScrolledTimer()
> +{
> +    m_notifyContentAreaScrolledTimer.startOneShot(0);
> +}

It would be slightly cleaner to move the isActive check in here, and rename this method to 'sendContentAreaScrolledSoon' or something. Then you don't need the notifyContentAreaScrolledTimerIsActive() method, which has a confusing name anyway.

[Ion Rosca]

Created attachment 146238 [details]
Fix 3 + manual test

[Simon Fraser (smfr)]

Comment on attachment 146238 [details]
Fix 3 + manual test

View in context: https://bugs.webkit.org/attachment.cgi?id=146238&action=review

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1238
> +    if(!m_sendContentAreaScrolledTimer.isActive())

Missing space after the if

[Ion Rosca]

Created attachment 146497 [details]
One more space

[WebKit Review Bot]

Comment on attachment 146497 [details]
One more space

Clearing flags on attachment: 146497

Committed r119834: <http://trac.webkit.org/changeset/119834>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.