Index setter of HTMLSelectElement and HTMLOptionsCollection should do nothing if the requested size…

Source/core/html/HTMLSelectElement.cpp

 /*
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2009, 2010, 2011 Apple Inc. All rights reserved.
  *           (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  * Copyright (C) 2010 Google Inc. All rights reserved.
  * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  *
@@ skipping 33 matching lines @@
 #include "core/events/MouseEvent.h"
 #include "core/frame/FrameHost.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/html/FormDataList.h"
 #include "core/html/HTMLFormElement.h"
 #include "core/html/HTMLOptGroupElement.h"
 #include "core/html/HTMLOptionElement.h"
 #include "core/html/forms/FormController.h"
 #include "core/input/EventHandler.h"
+#include "core/inspector/ConsoleMessage.h"
 #include "core/layout/HitTestRequest.h"
 #include "core/layout/HitTestResult.h"
 #include "core/layout/LayoutListBox.h"
 #include "core/layout/LayoutMenuList.h"
 #include "core/layout/LayoutText.h"
 #include "core/layout/LayoutTheme.h"
 #include "core/layout/LayoutView.h"
 #include "core/page/AutoscrollController.h"
 #include "core/page/ChromeClient.h"
 #include "core/page/Page.h"
@@ skipping 396 matching lines @@
     return options()->namedItem(name);
 }
 
 HTMLOptionElement* HTMLSelectElement::item(unsigned index)
 {
     return options()->item(index);
 }
 
 void HTMLSelectElement::setOption(unsigned index, HTMLOptionElement* option, ExceptionState& exceptionState)
 {
-    if (index > maxSelectItems - 1)
-        index = maxSelectItems - 1;
+    if (index >= length() && index >= maxSelectItems) {

[davve, 2015/08/20 07:40:43]

I like that you added the comparison to current length()!

That means that a parser-generated list with more than maxSelectItems (10000)
will not be silently clamped when using these setters.

Does this change in behavior warrant a note in the description perhaps?

[tkent, 2015/08/20 08:05:47]

Yeah, I'll add this to the description.
Shrinking the existing options is not reasonable.  IMO, this 10,000 limitation
itself doesn't make much sense.

+        document().addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel,
+            String::format("A request to update an option at index=%u is ignored.  The maximum length is %u.", index, maxSelectItems)));

[keishi, 2015/08/20 07:39:46]

nit: Other messages seemed to be like "Failed to blah blah", "Blocked blah
blah". Maybe it would be more consistent if the subject was the UserAgent.

[tkent, 2015/08/20 08:05:47]

will do.

+        return;
+    }
     int diff = index - length();
     HTMLOptionElementOrHTMLOptGroupElement element;
     element.setHTMLOptionElement(option);
     HTMLElementOrLong before;
     // Out of array bounds? First insert empty dummies.
     if (diff > 0) {
         setLength(index, exceptionState);
         // Replace an existing entry?
     } else if (diff < 0) {
         before.setHTMLElement(options()->item(index + 1));
         remove(index);
     }
     // Finally add the new element.
     if (!exceptionState.hadException()) {
         add(element, before, exceptionState);
         if (diff >= 0 && option->selected())
             optionSelectionStateChanged(option, true);
     }
 }
 
 void HTMLSelectElement::setLength(unsigned newLen, ExceptionState& exceptionState)
 {
-    if (newLen > maxSelectItems)
+    if (newLen > length() && newLen > maxSelectItems) {
+        document().addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel,
+            String::format("A request to update the option length to %u is ignored.  The maximum length is %u.", newLen, maxSelectItems)));
         return;
+    }
     int diff = length() - newLen;
 
     if (diff < 0) { // Add dummy elements.
         do {
             appendChild(document().createElement(optionTag, false), exceptionState);
             if (exceptionState.hadException())
                 break;
         } while (++diff);
     } else {
         const WillBeHeapVector<RawPtrWillBeMember<HTMLElement>>& items = listItems();
@@ skipping 1432 matching lines @@
 void HTMLSelectElement::detach(const AttachContext& context)
 {
     HTMLFormControlElementWithState::detach(context);
     if (m_popup)
         m_popup->disconnectClient();
     m_popupIsVisible = false;
     m_popup = nullptr;
 }
 
 } // namespace blink