Summary: | Crash possible when switching scrollbar appearance preference on Mac | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||||
Component: | Layout and Rendering | Assignee: | Beth Dakin <bdakin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bdakin | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Beth Dakin
2011-06-15 12:31:15 PDT
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. |