Bug 22080 – CRASH at Scrollbar::invalidateRect due to null m_client

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

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&)

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

Created attachment 24906 [details]
v1 patch

Simple null checking.

[Alexey Proskuryakov]

See also: bug 22074.

[Darin Adler]

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

[Darin Fisher (:fishd, Google)]

I'm happy to change to &&.  That sounds better to me too.

[Darin Fisher (:fishd, Google)]

*** Bug 22074 has been marked as a duplicate of this bug. ***

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/38130