Mozilla crashes on switching themes. Steps to reproduce: 1. Start Mozilla 2. On the preferences screen, select another theme 3. Click "Apply ____" 4. Press OK Actual Result: Crash Expected Result: Switch themes normally as it has always been This occuring on 082508 Win98 as well as NT4. I assume it occurs on other platforms as well. Asa has some talkback reports on this.

Adding crash keyword and nominating for beta3.

talkback stack trace: nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp,line356] nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp,line346] nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp,line346] nsViewManager2::DispatchEvent [d:\builds\seamonkey\mozilla\view\src\nsViewManager2.cpp,line1429] HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp,line68] nsWindow::DispatchEvent [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp,line618] nsWindow::DispatchWindowEvent [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp,line635] nsWindow::DispatchMouseEvent [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp,line3812] ChildWindow::DispatchMouseEvent [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp,line4020] nsWindow::ProcessMessage [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp,line2909] nsWindow::WindowProc [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp,line884] USER32.DLL+0x3eb0(0x77e13eb0) USER32.DLL+0x401a(0x77e1401a) USER32.DLL+0x92da(0x77e192da) nsAppShellService::Run [d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp,line379] main1 [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp,line965] main [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp,line1142] WinMain [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp,line1160] WinMainCRTStartup() KERNEL32.DLL+0x7903(0x77e87903)

Crash actually happens when I'm hittink OK to exit the prefs window after a theme switch. talkback reports located at http://climate/reports/incidenttemplate.cfm?bbid=16299079 and http://climate/reports/incidenttemplate.cfm?bbid=16298803

Adding the keywords again. Yes, the crash doesn't occur during the switch, but after OK is pressed to close the preferences screen.

