Bug 22080 - CRASH at Scrollbar::invalidateRect due to null m_client
Summary: CRASH at Scrollbar::invalidateRect due to null m_client
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Windows XP
: P1 Critical
Assignee: Darin Fisher (:fishd, Google)
URL: http://www.megajogos.com.br/
Keywords:
: 22074 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-11-04 23:49 PST by Darin Fisher (:fishd, Google)
Modified: 2008-11-05 10:41 PST (History)
3 users (show)

See Also:


Attachments
v1 patch (1.26 KB, patch)
2008-11-05 00:36 PST, Darin Fisher (:fishd, Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2008-11-04 23:49:03 PST
CRASH at Scrollbar::invalidateRect due to null m_client

I just updated Chrome to use the latest Scrollbar code, and our distributed reliability test is hitting a crash where m_client is null.  The stack trace looks like so:

[scrollbar.cpp:443] WebCore::Scrollbar::invalidateRect(WebCore::IntRect const &)
[scrollbarthemecomposite.cpp:233] WebCore::ScrollbarThemeComposite::invalidatePart(WebCore::Scrollbar *,WebCore::ScrollbarPart)
[scrollbar.cpp:292] WebCore::Scrollbar::setHoveredPart(WebCore::ScrollbarPart)
[scrollbar.cpp:342] WebCore::Scrollbar::mouseExited()
[eventhandler.cpp:1199] WebCore::EventHandler::handleMouseMoveEvent(WebCore::PlatformMouseEvent const &,WebCore::HitTestResult *)
[eventhandler.cpp:1134] WebCore::EventHandler::mouseMoved(WebCore::PlatformMouseEvent const &)

I'm guessing that there must be a code path that leads to setClient(0) being called on the same Scrollbar that EventHandler's m_lastScrollbarUnderMouse points to.

I suspect that the right fix involves nulling m_lastScrollbarUnderMouse at the right time.
Comment 1 Darin Fisher (:fishd, Google) 2008-11-04 23:51:44 PST
more readable stack...

[scrollbar.cpp:443]
  Scrollbar::invalidateRect(IntRect const&)
[scrollbarthemecomposite.cpp:233]
  ScrollbarThemeComposite::invalidatePart(Scrollbar*, ScrollbarPart)
[scrollbar.cpp:292]
  Scrollbar::setHoveredPart(ScrollbarPart)
[scrollbar.cpp:342]
  Scrollbar::mouseExited()
[eventhandler.cpp:1199]
  EventHandler::handleMouseMoveEvent(PlatformMouseEvent const&, HitTestResult*)
[eventhandler.cpp:1134]
  EventHandler::mouseMoved(PlatformMouseEvent const&)
Comment 2 Darin Fisher (:fishd, Google) 2008-11-05 00:22:01 PST
Our crash data shows that this bug happens frequently on http://www.megajogos.com.br/

I could not manually repro, but one thing I noticed about that site is that it repeatedly creates and destroys a DIV with overflow.  The contents of the widget titled "Megajogos Informa" appears to change over time.  Sometimes the DIV requires scrollbars, and sometimes it does not.

Given the way EventHandler and Scrollbar work (namely that Scrollbar is a reference counted object), I'm beginning to suspect that the right answer is for Scrollbar to null-check all accesses to its m_client.
Comment 3 Darin Fisher (:fishd, Google) 2008-11-05 00:36:22 PST
Created attachment 24906 [details]
v1 patch

Simple null checking.
Comment 4 Alexey Proskuryakov 2008-11-05 04:47:38 PST
See also: bug 22074.
Comment 5 Darin Adler 2008-11-05 09:19:09 PST
Comment on attachment 24906 [details]
v1 patch

+    return m_client ? m_client->isActive() : false;

I usually write those using && rather than ?:, but this seems fine.

r=me
Comment 6 Darin Fisher (:fishd, Google) 2008-11-05 10:23:41 PST
I'm happy to change to &&.  That sounds better to me too.
Comment 7 Darin Fisher (:fishd, Google) 2008-11-05 10:25:52 PST
*** Bug 22074 has been marked as a duplicate of this bug. ***
Comment 8 Darin Fisher (:fishd, Google) 2008-11-05 10:41:41 PST
http://trac.webkit.org/changeset/38130