Summary: | EventHandler references deleted Scrollbar | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||
Component: | Layout and Rendering | Assignee: | Brent Fulgham <bfulgham> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bfulgham, buildbot, commit-queue, mmaxfield, rniwa, simon.fraser, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 141923 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Brent Fulgham
2015-02-23 15:48:19 PST
Test that discovered this problem: ASAN or Guard Malloc run of "scrollbars/overflow-custom-scrollbar-crash.html" Created attachment 247156 [details]
Patch
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.
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
(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. (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. 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 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
(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! :-) 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 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
Created attachment 247177 [details]
Patch v2 (Fix for EWS failures)
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.
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.
Committed r180548: <http://trac.webkit.org/changeset/180548> |