806364 - Setting SELECT's innerHTML with empty OPTGROUPs with @label will cause Firefox hangs

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Gleb M. Borisov]

Attachment: Test case

someSelect.innerHTML = '<select><optgroup label="124"></optgroup><optgroup label="123"></optgroup></select>';

will cause firefox hang. Tested on Mac OS X 10.8 with Firefox 16.0.1. Activity Monitor shows 100% cpu load.

If we remove label attribute from optgroup all seems good.

I've attached minimal test case.

Comment 1

[Ioana (away)]

Does this issue reproduce with a clean profile or in safe mode?
http://support.mozilla.com/kb/Basic+Troubleshooting#w_8-make-a-new-profile
http://support.mozilla.com/kb/Safe+Mode

Comment 2

[Alice0775 White]

I can reproduce in windows7.

God build: Optgrpup labels are drawn in pull down list.
Bad build: 100% CPU core usage. And no upll down list pops up

Regresseion window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/bf37951c1104
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120623031143
Bad:
http://hg.mozilla.org/mozilla-central/rev/e21173ed2c38
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120623053647
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bf37951c1104&tochange=e21173ed2c38

Regresseion window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/80b8680bda1c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120622175046
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c35d2d3071ac
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120622182843
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=80b8680bda1c&tochange=c35d2d3071ac


In local build
Last Good: 23f5c88adb8f
First Bad: 612ee5c6300d
Triggered by 
612ee5c6300d	Mats Palmgren — Bug 575294. part=3/5 r=roc

Comment 3

[Gleb M. Borisov]

This issue reproduces in clean profile and in safe mode too.
Do you need more technical information or bug is confirmed?

(In reply to Ioana Budnar [QA] from comment #1)
> Does this issue reproduce with a clean profile or in safe mode?
> http://support.mozilla.com/kb/Basic+Troubleshooting#w_8-make-a-new-profile
> http://support.mozilla.com/kb/Safe+Mode

Comment 4

[Mats Palmgren (inactive)]

Oops, sorry about that.  I guess you can work around the problem by adding
a <option style='display:none'> to innerHTML.

Comment 5

[Gleb M. Borisov]

Thanks for prompt investigation!

Comment 6

[Mats Palmgren (inactive)]

Attachment: Calculate dropdown height based on number of principal child frames instead of GetNumberOfOptions() which only included <option> nodes, not <optgroup>s.

https://tbpl.mozilla.org/?tree=Try&rev=0356c2962c9f

Comment 7

[Alex Keybl [:akeybl]]

We'll try to get this fixed for FF17, but if it carries any risk, it'd likely need to land tomorrow on mozilla-beta to skip up the trains (we'd uplift to FF18 instead).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Doesn't PrincipalChildList potentially include text nodes between <option>s? We don't want to count those, do we?

Comment 9

[Mats Palmgren (inactive)]

We only create frames for <option> and <optgroup> in <select>:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#5209

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 676271 [details] [diff] [review]
Calculate dropdown height based on number of principal child frames instead of GetNumberOfOptions() which only included <option> nodes, not <optgroup>s.

Review of attachment 676271 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not totally comfortable with hard-coding the requirement that there's exactly one frame per row. How about creating a helper method GetNumberOfRows(), which calls PrincipalChildList().Length(), and calling that, so that requirement is at least confined to one place. r+ with that.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 676271 [details] [diff] [review]
Calculate dropdown height based on number of principal child frames instead of GetNumberOfOptions() which only included <option> nodes, not <optgroup>s.

Yeah, this is pretty wrong actually, since we should use the
GetOptionsContainer() frame.  I'll write up something better...

Comment 12

[Mats Palmgren (inactive)]

Attachment: fix

https://tbpl.mozilla.org/?tree=Try&rev=5cd058b9bc57

Comment 13

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1b11497135e

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e1b11497135e

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 677015 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined: hang with 100% CPU load when clicking on a combobox with no <option> but at least one <optgroup> in it
Testing completed (on m-c, etc.): on m-c since 2012-11-01
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 16

[Alex Keybl [:akeybl]]

Although the fix is low risk, the user scenario described in comment 15 is so uncommon that we'll get this first fixed in FF18.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a5944fe8a0e

Should this have a test?

Comment 19

[Debeet]

(In reply to Mats Palmgren [:mats] from comment #15)
> Comment on attachment 677015 [details] [diff] [review]
> User impact if declined: hang with 100% CPU load when clicking on a combobox
> with no <option> but at least one <optgroup> in it

This is actually wrong. It hangs as soon as the page loads. User doesn't need to click on the combobox and it doesn't happen when there is only one <optgroup>. This is a serious problem and I'm really surprised this wasn't fixed ASAP in Fx 16 and even 17.

Comment 20

[Ioana (away)]

Verified as fixed on Firefox 18 beta 4:
Mozilla/5.0 (Windows NT 6.1; rv:18.0) Gecko/18.0 Firefox/18.0
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18.0 Firefox/18.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:18.0) Gecko/20100101 Firefox/18.0

Comment 21

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old