User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113 I've a select element filled in with some options. I want to make it disappear when the selected value has changed. It works fine when I do it with mouse, but crash if I do it with arrow keys. 3 conditions : - must have 'onchange' (onblur works) - must do style.display = 'none' (style.visibility = 'hidden' works) - value must be changed with the keyboard (with the mouse, it... works) The Bug is also reproductible on MozillaFirebird-0.7/Win32 Reproducible: Always Steps to Reproduce: 1. select the select element (default value : 'A') with mouse or keyboard 2. press the down arrow key to select the value 'B" 3. press <TAB> Actual Results: Mozilla crashes Expected Results: select element disappear (display = 'none')

WFM Firebird 0.8 branch builds 20040120.

I tried Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040120 Firebird/0.8.0+ it, crashes too. If after selecting 'B', it works if you press enter, but crash if you lose focus with TAB

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040122 Confirming crash using Mozilla Nightly 2004012209 on Windows 2000. Requesting Status: NEW, Keyword: Crash

confirmed with linux trunk 2004012109 ==> layout

reassign

Comment on attachment 139715 [details] stacktrace The nsComboboxControlFrame in frame #8 of this stack is clearly deleted.

bug #230842 seems to be the same

The question is why GetPrimaryFrameFor is returning a deleted frame...

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

The Mac-OS-X version is also reproduced. Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040404 Firefox/0.8.0+

mats, haven't you looked into similar things before?

Talkback-ID: http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=564464

OK, the problem here is that the onchange event is fired synchronously from frame code (in nsComboboxControlFrame::SetFocus, when it calls FireOnChange() on its mListControlFrame). When this FireOnChange() call returns, the list combobox frame object is dead. Then it tries to access its members (and hence crashes). I looked in nsListControlFrame, and there are other codepaths that call FireOnChange() and then access members (eg nsListControlFrame::KeyPress will call UpdateSelection(), which calls FireOnChange(), and then do stuff with mStartSelectionIndex and such). Can we just not fire events from frames, somehow?

Maybe we could have a way to post a DOM event with a callback object that will be invoked when the event has been processed. This callback would hold references to content, not frames, and reacquire the frame before doing the rest of the processing.

The problem in this case, I think, is that we need to somehow "synchronously" dispatch the onchange (that is, we need to preserve existing ordering of focus/blur/change events, whatever it is). The more I think about it, the more I think what we need is a system for firing things (DOM events, XBL constructors, HTML scripts) "when it's safe"....

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

