HTMLSelectElement does not include selected index/indices while saving state

HTMLSelectElement does not include selected index/indices while saving state

Since index is not part of the FormControl state for select, there is no
way to restore it to proper index if there are duplicate values. This
patch includes the index in the FormControl state for select.

BUG=401506
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181883

[spartha, 2014-09-08 10:05:48]

sudarshan.p@samsung.com changed reviewers:
+ keishi@chromium.org, tkent@chromium.org

[spartha, 2014-09-08 10:06:02]

PTAL!

[tkent, 2014-09-09 02:14:46]

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1042: state.append(String::number(i));
Because you change the format of the state, you need to update
formStateSignature() in FormController.cpp.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1077: // The saved state should have
atleast one value and an index.
atleast -> at least

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1080: size_t index = state[1].toInt();
This code parsing a unsigned value as a signed value.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1082:
toHTMLOptionElement(items[index])->setSelectedState(true);
This has a bad-cast vulnerability if state[0] is an empty string and
items[index] is not an <option>.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1092: const size_t index = state[i +
1].toInt();
parsing a unsigned value as a signed value.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1094:
toHTMLOptionElement(items[index])->setSelectedState(true);
Ditto. bad-cast.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.h (right):

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.h:195: const
WillBeHeapVector<RawPtrWillBeMember<HTMLElement> >& items = listItems();
Don't define function body in a header file if it contains two or more lines.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElementTest.cpp (right):

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElementTest.cpp:41: "<option
value='111'>111</option>"
Indent continuous lines.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElementTest.cpp:50: // Test passes if the restored
state is not changed.
This is a bad test.  The test passes even if restoreFormControlState() does
nothing.

[spartha, 2014-09-09 08:08:05]

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElementTest.cpp (right):

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElementTest.cpp:50: // Test passes if the restored
state is not changed.
On 2014/09/09 02:14:45, tkent (overloaded) wrote:
> This is a bad test.  The test passes even if restoreFormControlState() does
> nothing.
That is the nature of the issue that this patch tries to address. In this case
even though the selected option has index 3, index 0 would be selected after
restore without this patch. This testcase tries to check if the restore does not
alter the select state.

[spartha, 2014-09-09 08:48:53]

On 2014/09/09 02:14:46, tkent (overloaded) wrote:

>
https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
> Source/core/html/HTMLSelectElement.cpp:1042: state.append(String::number(i));
> Because you change the format of the state, you need to update
> formStateSignature() in FormController.cpp.
Do you mean the version number?

[tkent, 2014-09-09 08:56:52]

On 2014/09/09 08:48:53, spartha wrote:
> > Because you change the format of the state, you need to update
> > formStateSignature() in FormController.cpp.
> Do you mean the version number?

Right.  Also, we may replace "WebKit" with "Blink" because this is the first
update after the fork.

[spartha, 2014-09-09 11:09:57]

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1042: state.append(String::number(i));
On 2014/09/09 02:14:45, tkent (overloaded) wrote:
> Because you change the format of the state, you need to update
> formStateSignature() in FormController.cpp.

Done.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1077: // The saved state should have
atleast one value and an index.
On 2014/09/09 02:14:45, tkent (overloaded) wrote:
> atleast -> at least

Done.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1080: size_t index = state[1].toInt();
On 2014/09/09 02:14:45, tkent (overloaded) wrote:
> This code parsing a unsigned value as a signed value.

Done.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1082:
toHTMLOptionElement(items[index])->setSelectedState(true);
On 2014/09/09 02:14:45, tkent (overloaded) wrote:
> This has a bad-cast vulnerability if state[0] is an empty string and
> items[index] is not an <option>.

Done.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1092: const size_t index = state[i +
1].toInt();
On 2014/09/09 02:14:45, tkent (overloaded) wrote:
> parsing a unsigned value as a signed value.

Done.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1094:
toHTMLOptionElement(items[index])->setSelectedState(true);
On 2014/09/09 02:14:45, tkent (overloaded) wrote:
> Ditto. bad-cast.

Done.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.h (right):

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.h:195: const
WillBeHeapVector<RawPtrWillBeMember<HTMLElement> >& items = listItems();
On 2014/09/09 02:14:45, tkent (overloaded) wrote:
> Don't define function body in a header file if it contains two or more lines.

