Bug 54262 - Some Scrollbar functions assume an attached ScrollableArea but can be called without one
Summary: Some Scrollbar functions assume an attached ScrollableArea but can be called ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Critical
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-10 17:51 PST by Peter Kasting
Modified: 2011-02-13 10:47 PST (History)
1 user (show)

See Also:


Attachments
Patch (2.47 KB, patch)
2011-02-13 10:42 PST, Sam Weinig
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2011-02-10 17:51:54 PST
EventHandler caches |m_lastScrollbarUnderMouse| while the user is interacting with a scrollbar.  This can cause problems if, during this interaction, the page goes through relayout and destroys its scrollbar.

Because |m_lastScrollbarUnderMouse| is a RefPtr, we don't need to worry about it being deleted.  However, in RenderLayer::destroyScrollbar(), scrollbar->disconnectFromScrollableArea() is called before the RenderLayer drops its ref.  As a result, the scrollbar no longer has an attached ScrollableArea.  The next time EventHandler calls e.g. Scrollbar::mouseMoved(), it blindly accesses scrollableArea() and we crash.

I don't know the model here well enough to know whether the right fix is for the Scrollbar to add NULL-checks in several places, or whether instead at the time the scrollbar is dropped from the RenderLayer the EventHandler should be told to drop |m_lastScrollbarUnderMouse|, or perhaps something else.

I don't have a minimal testcase that reproduces this bug, but I imagine a page that either continually flips between needing a scrollbar and not on a timer, or else one that turns of its scrollbar in response to onscroll, might be able to demonstrate this.  Setting up such a page and then grabbing the scroll thumb and dragging around might work.
Comment 1 Peter Kasting 2011-02-10 17:55:59 PST
(BTW, I may not have a _minimal_ testcase, but I do have one, locally, that's Chromium-specific, so I'm happy to test out any patches to fix.)
Comment 2 James Robinson 2011-02-10 18:32:00 PST
FYI this is a leading crasher on recent Chromium builds and does not seem restricted to any particular webpages (there are crash reports from youtube.com, facebook.com, google.*, etc).
Comment 3 Sam Weinig 2011-02-11 10:57:53 PST
I will add null checks to Scrollbar.  That is the correct way to fix this.  I should have a patch ready later today.
Comment 4 Sam Weinig 2011-02-13 10:42:43 PST
Created attachment 82268 [details]
Patch
Comment 5 Sam Weinig 2011-02-13 10:47:03 PST
Fixed in r78431.