Bug 64256

Summary: REGRESSION(r90552): platform/mac/accessibility/html-slider-indicator.html fails
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Shinya Kawanaka
Reported Monday, July 11, 2011 10:14:59 AM UTC
Accessibility component in range input does not work after this patch: https://bugs.webkit.org/show_bug.cgi?id=52262
Attachments
Patch (1.77 KB, patch)
2011-07-11 02:26 PDT, Shinya Kawanaka
no flags
Patch (1.76 KB, patch)
2011-07-11 04:38 PDT, Shinya Kawanaka
no flags
Shinya Kawanaka
Comment 1 Monday, July 11, 2011 10:26:25 AM UTC
Kent Tamura
Comment 2 Monday, July 11, 2011 10:33:28 AM UTC
Comment on attachment 100256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100256&action=review > Source/WebCore/html/RangeInputType.cpp:224 > element()->dispatchFormControlChangeEvent(); > + > + if (AXObjectCache::accessibilityEnabled()) > + element()->document()->axObjectCache()->postNotification(element()->renderer(), AXObjectCache::AXValueChanged, true); > } dispatchFormControlChangeEvent() dispatch an 'change' event, so a JavaScript code runs in it. The JavaScriptCode can delete the parent <input>, and can change the type of the <input>. So accessing element() after dispatchFormControlChangeEvent() causes a use-after-free. You need to protect a reference of element() by RefPtr<> in order to keep <input> alive and in order to avoid 'this' access. RefPtr<HTMLInputELement> input = element(); input->dispatchFormControlChangeEvent(); if (...) input->document()->axObjectCache()->postNotification(input->renderer(), ...
Shinya Kawanaka
Comment 3 Monday, July 11, 2011 11:45:09 AM UTC
Comment on attachment 100256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100256&action=review >> Source/WebCore/html/RangeInputType.cpp:224 >> } > > dispatchFormControlChangeEvent() dispatch an 'change' event, so a JavaScript code runs in it. > The JavaScriptCode can delete the parent <input>, and can change the type of the <input>. So accessing element() after dispatchFormControlChangeEvent() causes a use-after-free. > > You need to protect a reference of element() by RefPtr<> in order to keep <input> alive and in order to avoid 'this' access. > RefPtr<HTMLInputELement> input = element(); > input->dispatchFormControlChangeEvent(); > if (...) > input->document()->axObjectCache()->postNotification(input->renderer(), ... I found that postNotification() is called before dispatchFormControlChangeEvent() in HTMLInputElement::setChecked(). So If it is OK, I would like to move postNotification before dispatchFormControlChangeEvent. Do you think it may also break something? If so, I will use RefPtr to keep it.
Kent Tamura
Comment 4 Monday, July 11, 2011 12:00:09 PM UTC
Comment on attachment 100256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100256&action=review >>> Source/WebCore/html/RangeInputType.cpp:224 >>> } >> >> dispatchFormControlChangeEvent() dispatch an 'change' event, so a JavaScript code runs in it. >> The JavaScriptCode can delete the parent <input>, and can change the type of the <input>. So accessing element() after dispatchFormControlChangeEvent() causes a use-after-free. >> >> You need to protect a reference of element() by RefPtr<> in order to keep <input> alive and in order to avoid 'this' access. >> RefPtr<HTMLInputELement> input = element(); >> input->dispatchFormControlChangeEvent(); >> if (...) >> input->document()->axObjectCache()->postNotification(input->renderer(), ... > > I found that postNotification() is called before dispatchFormControlChangeEvent() in HTMLInputElement::setChecked(). > So If it is OK, I would like to move postNotification before dispatchFormControlChangeEvent. Do you think it may also break something? If so, I will use RefPtr to keep it. It's ok to move because the code without r90552 dispatched AXValueChanged then 'change'. But I'm not confident AXValueChanged notification won't delete the element.
Shinya Kawanaka
Comment 5 Monday, July 11, 2011 12:38:58 PM UTC
Kent Tamura
Comment 6 Monday, July 11, 2011 12:41:52 PM UTC
Comment on attachment 100266 [details] Patch ok
WebKit Review Bot
Comment 7 Monday, July 11, 2011 1:28:19 PM UTC
The commit-queue encountered the following flaky tests while processing attachment 100266 [details]: http/tests/misc/slow-loading-mask.html bug 64069 (author: bdakin@apple.com) fast/blockflow/japanese-lr-selection.html bug 64264 (author: hyatt@apple.com) fast/blockflow/japanese-rl-selection.html bug 64265 (author: hyatt@apple.com) fast/box-shadow/scaled-box-shadow.html bug 64266 (author: simon.fraser@apple.com) fast/backgrounds/background-leakage.html bug 64267 (author: simon.fraser@apple.com) fast/backgrounds/repeat/negative-offset-repeat.html bug 64268 (author: mitz@webkit.org) fast/filesystem/filesystem-uri-origin.html bug 63206 (author: adamk@chromium.org) transforms/2d/hindi-rotated.html bug 64269 (authors: darin@apple.com and jshin@chromium.org) svg/repaint/filter-repaint.svg bug 64270 (author: krit@webkit.org) svg/W3C-SVG-1.1/struct-use-01-t.svg bug 64271 (authors: darin@apple.com and eric@webkit.org) svg/transforms/text-with-mask-with-svg-transform.svg bug 64272 (author: zimmermann@kde.org) The commit-queue is continuing to process your patch.
WebKit Review Bot
Comment 8 Monday, July 11, 2011 1:29:50 PM UTC
Comment on attachment 100266 [details] Patch Clearing flags on attachment: 100266 Committed r90737: <http://trac.webkit.org/changeset/90737>
WebKit Review Bot
Comment 9 Monday, July 11, 2011 1:29:54 PM UTC
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.