Bug 87028 - Chromium AX: Crash when menulist adds selected option via document.write
Summary: Chromium AX: Crash when menulist adds selected option via document.write
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominic Mazzoni
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-21 10:16 PDT by Dominic Mazzoni
Modified: 2012-05-22 08:23 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.16 KB, patch)
2012-05-21 11:07 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (4.83 KB, patch)
2012-05-21 12:13 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Mazzoni 2012-05-21 10:16:40 PDT
Somewhat obscure, but this happened "in the wild" - if you add a selected option inside of a <select> element using document.write, AccessibilityMenuListPopup::didUpdateActiveOption gets called with the new index before the children of the AccessibilityMenuListPopup get updated, leading to a crash.

I can only reproduce on Chromium because on Mac, the AccessibilityMenuListPopup is ignored if the pop-up is closed. Should repro on Windows in theory, maybe GTK.

Here's the repro - the crash only happens if the AccessibilityMenuListPopup is actually created before the script runs; I can trigger this on Chromium by adding a delay to the JavaScript and tabbing to the control (while accessibility is on) before the script runs. When the script does run it triggers the crash.

<select id="menulist">
    <option>1</option>
    <script src="data:text/javascript,document.write('<option selected>2');"></script>
</select>
Comment 1 Dominic Mazzoni 2012-05-21 11:07:28 PDT
Created attachment 143063 [details]
Patch
Comment 2 chris fleizach 2012-05-21 11:19:28 PDT
Comment on attachment 143063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143063&action=review

> Source/WebCore/rendering/RenderMenuList.cpp:124
> +        document()->axObjectCache()->childrenChanged(this);

what about removeChild?
Comment 3 Dominic Mazzoni 2012-05-21 12:13:58 PDT
Created attachment 143074 [details]
Patch
Comment 4 Dominic Mazzoni 2012-05-21 12:17:44 PDT
(In reply to comment #2)
> what about removeChild?

Good thought, but I couldn't come up with an case where removeChild didn't work. I updated the test to cover removeChild, but it already worked correctly so no more code changes were needed.
Comment 5 chris fleizach 2012-05-21 23:39:51 PDT
Comment on attachment 143074 [details]
Patch

looks good
Comment 6 WebKit Review Bot 2012-05-22 08:23:51 PDT
Comment on attachment 143074 [details]
Patch

Clearing flags on attachment: 143074

Committed r117976: <http://trac.webkit.org/changeset/117976>
Comment 7 WebKit Review Bot 2012-05-22 08:23:55 PDT
All reviewed patches have been landed.  Closing bug.