theme switching -> skinability. i've been getting crashes, too --but am unsure if it's the same as this bug, 'coz i'm not even getting talkback (it's not 4pm yet :). 1. open prefs, go to themes panel 2. select a theme, click apply 3. click OK to dismiss prefs 4. click on Go menu result: instant crash. no talkback, no console info. occurred on 2000.08.25.04 Mac commercial and 2000.08.25.08 linux commercial. different or same bug?

It works fine for Linux and Mac (Build: 2000082508M18). However, it crashes on Windows. I'm not sure exactly whether there's a problem with my windows because my windows is easily crashed.

se: that's bug 47490. hyatt, any ideas? might be related to hidetoshi.tajima@eng.sun.com's nsWindow.cpp checkin (which also causes the 'no window titles with certain unix window managers' problem), or adam lock's recent landing

tpringle and I (skins PMs) recommend nsbeta3+

Using build 2000082608 on Win98: bug remains. I've also tried to do other stuff after switching themes, like changing "Appearance|Fonts" and dismiss the screen with by clicking either OK or Cancel, but the app would crash _always_. Of course, after launching Mozilla again, the _new_ theme is applied.

Adding topcrash keyword. This is showing up prominently in talkback data after only one day.

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

Seems to only be occuring on Win32. Adding pp keyword. Remove if this is reproduced on Mac or Linux.

Recent checkin to nsView.cpp seems the likely culprit, CC'ing checkin author. Checkin history: http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/view/src/nsView.cpp

The bug is in my checkin, although I'm not actually the author. The view that handles the event is being destroyed in the event handler so we crash trying to get its next sibling. I have a fix. If the fix gets review and approval, I can take the bug and check in the fix.

adding review and approval keywords

I have a concern about the short-circuiting of the event propagation when the event has been handled: if (aHandled) { break; } I'm not sure, but we many need to pass the event to sibling views even if it is a handled. CC'ing joki. Also: Aren't we going back to an O^n algorithm for event propagation? nsIView* curKid; GetChild(childIndex, curKid); Is executed for each loop and the GetChild is a linear lookup through the set of child views. If so, maybe we should go back to the previous O^n implementation.

The short circuiting is exactly what we were doing before I checked in Alex's patch. I believe that its removal was simply accidental. In my patch, I only call GetChild(childIndex) for views that contain the event's point. (The diff doesn't have enough context.) So, if the child views don't overlap, the code is still O(number_of_children) because only one of them can contain the event's point. It could be O(n^2) in the worst case, but that's no worse than the old behavior and there's no evidence that that case ever occurs in reality. There doesn't seem to be an easy safe way to do this that's always O(n). We can't just grab the next sibling before we call the event handler because the next sibling might disappear too. The fastest safe way to do this that I can think of would be to have every view, on destruction, set a flag on its parent to indicate that the child list changed. The parent view can clear this flag before enumerating the children in the event handler, and check the flag after calling each child's event handler, and only call GetChild(childIndex) if the flag is set. If you think it's worth doing this optimization, I can roll a patch. Anyway, I don't think we should really be tearing down views while we're in their event handlers. There could be other nasty consequences. However, we should fix this crasher now.

Ok, the patch looks acceptable. I think the optimization would have to set a flag on the ViewManager and not the parent view. The parent view may be destroyed by handling the event. I don't think it necessary now. Most pages don't have that many views. I don't think we will get away from destroying views from within the event handlers because that is where all events are handled which are the result of user-interaction. (i.e As the result of executing the handler some javscript may fire which changes style rules - which causes views to be destroyed or added) We need to make it 100% safe to destroy views during iteration of the view hierachy or we will have to buffer all of the view hierachy changes until after the event is handled.

I think that normally the view hierarchy is modified during reflow processing, which is not usually done inside a view event handler. The situation here is slightly unusual because we're reflowing a window from inside a modal dialog box which it spawned. Now that you mention it, if the parent view is destroyed, then this code will still crash (when we call GetChild(childIndex)). This patch is not good enough. How about this: we add a flag to nsView that's set for the duration of nsView::HandleEvent processing for that view. If a view has that flag set when it's destroyed, we set a flag in the view manager to tell us to bail out of HandleEvent immediately. So if we destroy a view while handling an event on the view, no further event processing will happen (even if the event handler sets aHandled to PR_FALSE and a sibling view, or the view observer, would normally get a chance to handle the event). Sounds like a reasonable workaround to me, since event handlers that bring up modal dialog boxes should not be pretending they didn't handle the event :-). I think that the real way to fix this would be to lift the HandleEvent logic up to the view manager, in the same way that the paint logic was moved up into the display list. The view manager would collect the set of views that can handle the event and traverse the list itself. The view manager would be notified of changes to the view hierarchy and fix up its view list if necessary. This would also fix an existing problem where, in my view manager, complicated z-index settings are rendered correctly but the view event handlers aren't called in the correct order.

The "I'm destroyed" flag approach sounds promising. I think we should give it a try, but we should assert if the aHandled is PR_FALSE and the "destroyed flag" is set. I totally agree that the event handling logic should be moved out of the view and into the view manager. The events need to be handled in a way which corresponds to how the views are rendered. (i.e they need to processed in display list order.)

The communication between nsView and nsViewManager2 required to fix this using destruction flags is going to pollute the nsIView/nsIViewManager interfaces. Instead, I think we should just hoist the existing event handling logic into nsViewManager2 now. It's not much code. BTW, nsViewManager2 calls nsIView::HandleEvent with the flags NS_VIEW_FLAG_CHECK_CHILDREN | NS_VIEW_FLAG_CHECK_PARENT | NS_VIEW_FLAG_CHECK_SIBLINGS, but it seems to me that those flags are completely ignored and in fact the view never checks its parent or siblings. Should it?

It turned out that keeping the "I am handling an event" flag in the view is impossible, because if you're not sure whether or not the view has gone away, there's no way to reliably clear the flag. So I keep the data out of line instead. I also keep track of exactly which views have been deleted. I also did some work to make sure that things work right in weird cases, like if we re-enter event handling for the same view (e.g. could happen if you mouse over a button which just popped up a modal dialog, I think). I even handle really weird stuff like a view being deleted and then another view being allocated at the same address, while we're in the event handler. There were some issues with just panicing and aborting all event processing for the view manager, like what happens if you re-enter the view manager's event handler a couple of times via nested modal dialogs and then start aborting? So we just back out until we find live views again and then keep going.

Please check this in ASAP, I am crashing all over the place in nsView with a build just pulled.

What are you doing to cause the crash? I should test with my new code.

Nav triage team: nsbeta3+

Nav triage team: changing to P1

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

Is this bug fixed by simply by going back to old O^n event processing for views?. Is so, I would vote for going back to the old code and applying the patch to the new ViewManager code only. This would give us chance to test it out, to make sure it doesn't cause any regressions. What do think Robert?

Reverting to the old traversal behavior will fix the bug everyone is seeing. I'm pretty sure we could come up with another test case where the parent view is destroyed so we'd crash on the call to GetChildAt(). However, I don't know if anyone will actually see that in real life.

Ben really isn't the right owner for this bug. Changing to Views.

Lets revert to the old code for use with nsViewManager2.cpp. We should add #ifdefs's to nsView.cpp to support the new logic for nsViewManager.cpp however.

OK, sure. Don't bother with #ifdefs for me. Just back out the change (but put the aHandled short-circuit back in!). When I get around to really fixing event handling in nsViewManager, I will also produce a separate patch to nsView that uses the flags parameter to nsIView::HandleEvent to get the behavior I need. This will allow nsViewManager and nsViewManager2 to continue to coexist.

With build 083008 for Linux I am no longer crashing when switching themes from the preferences menu, however, when I select to apply a theme from the view menu, apply themes and select one, the program will crash and have to relaunch.

PDT agrees P1

Kevin, you're taking care of this, right?

Yes, I'm ready to check in, just waiting for some green.

Reverted back to old loop construct for dispatching events which is safer when the event destroys the current view. Fixed in 8/30/2000 7:38PM build.

Still crashing when swtiching themes from view dropdown menu. No error message, just quits. Build 083021. Linux i686.

It is crashing in the menu popup code. The nsMenuPopupFrame has a null vptr when GetParent is called. nsMenuPopupFrame::DismissChain() { // Stop capturing rollups if (nsMenuFrame::mDismissalListener) nsMenuFrame::mDismissalListener->Unregister(); // Get our menu parent. nsIFrame* frame; ==> GetParent(&frame); Full stack trace on WIN32: nsMenuPopupFrame::DismissChain(nsMenuPopupFrame * const 0x02d5cb30) line 1150 + 17 bytes nsMenuFrame::Execute() line 1505 nsMenuFrame::HandleEvent(nsMenuFrame * const 0x03965a80, nsIPresContext * 0x02a18c70, nsGUIEvent * 0x0012f894, nsEventStatus * 0x0012f784) line 378 PresShell::HandleEventInternal(nsEvent * 0x0012f894, nsIView * 0x03f3a2f0, nsEventStatus * 0x0012f784) line 4055 + 38 bytes PresShell::HandleEvent(PresShell * const 0x02a572e4, nsIView * 0x03f3a2f0, nsGUIEvent * 0x0012f894, nsEventStatus * 0x0012f784, int 0, int & 1) line 3975 + 23 bytes nsView::HandleEvent(nsView * const 0x03f3a2f0, nsGUIEvent * 0x0012f894, unsigned int 8, nsEventStatus * 0x0012f784, int 0, int & 1) line 379 nsView::HandleEvent(nsView * const 0x03f3a4d0, nsGUIEvent * 0x0012f894, unsigned int 8, nsEventStatus * 0x0012f784, int 0, int & 1) line 352 nsView::HandleEvent(nsView * const 0x03f1e300, nsGUIEvent * 0x0012f894, unsigned int 8, nsEventStatus * 0x0012f784, int 0, int & 1) line 352 nsView::HandleEvent(nsView * const 0x02a18780, nsGUIEvent * 0x0012f894, unsigned int 28, nsEventStatus * 0x0012f784, int 1, int & 1) line 352 nsViewManager2::DispatchEvent(nsViewManager2 * const 0x02a18960, nsGUIEvent * 0x0012f894, nsEventStatus * 0x0012f784) line 1429 HandleEvent(nsGUIEvent * 0x0012f894) line 68 nsWindow::DispatchEvent(nsWindow * const 0x03f3a394, nsGUIEvent * 0x0012f894, nsEventStatus & nsEventStatus_eIgnore) line 614 + 10 bytes nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f894) line 635 nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 3811 + 21 bytes ChildWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4021 nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 4194356, long * 0x0012fc10) line 2889 + 24 bytes nsWindow::WindowProc(HWND__ * 0x000102fc, unsigned int 514, unsigned int 0, long 4194356) line 883 + 27 bytes USER32! 77e71820() 00400034() Re-opening bug.

Changing summary from: Crash on switching themes [@ nsView::HandleEvent] to Crash on switching themes using menu item Reassigning to hyatt.

Leaving this plus under protest. Adding features like this, especially menu access to such unstable features as skin switching, this late in the project, is INSANE. We should instead remove the menu item. M18 because this also fixes Alt-F-X.

accepting for hyatt

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

I will take a look at this when I return on Tuesday if hyatt doesn't get to it first, and will remove the menu if need be. I think I may have an idea where the problem lies.

Reporting comments from bug 51012 that has been marked as a duplicate of this bug: Reporter:ltskinol@edgebbs.com I didn't see this one listed in the last few days' bugs. I think it's a regression. 1. Start Mozilla. 2. View->Apply Theme->Blue 3. Browser crashes. Build: 2000083111, Linux/x86. I started out with a clean package and .mozilla directory. Seems to be always reproducable.

The problem is obvious, and it's similar to before: after you call a HandleEvent which could invoke Javascript, you must assume that your frame and view could have been destroyed. The menu code is touching "this" after the HandleEvent call and therefore crashes and burns. I agree with trudelle that dynamic theme switching is a kind of crazy thing to try to pull off this late in the game.

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

This also crashes on Linux 2000090121 in Edit->Preferences->Themes->Apply <theme>.

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

I've got the same behavior: 1. Start Mozilla. 2. View->Apply Theme->Classic 3. Browser crashes. 4. When I start it up again, I can see that the theme change has taken place, however. I've got this happening on 2000083111 Windows and 2000090108 Linux. On Windows I get "illegal operation->close" and then a talkback, but on Linux I don't even get any error messages from the console, it just exists without a talkback screen popping up.

I get this same thing happening under Purify on Solaris doing a theme switch through the view menu.

It's crashing on the Preferences screen again, using 090208 Win98. Back over to Skinibility.

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

Fixed.

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

Verified fixed 090308. Thanks Hyatt.

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

No, this needs to be verified fixed on all platforms. Reopening and QA assigning to me (sorry for spam)

re-resolving as FIXED so I can verify upon my return...

vrfy fixed - mac, win32, linux (today's builds), via both the menu and the prefs