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>
Created attachment 97347 [details] Patch
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()
(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.
Created attachment 97372 [details] Patch with better names
Change committed with revision 88982.