112241 - Mozilla locks up [and/or crahses] when select box is used (recursive onchange)

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[Jaon]

From Bugzilla Helper:[e-mail]
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.6) Gecko/20011120
BuildID:    2001112009

When attempting to select an item in the selct box on this page, Mozilla and all
instances of Mozilla (including mail) lock up and eventually Win2K labels them
"not responding" and closes them. This was tested several times. Works fine in IE.

Reproducible: Always
Steps to Reproduce:
1.go to
http://www.zdnet.com/yil/content/college/college2000/rank_university_100.html
2. try to use select-box
3.

Actual Results:  Mozilla locks up

Expected Results:  Normal select-box functionality.

Comment 1

[Matthias Versen [:Matti]]

confirming with win2k build 20011127..

we are looping

Comment 2

[Matthias Versen [:Matti]]

Attachment: Stack trace of the hang

this is the first time i got a a "out of memory warning" since I installed
win2k with 512MB RAM while generating a stack :-(

Comment 3

[rods (gone)]

reassigning

Comment 4

[John Keiser (jkeiser)]

From the stack trace, the problem appears to be:

1. select1.onChange calls a function
2. That function changes select1.text
3. Changing select1.text calls select1.onChange (this is bad behavior).

Comment 5

[alge]

on linux/2001112712 too. 
watching the "size" grow in top was interersting, topped at about 520MB (:

Comment 6

[alge]

btw, TB38584291Z on linux.

Comment 7

[John Keiser (jkeiser)]

I investigated some, the selection firing and updating is messy as hell and
that's where the confusion is coming in.  I will clean that up with this patch.
 At the least we need to split off event firing from the ever-mysterious
UpdateSelection().  That would fix this bug.

Even after that there are other hidden sillinesses with SetText()'s call to
UpdateSelection(), centering primarily on the fact that it calls
UpdateSelection() with new index=0.  This means we will behave inconsistently
depending on whether the first item is the selected index or not (we won't
repaint or anything in that case--that may actually be the source of some other
weird option text bugs I have heard of).  I think we should remove that param
from UpdateSelection() and put it into another method that does that check.

Comment 8

[John Keiser (jkeiser)]

*** Bug 112233 has been marked as a duplicate of this bug. ***

Comment 9

[John Keiser (jkeiser)]

OK, I'm making progress.  I have figured out what's going on, and splitting the
Combobox::UpdateSelection() up a little and removing the aIndex parameter is a
good start for a fix here.  I have made a good head start, making a redisplay
function to do just that, and removed two member variables in the bargain :)

Some of these changes are definitely out of scope for the bug, but will benefit
things overall.  I am soooo tired of trying to figure out how all these damn
event firing and redisplay functions work every single time I run across a bug.
 The names are nondescript, parameters get ignored, and some functions in
interfaces aren't even used.

This bug will last through the weekend though, I'm afraid.

I'll attach my preliminary analysis of the event-firing and redisplay-triggering
functions in Combobox and List.  This patch, when done, will hopefully untangle
the mess.

Comment 10

[John Keiser (jkeiser)]

Attachment: callers/called/descriptions for the major event- and redisplay-triggering functions in Combobox and List

Comment 11

[John Keiser (jkeiser)]

Comment on attachment 59660 [details]
callers/called/descriptions for the major event- and redisplay-triggering functions in Combobox and List

Ugh, I *have* to get out of the habit of marking things patches.

Comment 12

[John Keiser (jkeiser)]

The patch works pretty well, fixes this problem and several others, and makes
the whole notification scheme somewhat readable (though I am not sure if it's as
pretty as it can be yet).  I will regression test the crap out of it, clean it
up, and attach it in the next couple of days and we'll be on the way out here.

Note to self: performance test this against setting option.text on 10000 options
sequentially.

Comment 13

[Boris Zbarsky [:bzbarsky]]

*** Bug 113689 has been marked as a duplicate of this bug. ***

Comment 14

[Boris Zbarsky [:bzbarsky]]

*** Bug 115156 has been marked as a duplicate of this bug. ***

Comment 15

[John Keiser (jkeiser)]

The patch is working and posted in bug 113866.  Haven't found any regressions so
far.  Still, too big to dump in 0.9.7 at the last minute.  0.9.8 for sure unless
something big comes and smacks the Moz codebase.  Feel free to test the patch
over there!

Comment 16

[Phil Schwartau]

Note also bug 77271:
"Need to filter recursive events to prevent crashes"

Comment 17

[Boris Zbarsky [:bzbarsky]]

*** Bug 115256 has been marked as a duplicate of this bug. ***

Comment 18

[Phil Schwartau]

*** Bug 115325 has been marked as a duplicate of this bug. ***

Comment 19

[Dimitrios]

*** Bug 115613 has been marked as a duplicate of this bug. ***

Comment 20

[Jochen Magnus]

This bug is not solved in 0.9.7 as far as I see?

Work around in programming JavaScript is to use a 
   setTimeout("stuff which changes option.text",50)

This avoids the recursion to the onChange Event.

jo

Comment 21

[John Keiser (jkeiser)]

*** Bug 118939 has been marked as a duplicate of this bug. ***

Comment 22

[John Keiser (jkeiser)]

Working on this now.  I'll make a smaller patch to fix just this for the time being.

Comment 23

[John Keiser (jkeiser)]

Attachment: Select OnChange / Reflow Patch

This fixes this bug as well as making the whole flow of select reflow and
onchange stuff a lot clearer and cleaner.

Comment 24

[John Keiser (jkeiser)]

Attachment: Description of Changes In Patch

To speed review along, this is a description of the changes I made in the
patch.

Comment 25

[John Keiser (jkeiser)]

Attachment: Select OnChange/Reflow Patch 2

Fixes minor bustage in patch: keyboard selection when dropdown is not dropped
down was not firing onChange

Comment 26

[rods (gone)]

r=rods

Comment 27

[John Keiser (jkeiser)]

Attachment: Patch 3

Pressing return when the combobox was not dropped down was firing onChange even
though the selected index did not change.  It has now been beaten on and
scrutinized against both rods's and my regression tests.

Comment 28

[rods (gone)]

r=rods

Comment 29

[kinmoz]

I can't crash reproduce the recursive onChange() problem in my 01/24/02 Win32 
debug build. Did bryner's fix for bug 120189 fix this problem already?

Comment 30

[kinmoz]

Comment on attachment 66008 [details] [diff] [review]
Patch 3

sr=[e-mail]

Lets just make sure your changes don't conflict with whatever it was that fixed
the original recursion problem.

Comment 31

[John Morrison]

*** Bug 112052 has been marked as a duplicate of this bug. ***

Comment 32

[John Morrison]

*** Bug 121023 has been marked as a duplicate of this bug. ***

Comment 33

[John Keiser (jkeiser)]

Checked in and verified.

Comment 34

[John Keiser (jkeiser)]

Resolved, even.

Comment 35

[Dimitrios]

And fixed problem in bug 115613, among others. Nice job! Verifying.

Comment 36

[Ray Aspeitia]

I'm running version 2002020405 and the bug has reappeared. Version 2002012808 
nightly build seemed to have been fixed. 

Running the Macintosh version.

Can someone verify?

Comment 37

[m_mozilla]

verified on Mac OS X with Mozilla 0.9.8 2002020411

Mozilla does not hang, but rather crashes. Can someone test on other platforms
and see whether it does just one of the two, or depends on platform (and then
update the summary acordingly)?

Changing URL to give simple test case.

Changing OS to Mac OS X. Can someone change to ALL if they can verify on other
platforms?

Reopening bug. The milestone of 0.9.9 is still set, and seems appropriate. I'll
nominate this for inclusion in bug 122050

-matt

Comment 38

[Ray Aspeitia]

Addition to my previous post, 

I'm running Mac OS 9.2.2

Ray A.

Comment 39

[m_mozilla]

I don't see an option for "Mac OS X and Mac OS 9.x" so I'm putting it to ALL

-matt

Comment 40

[Boris Zbarsky [:bzbarsky]]

Hello... This was checked in Jan 28.  0.9.8 froze on Jan 16.  The patch does not
have a=.  This means that this patch did _not_ make 0.9.8 (hence the 0.9.9
milestone).  Testing this with branch builds is completely pointless.

Ray, are you seeing this in the _trunk_ 2002020405 build?

Comment 41

[m_mozilla]

my bad.

The trunk is fine.. (and I *love* the sheets support... now if only they'd be in
the tab instead of in the window)

-matt

Comment 42

[Ray Aspeitia]

Boris,

yes you are right (humbled look on face). I didn't catch that it did not make 
0.9.8.

Thanks.

Ray A.

Comment 43

[Boris Zbarsky [:bzbarsky]]

*** Bug 123935 has been marked as a duplicate of this bug. ***

Comment 44

[Alexander Zink :alex]

*** Bug 125670 has been marked as a duplicate of this bug. ***