Done.

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElementTest.cpp (right):

https://codereview.chromium.org/541693003/diff/60001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElementTest.cpp:41: "<option
value='111'>111</option>"
On 2014/09/09 02:14:45, tkent (overloaded) wrote:
> Indent continuous lines.

Done.

[tkent, 2014-09-09 23:55:24]

https://codereview.chromium.org/541693003/diff/100001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/541693003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1087: if
(isHTMLOptionElement(items[index]) && valueAtIndex(index) == state[0]) {
This line has out-of-bound-access vulnerability.  Please assume |index| is a
tainted value.

https://codereview.chromium.org/541693003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1099: if
(isHTMLOptionElement(items[index]) && valueAtIndex(index) == value) {
Ditto.

https://codereview.chromium.org/541693003/diff/100001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElementTest.cpp (right):

https://codereview.chromium.org/541693003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElementTest.cpp:52: // Test passes if the restored
state is not changed.
This is not acceptable.
Please use another Document, or re-make a <select> element, then restore.

[spartha, 2014-09-10 06:33:34]

https://codereview.chromium.org/541693003/diff/100001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/541693003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1087: if
(isHTMLOptionElement(items[index]) && valueAtIndex(index) == state[0]) {
On 2014/09/09 23:55:23, tkent (overloaded) wrote:
> This line has out-of-bound-access vulnerability.  Please assume |index| is a
> tainted value.
> 
> 

Done. Sorry that I missed it.

https://codereview.chromium.org/541693003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1099: if
(isHTMLOptionElement(items[index]) && valueAtIndex(index) == value) {
On 2014/09/09 23:55:23, tkent (overloaded) wrote:
> Ditto.

Done.

https://codereview.chromium.org/541693003/diff/100001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElementTest.cpp (right):

https://codereview.chromium.org/541693003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElementTest.cpp:52: // Test passes if the restored
state is not changed.
On 2014/09/09 23:55:23, tkent (overloaded) wrote:
> This is not acceptable.
> Please use another Document, or re-make a <select> element, then restore.

Done. I now clear the existing selection and then restore.

[spartha, 2014-09-10 10:45:52]

Patchset #7 (id:120001) has been deleted

[tkent, 2014-09-10 23:35:50]

https://codereview.chromium.org/541693003/diff/140001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/541693003/diff/140001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1087: if (index < itemsSize &&
isHTMLOptionElement(items[index]) && valueAtIndex(index) == state[0]) {
|index < items.size() && isHTMLOptionElement(items[index])| in valueAtIndex() is
redundant and should be removed.  valueAtIndex() would be helpless, and we
should remove valueAtIndex() entirely.

[spartha, 2014-09-12 05:46:08]

https://codereview.chromium.org/541693003/diff/140001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/541693003/diff/140001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1087: if (index < itemsSize &&
isHTMLOptionElement(items[index]) && valueAtIndex(index) == state[0]) {
On 2014/09/10 23:35:50, tkent (overloaded) wrote:
> |index < items.size() && isHTMLOptionElement(items[index])| in valueAtIndex()
is
> redundant and should be removed.  valueAtIndex() would be helpless, and we
> should remove valueAtIndex() entirely.

Done.

[tkent, 2014-09-12 06:30:40]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-09-12 06:30:40]

lgtm

[commit-bot: I haz the power, 2014-09-12 06:30:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541693003/160001

[commit-bot: I haz the power, 2014-09-12 07:50:58]

Committed patchset #8 (id:160001) as 181883

[Michael van Ouwerkerk, 2014-09-12 15:30:36]

On 2014/09/12 07:50:58, I haz the power (commit-bot) wrote:
> Committed patchset #8 (id:160001) as 181883

Looks like this test crashes consistently on Android Tests (dbg) starting at
this build:
http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg...

[Michael van Ouwerkerk, 2014-09-12 15:31:17]

A revert of this CL (patchset #8 id:160001) has been created in
https://codereview.chromium.org/566913002/ by mvanouwerkerk@chromium.org.

The reason for reverting is: Looks like this test crashes consistently on
Android Tests (dbg) starting at this build:
http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg...
.