Closed Bug 1212688 Opened 4 years ago Closed 4 years ago

Firefox freezes if scrolling option list has specific CSS.

Categories

(Core :: Layout: Form Controls, defect)

18 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- affected
firefox42 --- affected
firefox43 --- affected
firefox44 --- unaffected
firefox-esr38 --- affected

People

(Reporter: lyngaas, Assigned: mats)

References

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

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.
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
Blocks: 806364
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: Form Controls
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Version: 41 Branch → 18 Branch
Flags: needinfo?(mats)
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
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?)
Flags: needinfo?(mats)
Attached file reduced
Attached file 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.
Assignee: nobody → mats
Attached patch fix (obsolete) — Splinter Review
We should drill through any scroll frame before we recurse.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4a124f61035
Attachment #8672392 - Flags: review?(roc)
Attached patch crashtestSplinter Review
BTW, I tested that the patch fixes the problem also without the fix in bug 1208978.
Attached patch fixSplinter Review
Right fix this time.  (shouldn't clobber |option| since that's used for the loop)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50ac9e3b2a4a
Attachment #8672392 - Attachment is obsolete: true
Attachment #8672392 - Flags: review?(roc)
Attachment #8672394 - Flags: review?(roc)
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.
Attachment #8672394 - Flags: review?(roc) → review+
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.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/faa66615c0a1
https://hg.mozilla.org/mozilla-central/rev/090f71020a97
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1228670
You need to log in before you can comment on or make changes to this bug.