Bug 159058 – Regression: HTMLOptionsCollection's named properties have precedence over indexed properties

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[David Gasperoni]

When reading the options property of a <select> with 'multiple' property, the collection may have duplicate elements in it, resulting in the collection having more elements than 'length'.

The bug is present in Safari TP Release 7, in WebKit r202375, but is not present in Safari 9.1.1

[Chris Dumez]

(In reply to comment #0)
> When reading the options property of a <select> with 'multiple' property,
> the collection may have duplicate elements in it, resulting in the
> collection having more elements than 'length'.
> 
> The bug is present in Safari TP Release 7, in WebKit r202375, but is not
> present in Safari 9.1.1

Do you have a small reproduction case by any chance?

[David Gasperoni]

Created attachment 281924 [details]
Testcase with steps to reproduce

I copy-pasted directly from the page where I experienced this behaviour, which is breaking JS code a loop (the value of attribute 'length' is smaller than the amount of elements in the 'options' array).

To test, proceed as following:

1. Open file.
2. Open Web Inspector.
3. Select the <select> tag in the page.
4. Type $0.length in the Console // results 45
5. Type $0.options and expand the HTMLOptionsCollection in the Console.

You'll see a list of 92 elements. (see screenshot here: http://cl.ly/1s232B3L1s2Y)

[Radar WebKit Bug Importer]

<rdar://problem/26988542>

[Chris Dumez]

So it seems it is just the WebInpector output that is confusing:
select.options.length is 45
select.length is 45

However if you display select.options in WebInspector you see more.

I'll try and figure out why this happens. I am also CC'ing a few WebInspector people.

[Chris Dumez]

for (var a in $0.options) console.log(a);

Also only enumerate 45 items (+ the operations on HTMLOptionsCollection), as expected.

[David Gasperoni]

(In reply to comment #4)
> So it seems it is just the WebInpector output that is confusing:
> select.options.length is 45
> select.length is 45
> 
> However if you display select.options in WebInspector you see more.
> 
> I'll try and figure out why this happens. I am also CC'ing a few
> WebInspector people.


I have to say that I stumbled on this bug with a loop like this:

function add_attr_multiple()
{
	var attr = document.getElementById('attribute_group');
	var length = attr.length || 0;

	for (var i = 0; i < length; ++i)
	{
		elem = attr.options[i];
		if (elem.selected)
		{
			// Do stuff...
		}
	}
}

And if the selected elements are at the end of the list, they are never reached because 'length' is 45, but attr.options is much longer (because of duplicates) so 'i' is not high enough to access those elements. This happens with the Web Inspector closed.

[Chris Dumez]

This issue is likely related to the name of the options being numbers. The duplicates come from the fact that some options are returned by the indexed property getter and others come from the named property getter.

[Chris Dumez]

I believe I found the bug, I will work on fixing this ASAP.

[Chris Dumez]

Created attachment 282099 [details]
Patch

[Chris Dumez]

Note that the WebInspector still seems a bit confused with my fix. However, the HTMLOptionsCollection now no longer contains duplicates and indexed properties correctly take precedence over named properties.

> Selected Element
< <select multiple name="attributes[]" id="attribute_group" style="height: 500px">…</select>
> $0.options
< HTMLOptionsCollection (45) = $2
0 
<option name="86" id="attr_86" value="HiCo2750oe" title="HiCo2750oe">HiCo2750oe</option>
1 
<option name="62" id="attr_62" value="Campo Firma Singolo" title="Campo Firma Singolo">Campo Firma Singolo</option>
2 
<option name="40" id="attr_40" value="Campo Firma Multiplo" title="Campo Firma Multiplo">Campo Firma Multiplo</option>
3 
<option name="35" id="attr_35" value="Chip IC FM4442" title="Chip IC FM4442">Chip IC FM4442</option>
4 
<option name="87" id="attr_87" value="Chip IC FM4428" title="Chip IC FM4428">Chip IC FM4428</option>
5 
<option name="32" id="attr_32" value="Codice a Barre" title="Codice a Barre">Codice a Barre</option>
6 
<option name="89" id="attr_89" value="No" title="No">No</option>
7 
<option name="88" id="attr_88" value="Sì" title="Sì">Sì</option>
8 
<option name="90" id="attr_90" value="Codifica con numerazione" title="Codifica con numerazione">Codifica con numerazione</option>
9 
<option name="72" id="attr_72" value="Effetto 3D ruvido" title="Effetto 3D ruvido">Effetto 3D ruvido</option>
10 
<option name="41" id="attr_41" value="1 riga" title="1 riga">1 riga</option>
11 
<option name="70" id="attr_70" value="2 o più righe" title="2 o più righe">2 o più righe</option>
12 
<option name="45" id="attr_45" value="Etichetta removibile" title="Etichetta removibile">Etichetta removibile</option>
13 
<option name="42" id="attr_42" value="Finiture particolari" title="Finiture particolari">Finiture particolari</option>
14 
<option name="34" id="attr_34" value="Foro o asola" title="Foro o asola">Foro o asola</option>
15 
<option name="58" id="attr_58" value="Glitter" title="Glitter">Glitter</option>
16 
<option name="83" id="attr_83" value="No" title="No">No</option>
17 
<option name="84" id="attr_84" value="Sì" title="Sì">Sì</option>
18 
<option name="85" id="attr_85" value="Sì, con codifica antenna" title="Sì, con codifica antenna">Sì, con codifica antenna</option>
19 
<option name="63" id="attr_63" value="Numeri + Nomi e Cognomi" title="Numeri + Nomi e Cognomi">Numeri + Nomi e Cognomi</option>
20 
<option name="64" id="attr_64" value="Foto" title="Foto">Foto</option>
21 
<option name="76" id="attr_76" value="Standard 5gg (gratis)" title="Standard 5gg (gratis)">Standard 5gg (gratis)</option>
22 
<option name="74" id="attr_74" value="Express 24/48h (+30%)" title="Express 24/48h (+30%)">Express 24/48h (+30%)</option>
23 
<option name="73" id="attr_73" value="QR Code" title="QR Code">QR Code</option>
24 
<option name="36" id="attr_36" value="Tk4100 (125 Khz ; Read Only)" title="Tk4100 (125 Khz ; Read Only)">Tk4100 (125 Khz ; Read Only)</option>
25 
<option name="77" id="attr_77" value="T5577 (125 Khz ; Read & Write)" title="T5577 (125 Khz ; Read & Write)">T5577 (125 Khz ; Read & Write)</option>
26 
<option name="78" id="attr_78" value="F08 Compatibile 1K (ISO 14443 A)" title="F08 Compatibile 1K (ISO 14443 A)">F08 Compatibile 1K (ISO 14443 A)</option>
27 
<option name="79" id="attr_79" value="Mifare® S50 1K classic ( ISO 14443 A)" title="Mifare® S50 1K classic ( ISO 14443 A)">Mifare® S50 1K classic ( ISO 14443 A)</option>
28 
<option name="80" id="attr_80" value="Mifare® Ultralight NXP EV1 ( ISO 14443 A)" title="Mifare® Ultralight NXP EV1 ( ISO 14443 A)">Mifare® Ultralight NXP EV1 ( ISO 14443 A)</option>
29 
<option name="81" id="attr_81" value="Compatibile Ultralight ( ISO 14443 A)" title="Compatibile Ultralight ( ISO 14443 A)">Compatibile Ultralight ( ISO 14443 A)</option>
30 
<option name="82" id="attr_82" value="I-Code SLI 2K NXP (ISO 15693)" title="I-Code SLI 2K NXP (ISO 15693)">I-Code SLI 2K NXP (ISO 15693)</option>
31 
<option name="39" id="attr_39" value="Scratch off" title="Scratch off">Scratch off</option>
32 
<option name="43" id="attr_43" value="Spessore" title="Spessore">Spessore</option>
33 
<option name="65" id="attr_65" value="1 lato" title="1 lato">1 lato</option>
34 
<option name="66" id="attr_66" value="2 lati" title="2 lati">2 lati</option>
35 
<option name="52" id="attr_52" value="LoCo 300oe" title="LoCo 300oe">LoCo 300oe</option>
36 
<option name="53" id="attr_53" value="LoCo 300oe con codifica" title="LoCo 300oe con codifica">LoCo 300oe con codifica</option>
37 
<option name="54" id="attr_54" value="HiCo 2750oe" title="HiCo 2750oe">HiCo 2750oe</option>
38 
<option name="55" id="attr_55" value="HiCo 2750oe con codifica" title="HiCo 2750oe con codifica">HiCo 2750oe con codifica</option>
39 
<option name="56" id="attr_56" value="HiCo 4000oe" title="HiCo 4000oe">HiCo 4000oe</option>
40 
<option name="57" id="attr_57" value="HiCo 4000oe con codifica" title="HiCo 4000oe con codifica">HiCo 4000oe con codifica</option>
41 
<option name="91" id="attr_91" value="Trasparenza Lucida" title="Trasparenza Lucida">Trasparenza Lucida</option>
42 
<option name="92" id="attr_92" value="Trasparenza Opaca" title="Trasparenza Opaca">Trasparenza Opaca</option>
43 
<option name="46" id="attr_46" value="Tricard" title="Tricard">Tricard</option>
44 
<option name="71" id="attr_71" value="Vernice in rilievo" title="Vernice in rilievo">Vernice in rilievo</option>
45 undefined
46 undefined
52 undefined
53 undefined
54 undefined
55 undefined
56 undefined
57 undefined
58 undefined
62 undefined
63 undefined
64 undefined
65 undefined
66 undefined
70 undefined
71 undefined
72 undefined
73 undefined
74 undefined
76 undefined
77 undefined
78 undefined
79 undefined
80 undefined
81 undefined
82 undefined
83 undefined
84 undefined
85 undefined
86 undefined
87 undefined
88 undefined
89 undefined
90 undefined
91 undefined
92 undefined

HTMLOptionsCollection Prototype

[Chris Dumez]

Comment on attachment 282099 [details]
Patch

Clearing flags on attachment: 282099

Committed r202478: <http://trac.webkit.org/changeset/202478>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

@David Gasperoni: You should be able to confirm the fix in a day or two using a WebKit nightly build (https://webkit.org/nightly/) once they move past revision 202478.

[David Gasperoni]

(In reply to comment #13)
> @David Gasperoni: You should be able to confirm the fix in a day or two
> using a WebKit nightly build (https://webkit.org/nightly/) once they move
> past revision 202478.

That's awesome! I'll keep an eye out. ;)

[David Gasperoni]

I tried with r202481, and it works correctly as expected. The only oddity is that listing the contents of .options also reports many undefined elements (just like you reported in #10 ) even though .options.length is the correct amount.

[Chris Dumez]

(In reply to comment #15)
> I tried with r202481, and it works correctly as expected. The only oddity is
> that listing the contents of .options also reports many undefined elements
> (just like you reported in #10 ) even though .options.length is the correct
> amount.

Yes, we should file a separate bug about this and set WebInspector as component.

[Joseph Pecoraro]

(In reply to comment #16)
> (In reply to comment #15)
> > I tried with r202481, and it works correctly as expected. The only oddity is
> > that listing the contents of .options also reports many undefined elements
> > (just like you reported in #10 ) even though .options.length is the correct
> > amount.
> 
> Yes, we should file a separate bug about this and set WebInspector as
> component.

The inspector issue is because we use Object.getOwnPropertyNames on an array-like object with length less then 100. That said, I think we can remove that constraint, I think it was from an earlier time.

[Joseph Pecoraro]

See bug 159192 for handling this edge case better in Web Inspector.