Bug 64504 – Focus and selection events are not fired when a <select>'s selection changes

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

[Jon Honeycutt]

WebKit should fire focus and selection events when the selected <option> in a <select> element changes.

<rdar://problem/9319881>

[Jon Honeycutt]

Created attachment 100780 [details]
Patch

[Jon Honeycutt]

Created attachment 100957 [details]
Patch v2

Try to fix Chromium build. Changed an AXObjectCache::getOrCreate() call to just get() in RenderMenuList::didUpdateActiveOption() - we don't need to create the object if it doesn't exist.

[Alice Liu]

Comment on attachment 100957 [details]
Patch v2

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

r=me assuming the location of the early return in didSetSelectedIndex is either fine or fixed up.

> Source/WebCore/rendering/RenderMenuList.cpp:353
> +

It looks like didSetSelectedIndex used to set m_lastSelectedIndex even if accessibilityEnabled was false.  Now you're not going to be setting m_lastActiveIndex if accessibility isn't enabled.  I guess that's okay if m_lastActiveIndex is only ever read or written if accessibility is enabled, but i can't tell 100% from this patch.  could you confirm this?

[Jon Honeycutt]

(In reply to comment #3)
> (From update of attachment 100957 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100957&action=review
> 
> r=me assuming the location of the early return in didSetSelectedIndex is either fine or fixed up.
> 
> > Source/WebCore/rendering/RenderMenuList.cpp:353
> > +
> 
> It looks like didSetSelectedIndex used to set m_lastSelectedIndex even if accessibilityEnabled was false.  Now you're not going to be setting m_lastActiveIndex if accessibility isn't enabled.  I guess that's okay if m_lastActiveIndex is only ever read or written if accessibility is enabled, but i can't tell 100% from this patch.  could you confirm this?

Good catch! m_lastSelectedIndex is only used by this accessibility code, so I'll leave it as-is. Thanks for the review!

[Jon Honeycutt]

Landed in <http://trac.webkit.org/changeset/91132>. Build fix in <http://trac.webkit.org/changeset/91135>.

[Ryosuke Niwa]

A bunch of tests seem to have start hitting assertions on Chromium bots after this patch was landed:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20chromium.org&tests=editing%2Fselection%2Fiframe.html%2Cdom%2Fhtml%2Flevel2%2Fhtml%2FHTMLOptGroupElement01.html%2Cdom%2Fhtml%2Flevel2%2Fhtml%2FHTMLOptGroupElement02.html%2Cediting%2Fpasteboard%2F4641033.html%2Cfast%2Fcss%2Fpseudo-required-optional-unapplied.html%2Cfast%2Fevents%2Ftabindex-focus-blur-all.html%2Cfast%2Fforms%2FValidityState-valueMissing-001.html%2Cfast%2Fforms%2Fform-collection-elements.html%2Cfast%2Fforms%2Foption-in-optgroup-removal.html%2Cfast%2Fforms%2Fselect-out-of-bounds-index.html%2Cfast%2Fforms%2Fselect-selected.html%2Cfast%2Fforms%2Fstuff-on-my-optgroup.html%2Cfast%2Finvalid%2Fresidual-style.html%2Cfast%2Fregex%2FtoString.html%2Chtml5lib%2Frunner.html%2Cediting%2Fselection%2Fmove-by-word-visually-others.html%2Cfast%2Fjs%2Ffunction-apply.html%2Cfast%2Fjs%2Freserved-words-as-property.html%2Cjquery%2Fcss.html%2Cjquery%2Fdata.html%2Cstorage%2Findexeddb%2Findex-cursor.html

[Ryosuke Niwa]

Looking at http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug?numbuilds=50
and http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/24311

this patch also caused major failures on GTK :(

[Philippe Normand]

GTK stack trace:


#0  0x00002ae02f0b9016 in WebCore::RenderMenuList::didUpdateActiveOption (this=0xfb9eb28, optionIndex=0) at ../../Source/WebCore/rendering/RenderMenuList.cpp:362
362	    ASSERT(toOptionElement(select->listItems()[optionIndex]));

Thread 1 (Thread 15390):
#0  0x00002ae02f0b9016 in WebCore::RenderMenuList::didUpdateActiveOption (this=0xfb9eb28, optionIndex=0) at ../../Source/WebCore/rendering/RenderMenuList.cpp:362
#1  0x00002ae02f0b801e in WebCore::RenderMenuList::setTextFromOption (this=0xfb9eb28, optionIndex=0) at ../../Source/WebCore/rendering/RenderMenuList.cpp:208
#2  0x00002ae02f0b7ead in WebCore::RenderMenuList::updateFromElement (this=0xfb9eb28) at ../../Source/WebCore/rendering/RenderMenuList.cpp:188
#3  0x00002ae02eade996 in WebCore::SelectElement::setSelectedIndex (data=..., element=0xfb2f2f0, optionIndex=0, deselect=true, fireOnChangeNow=false, userDrivenChange=false) at ../../Source/WebCore/dom/SelectElement.cpp:381
#4  0x00002ae02ec2f0ec in WebCore::HTMLSelectElement::setSelectedIndex (this=0xfb2f2f0, optionIndex=0, deselect=false) at ../../Source/WebCore/html/HTMLSelectElement.cpp:86
#5  0x00002ae02ec28fe3 in WebCore::HTMLOptionElement::insertedIntoTree (this=0xef04990, deep=false) at ../../Source/WebCore/html/HTMLOptionElement.cpp:251
#6  0x00002ae02ea13ec7 in WebCore::ContainerNode::insertedIntoDocument (this=0xef04990) at ../../Source/WebCore/dom/ContainerNode.cpp:784
#7  0x00002ae02ea744a3 in WebCore::Element::insertedIntoDocument (this=0xef04990) at ../../Source/WebCore/dom/Element.cpp:959
#8  0x00002ae02eae7825 in WebCore::StyledElement::insertedIntoDocument (this=0xef04990) at ../../Source/WebCore/dom/StyledElement.cpp:455
#9  0x00002ae02ebf5416 in WebCore::HTMLFormControlElement::insertedIntoDocument (this=0xef04990) at ../../Source/WebCore/html/HTMLFormControlElement.cpp:194
#10 0x00002ae02ea13a40 in WebCore::ContainerNode::parserAddChild (this=0xfb9f180, newChild=...) at ../../Source/WebCore/dom/ContainerNode.cpp:690
#11 0x00002ae02ec527fd in WebCore::HTMLConstructionSite::attach<WebCore::Element> (this=0xc7e9578, rawParent=0xfb9f180, prpChild=...) at ../../Source/WebCore/html/parser/HTMLConstructionSite.cpp:103
#12 0x00002ae02ec50738 in WebCore::HTMLConstructionSite::attachToCurrent (this=0xc7e9578, child=...) at ../../Source/WebCore/html/parser/HTMLConstructionSite.cpp:265
#13 0x00002ae02ec50be2 in WebCore::HTMLConstructionSite::insertHTMLElement (this=0xc7e9578, token=...) at ../../Source/WebCore/html/parser/HTMLConstructionSite.cpp:295
#14 0x00002ae02ec77c32 in WebCore::HTMLTreeBuilder::processStartTag (this=0xc7e9550, token=...) at ../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1429
#15 0x00002ae02ec72547 in WebCore::HTMLTreeBuilder::processToken (this=0xc7e9550, token=...) at ../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:481
#16 0x00002ae02ec723f4 in WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken (this=0xc7e9550, token=...) at ../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:462
#17 0x00002ae02ec72356 in WebCore::HTMLTreeBuilder::constructTreeFromToken (this=0xc7e9550, rawToken=...) at ../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:452
#18 0x00002ae02ec54b14 in WebCore::HTMLDocumentParser::pumpTokenizer (this=0xef07310, mode=WebCore::HTMLDocumentParser::AllowYield) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:276
#19 0x00002ae02ec54532 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible (this=0xef07310, mode=WebCore::HTMLDocumentParser::AllowYield) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:175
#20 0x00002ae02ec55080 in WebCore::HTMLDocumentParser::append (this=0xef07310, source=...) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:367
#21 0x00002ae02ea18e8c in WebCore::DecodedDataDocumentParser::appendBytes (this=0xef07310, writer=0xf150550, data=0xfb2c7c0 "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML//EN\">\n<html>\n<head>\n<title>required and basic valueMissing</title>\n<link rel=\"stylesheet\" href=\"../../fast/js/resources/js-test-style.css\">\n<script src=\"../../"..., length=3821) at ../../Source/WebCore/dom/DecodedDataDocumentParser.cpp:50
#22 0x00002ae02ed923aa in WebCore::DocumentWriter::addData (this=0xf150550, bytes=0xfb2c7c0 "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML//EN\">\n<html>\n<head>\n<title>required and basic valueMissing</title>\n<link rel=\"stylesheet\" href=\"../../fast/js/resources/js-test-style.css\">\n<script src=\"../../"..., length=3821) at ../../Source/WebCore/loader/DocumentWriter.cpp:203
#23 0x00002ae02ed8632b in WebCore::DocumentLoader::commitData (this=0xf150450, bytes=0xfb2c7c0 "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML//EN\">\n<html>\n<head>\n<title>required and basic valueMissing</title>\n<link rel=\"stylesheet\" href=\"../../fast/js/resources/js-test-style.css\">\n<script src=\"../../"..., length=3821) at ../../Source/WebCore/loader/DocumentLoader.cpp:321
#24 0x00002ae02e70c16d in WebKit::FrameLoaderClient::committedLoad (this=0x20ea130, loader=0xf150450, data=0xfb2c7c0 "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML//EN\">\n<html>\n<head>\n<title>required and basic valueMissing</title>\n<link rel=\"stylesheet\" href=\"../../fast/js/resources/js-test-style.css\">\n<script src=\"../../"..., length=3821) at ../../Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:320
#25 0x00002ae02ed86203 in WebCore::DocumentLoader::commitLoad (this=0xf150450, data=0xfb2c7c0 "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML//EN\">\n<html>\n<head>\n<title>required and basic valueMissing</title>\n<link rel=\"stylesheet\" href=\"../../fast/js/resources/js-test-style.css\">\n<script src=\"../../"..., length=3821) at ../../Source/WebCore/loader/DocumentLoader.cpp:307
#26 0x00002ae02ed863e6 in WebCore::DocumentLoader::receivedData (this=0xf150450, data=0xfb2c7c0 "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML//EN\">\n<html>\n<head>\n<title>required and basic valueMissing</title>\n<link rel=\"stylesheet\" href=\"../../fast/js/resources/js-test-style.css\">\n<script src=\"../../"..., length=3821) at ../../Source/WebCore/loader/DocumentLoader.cpp:333
#27 0x00002ae02edd1267 in WebCore::MainResourceLoader::addData (this=0xef06600, data=0xfb2c7c0 "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML//EN\">\n<html>\n<head>\n<title>required and basic valueMissing</title>\n<link rel=\"stylesheet\" href=\"../../fast/js/resources/js-test-style.css\">\n<script src=\"../../"..., length=3821, allAtOnce=false) at ../../Source/WebCore/loader/MainResourceLoader.cpp:168
#28 0x00002ae02eddeaaf in WebCore::ResourceLoader::didReceiveData (this=0xef06600, data=0xfb2c7c0 "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML//EN\">\n<html>\n<head>\n<title>required and basic valueMissing</title>\n<link rel=\"stylesheet\" href=\"../../fast/js/resources/js-test-style.css\">\n<script src=\"../../"..., length=3821, encodedDataLength=3821, allAtOnce=false) at ../../Source/WebCore/loader/ResourceLoader.cpp:284
#29 0x00002ae02edd2696 in WebCore::MainResourceLoader::didReceiveData (this=0xef06600, data=0xfb2c7c0 "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML//EN\">\n<html>\n<head>\n<title>required and basic valueMissing</title>\n<link rel=\"stylesheet\" href=\"../../fast/js/resources/js-test-style.css\">\n<script src=\"../../"..., length=3821, encodedDataLength=3821, allAtOnce=false) at ../../Source/WebCore/loader/MainResourceLoader.cpp:464
#30 0x00002ae02eddf3b4 in WebCore::ResourceLoader::didReceiveData (this=0xef06600, data=0xfb2c7c0 "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML//EN\">\n<html>\n<head>\n<title>required and basic valueMissing</title>\n<link rel=\"stylesheet\" href=\"../../fast/js/resources/js-test-style.css\">\n<script src=\"../../"..., length=3821, encodedDataLength=3821) at ../../Source/WebCore/loader/ResourceLoader.cpp:442
#31 0x00002ae02ef73f1b in WebCore::readCallback (source=0xb05f400, asyncResult=0xa9205e0, data=0x0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:801
#32 0x00002ae032a5db59 in async_ready_callback_wrapper (source_object=0xb05f400, res=0xa9205e0, user_data=0x0) at /tmp/buildd/glib2.0-2.28.6/./gio/ginputstream.c:470
#33 0x00002ae032a6da68 in complete_in_idle_cb_for_thread (_data=0xfb553d0) at /tmp/buildd/glib2.0-2.28.6/./gio/gsimpleasyncresult.c:812
#34 0x00002ae0335db4a3 in g_main_dispatch (context=0x2073a40) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:2440
#35 g_main_context_dispatch (context=0x2073a40) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:3013
#36 0x00002ae0335dbc80 in g_main_context_iterate (context=0x2073a40, block=1, dispatch=1, self=<value optimized out>) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:3091
#37 0x00002ae0335dc2f2 in g_main_loop_run (loop=0xfaeb880) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:3299
#38 0x00002ae0315172b7 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#39 0x000000000042dc00 in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:707
#40 0x000000000042d29d in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:499
#41 0x000000000042f558 in main (argc=2, argv=0x7ffff5b64fd8) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1187

[Philippe Normand]

http://webkit-bots.igalia.com/amd64/svn_91171.core-when_1310958982-_-who_DumpRenderTree-_-why_11.trace.html

is the link to the above stack-trace.

[Ryosuke Niwa]

Per discussion on IRC, we're rolling this patch out: https://bugs.webkit.org/show_bug.cgi?id=64681

Sorry :(

[Ryosuke Niwa]

fast/forms/option-in-optgroup-removal.html

Chromium Windows

ASSERTION FAILED: toOptionElement(select->listItems()[optionIndex])
Backtrace:
	WebCore::RenderMenuList::didUpdateActiveOption [0x02422B1C+220] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\rendering\rendermenulist.cpp:362)
	WebCore::RenderMenuList::setTextFromOption [0x02421D3D+285] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\rendering\rendermenulist.cpp:209)
	WebCore::RenderMenuList::updateFromElement [0x02421C09+169] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\rendering\rendermenulist.cpp:189)
	WebCore::updateFromElementCallback [0x016C0F97+215] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\html\htmlformcontrolelement.cpp:258)
	WebCore::ContainerNode::dispatchPostAttachCallbacks [0x017D9CCE+126] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\dom\containernode.cpp:746)
	WebCore::ContainerNode::resumePostAttachCallbacks [0x017D9ABE+46] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\dom\containernode.cpp:714)
	WebCore::Document::recalcStyle [0x01788758+936] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\dom\document.cpp:1563)
	WebCore::Document::updateStyleIfNeeded [0x01788A38+248] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\dom\document.cpp:1582)
	WebCore::Document::finishedParsing [0x01793752+322] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\dom\document.cpp:4193)
	WebCore::HTMLTreeBuilder::finished [0x017639B4+100] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\html\parser\htmltreebuilder.cpp:2825)
	WebCore::HTMLDocumentParser::end [0x01739503+131] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:379)
	WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd [0x017395F6+182] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:388)
	WebCore::HTMLDocumentParser::prepareToStopParsing [0x017382AC+188] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:152)
	WebCore::HTMLDocumentParser::attemptToEnd [0x01739669+57] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:399)
	WebCore::HTMLDocumentParser::finish [0x017397B3+51] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:427)
	WebCore::DocumentWriter::endIfNotLoadingMainResource [0x010DE429+201] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\documentwriter.cpp:226)
	WebCore::DocumentWriter::end [0x010DE347+39] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\documentwriter.cpp:210)
	WebCore::DocumentLoader::finishedLoading [0x011D2894+84] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\documentloader.cpp:290)
	WebCore::FrameLoader::finishedLoading [0x0110FDC2+82] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:2045)
	WebCore::MainResourceLoader::didFinishLoading [0x01426890+304] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\mainresourceloader.cpp:486)
	WebCore::ResourceLoader::didFinishLoading [0x0141F67B+91] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webcore\loader\resourceloader.cpp:449)
	WebCore::ResourceHandleInternal::didFinishLoading [0x022FB9F0+144] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\source\webkit\chromium\src\resourcehandle.cpp:190)
	webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest [0x00B503E5+533] (e:\b\build\slave\webkit_win__dbg__1_\build\src\webkit\glue\weburlloader_impl.cc:664)
	`anonymous namespace'::RequestProxy::NotifyCompletedRequest [0x00ACED69+57] (e:\b\build\slave\webkit_win__dbg__1_\build\src\webkit\tools\test_shell\simple_resource_loader_bridge.cc:274)
	 [0x00AD6391+33] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\tuple.h:564)
	 [0x00AD4AB5+53] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\task.h:338)
	`anonymous namespace'::TaskClosureAdapter::Run [0x01F07812+50] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:104)
	base::internal::Invoker1<0,base::internal::InvokerStorage1<void (__thiscall `anonymous namespace'::TaskClosureAdapter::*)(void),A0x17a47d7c::TaskClosureAdapter *>,void (__thiscall `anonymous namespace'::TaskClosureAdapter::*)(void)>::DoInvoke [0x01F1280D+45] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\bind_internal.h:595)
	base::Callback<void __cdecl(void)>::Run [0x00E0B47F+47] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\callback.h:265)
	MessageLoop::RunTask [0x01F08E75+293] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:486)
	MessageLoop::DeferOrRunPendingTask [0x01F08FC3+51] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:505)
	MessageLoop::DoWork [0x01F0999D+221] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:693)
	base::MessagePumpForUI::DoRunLoop [0x01F6BBA4+84] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_pump_win.cc:203)
	base::MessagePumpWin::RunWithDispatcher [0x01F6B472+130] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_pump_win.cc:51)
	base::MessagePumpWin::Run [0x01F6B6DC+28] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_pump_win.h:80)
	MessageLoop::RunInternal [0x01F08BC7+247] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:451)
	MessageLoop::RunHandler [0x01F0899E+46] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:425)
	MessageLoop::Run [0x01F082AA+58] (e:\b\build\slave\webkit_win__dbg__1_\build\src\base\message_loop.cc:349)
	webkit_support::RunMessageLoop [0x008E01BF+15] (e:\b\build\slave\webkit_win__dbg__1_\build\src\webkit\support\webkit_support.cc:375)
	TestShell::waitTestFinished [0x0042BCED+141] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\tools\dumprendertree\chromium\testshellwin.cpp:106)
	TestShell::runFileTest [0x0044646F+479] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\tools\dumprendertree\chromium\testshell.cpp:235)
	runTest [0x0041B12D+733] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\tools\dumprendertree\chromium\dumprendertree.cpp:125)
	main [0x0041A928+2232] (e:\b\build\slave\webkit_win__dbg__1_\build\src\third_party\webkit\tools\dumprendertree\chromium\dumprendertree.cpp:248)
	__tmainCRTStartup [0x00A74967+279] (f:\dd\vctools\crt_bld\self_x86\crt\src\crt0.c:266)
	mainCRTStartup [0x00A7483F+15] (f:\dd\vctools\crt_bld\self_x86\crt\src\crt0.c:182)
	RegisterWaitForInputIdle [0x7C817077+73]

