Bug 64256 – REGRESSION(r90552): platform/mac/accessibility/html-slider-indicator.html fails

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Shinya Kawanaka]

Accessibility component in range input does not work after this patch:
https://bugs.webkit.org/show_bug.cgi?id=52262

[Shinya Kawanaka]

Created attachment 100256 [details]
Patch

[Kent Tamura]

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 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 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]

Created attachment 100266 [details]
Patch

[Kent Tamura]

Comment on attachment 100266 [details]
Patch

ok

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 100266 [details]:

http/tests/misc/slow-loading-mask.html bug 64069 (author: [e-mail])
fast/blockflow/japanese-lr-selection.html bug 64264 (author: [e-mail])
fast/blockflow/japanese-rl-selection.html bug 64265 (author: [e-mail])
fast/box-shadow/scaled-box-shadow.html bug 64266 (author: [e-mail])
fast/backgrounds/background-leakage.html bug 64267 (author: [e-mail])
fast/backgrounds/repeat/negative-offset-repeat.html bug 64268 (author: [e-mail])
fast/filesystem/filesystem-uri-origin.html bug 63206 (author: [e-mail])
transforms/2d/hindi-rotated.html bug 64269 (authors: [e-mail] and [e-mail])
svg/repaint/filter-repaint.svg bug 64270 (author: [e-mail])
svg/W3C-SVG-1.1/struct-use-01-t.svg bug 64271 (authors: [e-mail] and [e-mail])
svg/transforms/text-with-mask-with-svg-transform.svg bug 64272 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 100266 [details]
Patch

Clearing flags on attachment: 100266

Committed r90737: <http://trac.webkit.org/changeset/90737>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.