Bug 141931 – EventHandler references deleted Scrollbar

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

The EventHandler class holds a RefPtr of the last Scrollbar used during an event. If the Scrollbar is destroyed (as well as its underlying RenderLayer), the EventHandler keeps the Scrollbar from being deleted. However, the underlying ScrollableArea has already been destroyed at this point, leaving this "m_lastScrollbarUnderMouse" Scrollbar holding a reference to deleted memory. This can happen during EventHandling related to scrollbars, and should be prevented.

Prior to r180474, this was avoided by using a "disconnectFromScrollableArea" method to set the internal ScrollableArea to null, and relying on internal nullptr checks to avoid dereferencing the null value.

But this behavior is not correct either; we should not artificially prolong the life of a Scrollbar simply because of an implementation detail of the EventHandler class.

Instead, EventHandler should be notified when the Scrollbar has been destroyed, and stop trying to use it when this happens.

[Brent Fulgham]

<rdar://problem/19915210>

[Brent Fulgham]

Test that discovered this problem: ASAN or Guard Malloc run of "scrollbars/overflow-custom-scrollbar-crash.html"

[Brent Fulgham]

Created attachment 247156 [details]
Patch

[WebKit Commit Bot]

Attachment 247156 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/Scrollbar.cpp:84:  Wrong number of spaces before statement. (expected: 25)  [whitespace/indent] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Comment on attachment 247156 [details]
Patch

Are there any other places in EventHandler that use m_lastScrollbarUnderMouse that need to be audited to make sure they work with a null pointer? Other than that, the use of WeakPtr looks good. I'll leave it to smfr to review the semantics of this change regarding the lifetimes of EventHandler and Scrollbar

[Brent Fulgham]

(In reply to comment #5)
> Comment on attachment 247156 [details]
> Patch
> 
> Are there any other places in EventHandler that use
> m_lastScrollbarUnderMouse that need to be audited to make sure they work
> with a null pointer? Other than that, the use of WeakPtr looks good. I'll
> leave it to smfr to review the semantics of this change regarding the
> lifetimes of EventHandler and Scrollbar

I don't think so. It was a RefPtr before, so the same possibility of null-ness existed in the earlier code. I'll double check, though.

[Brent Fulgham]

(In reply to comment #4)
> Attachment 247156 [details] did not pass style-queue:
> 
> ERROR: Source/WebCore/platform/Scrollbar.cpp:84:  Wrong number of spaces
> before statement. (expected: 25)  [whitespace/indent] [4]
> Total errors found: 1 in 5 files

This is a strange error message -- this line has the correct number of spaces on it.

[Build Bot]

Comment on attachment 247156 [details]
Patch

Attachment 247156 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5830458965229568

New failing tests:
fast/forms/select/listbox-click-on-scrollbar.html
scrollbars/scrollbar-miss-mousemove-disabled.html
scrollbars/scrollbar-drag-thumb-with-large-content.html
fast/scrolling/scrollbar-mousedown-move-mouseup.html

[Build Bot]

Created attachment 247167 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Brent Fulgham]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 247156 [details]
> > Patch
> > 
> > Are there any other places in EventHandler that use
> > m_lastScrollbarUnderMouse that need to be audited to make sure they work
> > with a null pointer? Other than that, the use of WeakPtr looks good. I'll
> > leave it to smfr to review the semantics of this change regarding the
> > lifetimes of EventHandler and Scrollbar
> 
> I don't think so. It was a RefPtr before, so the same possibility of
> null-ness existed in the earlier code. I'll double check, though.

Well, EWS just told me that I am full of crap, and it definitely was *not* expecting the pointer to be null in a few cases. I'll revise the patch! :-)

[Build Bot]

Comment on attachment 247156 [details]
Patch

Attachment 247156 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5496935326679040

New failing tests:
fast/forms/select/listbox-click-on-scrollbar.html
fast/events/scroll-div-with-prevent-default-in-subframe.html
scrollbars/scrollbar-miss-mousemove-disabled.html

[Build Bot]

Created attachment 247169 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Brent Fulgham]

Created attachment 247177 [details]
Patch v2 (Fix for EWS failures)

[Brent Fulgham]

Comment on attachment 247177 [details]
Patch v2 (Fix for EWS failures)

I had originally written my patch as:

        m_lastScrollbarUnderMouse = setLast ? scrollbar->createWeakPtr() : nullptr;

But this is wrong, because the scrollbar might be null. Previously this wasn't a problem because we were assigning a RefPtr from the original pointer. But we must call 'createWeakPtr' now, which is obviously bad if the pointer we were handed is a nullptr.

We should probably try to make WeakPtr understand this case so others don't make this mistake.

[WebKit Commit Bot]

Attachment 247177 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/Scrollbar.cpp:84:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Committed r180548: <http://trac.webkit.org/changeset/180548>