Summary: | REGRESSION(r90552): platform/mac/accessibility/html-slider-indicator.html fails | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shinya Kawanaka <shinyak> | ||||||
Component: | Accessibility | Assignee: | 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
Shinya Kawanaka
2011-07-11 02:14:59 PDT
Created attachment 100256 [details]
Patch
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(), ... 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. 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. Created attachment 100266 [details]
Patch
Comment on attachment 100266 [details]
Patch
ok
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. Comment on attachment 100266 [details] Patch Clearing flags on attachment: 100266 Committed r90737: <http://trac.webkit.org/changeset/90737> All reviewed patches have been landed. Closing bug. |