Bug 62746 - Crash possible when switching scrollbar appearance preference on Mac
Summary: Crash possible when switching scrollbar appearance preference on Mac
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: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-06-15 12:31 PDT by Beth Dakin
Modified: 2011-06-15 16:13 PDT (History)
1 user (show)

See Also:


Attachments
Patch (12.30 KB, patch)
2011-06-15 12:48 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch with better names (12.39 KB, patch)
2011-06-15 16:05 PDT, Beth Dakin
simon.fraser: review+
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-06-15 12:31:15 PDT
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>
Comment 1 Beth Dakin 2011-06-15 12:48:18 PDT
Created attachment 97347 [details]
Patch
Comment 2 Simon Fraser (smfr) 2011-06-15 14:24:52 PDT
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()
Comment 3 Beth Dakin 2011-06-15 14:37:06 PDT
(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.
Comment 4 Beth Dakin 2011-06-15 16:05:12 PDT
Created attachment 97372 [details]
Patch with better names
Comment 5 Beth Dakin 2011-06-15 16:13:42 PDT
Change committed with revision 88982.