Bug 64256 - REGRESSION(r90552): platform/mac/accessibility/html-slider-indicator.html fails
Summary: REGRESSION(r90552): platform/mac/accessibility/html-slider-indicator.html fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-11 02:14 PDT by Shinya Kawanaka
Modified: 2011-07-11 05:29 PDT (History)
2 users (show)

See Also:


Attachments
Patch (1.77 KB, patch)
2011-07-11 02:26 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (1.76 KB, patch)
2011-07-11 04:38 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2011-07-11 02:14:59 PDT
Accessibility component in range input does not work after this patch:
https://bugs.webkit.org/show_bug.cgi?id=52262
Comment 1 Shinya Kawanaka 2011-07-11 02:26:25 PDT
Created attachment 100256 [details]
Patch
Comment 2 Kent Tamura 2011-07-11 02:33:28 PDT
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 3 Shinya Kawanaka 2011-07-11 03:45:09 PDT
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 4 Kent Tamura 2011-07-11 04:00:09 PDT
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.
Comment 5 Shinya Kawanaka 2011-07-11 04:38:58 PDT
Created attachment 100266 [details]
Patch
Comment 6 Kent Tamura 2011-07-11 04:41:52 PDT
Comment on attachment 100266 [details]
Patch

ok
Comment 7 WebKit Review Bot 2011-07-11 05:28:19 PDT
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 8 WebKit Review Bot 2011-07-11 05:29:50 PDT
Comment on attachment 100266 [details]
Patch

Clearing flags on attachment: 100266

Committed r90737: <http://trac.webkit.org/changeset/90737>
Comment 9 WebKit Review Bot 2011-07-11 05:29:54 PDT
All reviewed patches have been landed.  Closing bug.