(In reply to comment #15) Yep, that is indeed the problem. I changed it to post an event instead (in the same way we did for RedisplayText) and it does not crash anymore. The problem is that this reorders the sequence handlers are called, from change-blur to blur-change which will probably break some sites. I will have a look what IE6 does and then see if I can do the event posting earlier, maybe directly on select?

I am experiencing this crash in my code, which sets style.display to "none" on a table row which contains a select which triggers the code. I have devised a workaround for this crash. The function that is called by the onchange handler simply creates a timeout on a global variable. This seems to avoid the browser crash completely. Example: ... <select name="foo" onchange="changeMe(event);"> ... <script type="JavaScript"> var theTO; var theEvt; function changeMe(evt) { theEvt = evt; theTO = window.setTimeout("realChangeMe(theEvt);",500); } function realChangeMe(evt) { ... Your actual event handler here ... ) I have not done extensive testing, but this seems to work. Please keep me updated on your progress in fixing this bug, as I use style.display="none" quite often for dynamic web sites.

adding myself to the CC list

We should really fix this -- accessing deleted frames is bad juju. Mats, please let me know if you won't be able to work on this in the near future; in that case I'll try to take a look...

TB17145720K, TB17145584K

TB17145760Z, TB17145107K Seamonkey Trunk

Taking bz up on his comment 22 offer. Given the timeframe a 1.8.0.4 fix is unrealistic, moving nomination to a later release.

OK, I tried poking at this for the last few days, and I've totally failed to do anything about it without regressing firing of onchange events, generally speaking. I can probably fix the testcase in this bug, but not the various of UpdateSelection() callers... roc, this seems like a place where your safe script thing would be really nice. :( I guess I'll try again next time I have a chance, but that's looking like it'll be sometime after May 29 at this point.

Back to Mats, since apparently he'll be around again before the next chance I'll have to look at this.

This is a situation where delaying event firing to a safe time will likely break things, even if we do it "automatically". Changes to the timing of onchange firing have traditionally been very fragile with respect to breaking Web content. This is why I'm not satisfied that my run-at-same-times patch is a good solution to these problems. The solution here could be to move a lot of this stuff to content. Not very branch-friendly but we could do it on trunk and see how invasive it is.

Doesn't look like it's being worked on (no comment in 6 weeks), can't see how it can make 1.8.0.5

Mats - will you have a chance to take a look?

(In reply to comment #30) > Mats - will you have a chance to take a look? It seems the order of events in a current trunk build is blur-change, so the problem I mentioned in comment 19 is not an issue anymore. (See Testcase #2 - it does not crash, just logs the order of events) I have a patch that fixes the crash coming up... I think the way we fire DOM events from comboboxes in general is a bit strange though. All the other browsers I tested fire Change then Blur for Testcase #2, in fact they fire Change immediately (and each time) a new value is selected using the arrow keys, which makes sense to me and I think we should do it like that too eventually (currently we fire Change on blur, from nsComboboxControlFrame::SetFocus()). But I'll handle that in a different bug, let's fix the crash first.

> It seems the order of events in a current trunk build is blur-change, Er... so it changed and is not compatible with other browsers? When did that happen? That sounds bad. > in fact they fire Change immediately (and each time) We don't do that on purpose, for accessibility reasons; see bugs on it... Doing that makes it pretty painful to use the thing with a keyboard.

(In reply to comment #33) > > in fact they fire Change immediately (and each time) > > We don't do that on purpose, for accessibility reasons; see bugs on it... One being bug 126379? My thoughts on that: bug 126379 comment 83 Are there other bugs on it?

This fixes the crash and bug 126379.

Comment on attachment 227736 [details] [diff] [review] Patch rev. 2 > One being bug 126379? That's the one. Let's put discussion on that issue in that bug, where it belongs. I'm really not happy making that change in the context of this bug. Is there really absolutely no other way to fix this?

This patch fixes the crash only, but the order of events is still wrong...

Filed bug 343303 on the event order regression.

Did you want to request review for that third patch?

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

Bit ugly, but doesn't change event ordering. The fix for this bug is in nsComboboxControlFrame.cpp, but changed also nsListControlFrame to avoid other similar crashes.

Comment on attachment 232102 [details] [diff] [review] making sure that deleted objects aren't used I know this is not perfect, especially the changes to nsListControlFrame.cpp, but those crashes must be prevented and changing the order of the events is not an option, IMO.

(In reply to comment #41) > Did you want to request review for that third patch? I was waiting for Boris reply to bug 126379 comment 92 before I did anything further with this bug. I still think "Patch rev. 2" is the way to go.

> I was waiting for Boris reply to bug 126379 comment 92 Er... I'm not cced on that bug, apparently, so that would have been a really long wait, esp. since you didn't tell me you were waiting. ;) I've commented there.

Comment on attachment 232102 [details] [diff] [review] making sure that deleted objects aren't used This generally works, yes. You could also put an nsWeakFrame thisPtr(this); on the stack and null-check that, right? That would handle the case of a new frame object being allocated in the memory slot freed by |this| when it's deallocated...

Comment on attachment 232889 [details] [diff] [review] using weakframe Yeah, I like this.

I'll make patches for branches asap.

I would think that we want this fixed in 1.8.1, renominating.

Comment on attachment 232889 [details] [diff] [review] using weakframe approved for 1.8.0 branch, a=dveditz for drivers

Comment on attachment 232889 [details] [diff] [review] using weakframe a=drivers, please land on MOZILLA_1_8_BRANCH

Verified FIXED using latest 1.8 and 1.8.0 on Linux (X11; U; Linux i686; en-US;). I no longer crash with the testcase in attachment 227652 [details].

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

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