[Jon Honeycutt]

Reopening, since this was rolled out. Updated patch coming shortly.

[Jon Honeycutt]

Created attachment 101150 [details]
Patch v3

This should fix the logic error that was causing the assertion failures on the bots - the error being that an option index was used where a list index should've been used. Also note that an incorrect > was replaced with a >=. Here is the relevant diff:


--- a/Source/WebCore/rendering/RenderMenuList.cpp
+++ b/Source/WebCore/rendering/RenderMenuList.cpp
@@ -356,10 +356,11 @@ void RenderMenuList::didUpdateActiveOption(int optionIndex)
     m_lastActiveIndex = optionIndex;

     SelectElement* select = toSelectElement(static_cast<Element*>(node()));
-    if (optionIndex < 0 || optionIndex > static_cast<int>(select->listItems().size()))
+    int listIndex = select->optionToListIndex(optionIndex);
+    if (listIndex < 0 || listIndex >= select->listItems().size())
         return;

-    ASSERT(toOptionElement(select->listItems()[optionIndex]));
+    ASSERT(toOptionElement(select->listItems()[listIndex]));

     if (AccessibilityMenuList* menuList = static_cast<AccessibilityMenuList*>(document()->axObjectCache()->get(this)))
         menuList->didUpdateActiveOption(optionIndex);

[WebKit Review Bot]

Comment on attachment 101150 [details]
Patch v3

Attachment 101150 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9110391

[Jon Honeycutt]

Created attachment 101152 [details]
Patch v4

Fix a signed/unsigned comparison mismatch.

[Jon Honeycutt]

Landed in r91219.