Bug 62746 – Crash possible when switching scrollbar appearance preference on Mac

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

[Beth Dakin]

A crash is *still* possible when switching the scrollbar appearance on Mac because the current mechanism that is intended to flag ScrollAnimators as being in the page cache or not does not work correctly. Long-term the fix for this is to move the ScrollabelArea HashSet to a more appropriate place -- it's on Page right now, but instead it should be on another class that represents a single web page. Something like the top-level Document. In the meantime, I have a patch that fixes the crash.

<rdar://problem/9323983>

[Beth Dakin]

Created attachment 97347 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 97347 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2258
> +        if (!(*it)->isInPageCache())
> +            (*it)->scrollAnimator()->setIsActive();

Wait, what? Why do we need both ScrollableArea::isInPageCache() and ScrollAnimator::setIsActive()?

> Source/WebCore/platform/ScrollableArea.h:140
> +    virtual bool isInPageCache() const { ASSERT_NOT_REACHED(); return false; }

It seems wrong that ScrollableArea knows anything about the page cache. Maybe make this more generic and flip the sense, like isOnActivePage()

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 97347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97347&action=review
> 
> > Source/WebCore/page/FrameView.cpp:2258
> > +        if (!(*it)->isInPageCache())
> > +            (*it)->scrollAnimator()->setIsActive();
> 
> Wait, what? Why do we need both ScrollableArea::isInPageCache() and ScrollAnimator::setIsActive()?
> 

This is basically the whole source of the crash. And it's why the REAL fix is to move the HashSet away from Page. Page does not correspond to a single "web page," but we crash because this code originally assumed that it did. There can be ScrollableAreas in the HashSet that are in the page cache right now and are NOT a part of the "web page" that is becoming active. The real fix involves re-arhitecting this, but this is a quick fix in the meantime. 

> > Source/WebCore/platform/ScrollableArea.h:140
> > +    virtual bool isInPageCache() const { ASSERT_NOT_REACHED(); return false; }
> 
> It seems wrong that ScrollableArea knows anything about the page cache. Maybe make this more generic and flip the sense, like isOnActivePage()

I like this idea.

[Beth Dakin]

Created attachment 97372 [details]
Patch with better names

[Beth Dakin]

Change committed with revision 88982.