Bug 74111 - Some overlay scrollbar API calls in ScrollAnimatorMac can lead to an assertion in RenderBox::mapAbsoluteToLocalPoint
Summary: Some overlay scrollbar API calls in ScrollAnimatorMac can lead to an assertio...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-08 11:44 PST by Beth Dakin
Modified: 2012-06-08 08:23 PDT (History)
7 users (show)

See Also:


Attachments
Fix + manual test that asserts (6.65 KB, patch)
2012-05-30 07:58 PDT, Ion Rosca
simon.fraser: review+
Details | Formatted Diff | Diff
Fix 2 + manual test that asserts (6.29 KB, patch)
2012-05-31 02:16 PDT, Ion Rosca
simon.fraser: review-
Details | Formatted Diff | Diff
Fix 3 + manual test (5.78 KB, patch)
2012-06-07 02:38 PDT, Ion Rosca
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
One more space (5.78 KB, patch)
2012-06-08 00:06 PDT, Ion Rosca
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2011-12-08 11:44:31 PST
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.)
Comment 1 mitz 2011-12-25 10:07:22 PST
See also bug 71049.
Comment 2 Ion Rosca 2012-05-25 06:17:30 PDT
(In reply to comment #1)
> See also bug 81607.
Comment 3 Ion Rosca 2012-05-30 07:58:10 PDT
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.
Comment 4 Simon Fraser (smfr) 2012-05-30 09:11:49 PDT
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?
Comment 5 Ion Rosca 2012-05-30 09:40:11 PDT
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.
Comment 6 Ion Rosca 2012-05-31 02:16:50 PDT
Created attachment 145022 [details]
Fix 2 + manual test that asserts
Comment 7 Simon Fraser (smfr) 2012-05-31 08:38:59 PDT
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.
Comment 8 Ion Rosca 2012-06-07 02:38:03 PDT
Created attachment 146238 [details]
Fix 3 + manual test
Comment 9 Simon Fraser (smfr) 2012-06-07 08:40:13 PDT
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
Comment 10 Ion Rosca 2012-06-08 00:06:06 PDT
Created attachment 146497 [details]
One more space
Comment 11 WebKit Review Bot 2012-06-08 08:23:27 PDT
Comment on attachment 146497 [details]
One more space

Clearing flags on attachment: 146497

Committed r119834: <http://trac.webkit.org/changeset/119834>
Comment 12 WebKit Review Bot 2012-06-08 08:23:32 PDT
All reviewed patches have been landed.  Closing bug.