1212688 - Firefox freezes if scrolling option list has specific CSS.

Status: RESOLVED FIXED

Description

[James]

Created attachment 8671122 [details]
firefoxoptionlistcssbug.htm

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150929144111

Steps to reproduce:

This CSS: * {overflow-x: hidden;} causes firefox to freeze if you make a <select> list with 17 or more <option>'s


Actual results:

Putting 17 items in a select list display fine, but when you put 18 options in and click the select menu it freezes firefox and slows it to a crawl (instead of triggering a vertical scrollbar).


Expected results:

The overflow-x CSS should have just prevented horizontal scrollbars on everything, and not frozen the page.

Comment 1

[Alice0775 White]

I can reproduce 100% cpu usage and significant response performance degradation.

Regressed since Firefox18.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=533faa3c50ed&tochange=b963ae0f9b60

Regressed by: Bug 806364

Comment 2

[James]

NOTE from further observation:  The bug triggers at 20 items in a select list, not 18 or 17.  I have an extra option above my option group I didn't notice when I submitted, and the option group itself should count as one item since it takes up one line.  

Still it freezes when it hits the scrolling point for a <select> list.  Thanks

Comment 3

[Mats Palmgren (:mats)]

A minimal testcase would be nice.  In particular, which element needs to have
"overflow-x: hidden" for it to occur?

FWIW, I can reproduce it in Aurora(v43), but not in Nightly(v44), both on Linux64.
(Maybe we optimize away reflows in Nightly in some situations that masks it?)

Comment 4

[Alice0775 White]

Created attachment 8672344 [details]
reduced

Comment 5

[Alice0775 White]

Fixed range without e10s:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=36cd98d7f222d2e4177dc43eb55e0b1f655b55dd&tochange=67f016207dde

Comment 6

[Mats Palmgren (:mats)]

Created attachment 8672391 [details]
frame tree

The problem is the <optgroup> scroll frame.  When GetMaxOptionBSize
finds a frame with <optgroup> content it recurses into its primary frame:
http://hg.mozilla.org/mozilla-central/annotate/b68eab795f9d/layout/forms/nsListControlFrame.cpp#l265
which is the scroll frame in this case, and the first child of that
is the scrollbar frame which has the block-size large enough to fit
all its <option> children.  So we overestimate the row size by a huge
amount.  Apparently, that sends us into a reflow loop.

Bug 1208978 wallpapers that by limiting the size, breaking the loop.
But that seems mostly like a coincidence.

Comment 7

[Mats Palmgren (:mats)]

Created attachment 8672392 [details] [diff] [review]
fix

We should drill through any scroll frame before we recurse.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4a124f61035

Comment 8

[Mats Palmgren (:mats)]

Created attachment 8672393 [details] [diff] [review]
crashtest

Comment 9

[Mats Palmgren (:mats)]

BTW, I tested that the patch fixes the problem also without the fix in bug 1208978.

Comment 10

[Mats Palmgren (:mats)]

Created attachment 8672394 [details] [diff] [review]
fix

Right fix this time.  (shouldn't clobber |option| since that's used for the loop)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50ac9e3b2a4a

Comment 11

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

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

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

Looks good. However, shouldn't we also prevent scrollframe construction on descendants of a <select>? That doesn't make sense to me and could break other things. I guess that would require a change to nsCSSFrameConstructor?

But we should take this patch anyway since it's simple and correct in itself.

Comment 12

[Mats Palmgren (:mats)]

Yeah, we should probably just ignore a bunch of properties on <option>/<optgroup>.
A quick test in Chrome shows that it ignores 'overflow' and 'height' for example.
Filed bug 1213819.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/faa66615c0a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/090f71020a97

Comment 14

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/faa66615c0a1
https://hg.mozilla.org/mozilla-central/rev/090f71020a97