Closed Bug 50335 Opened 24 years ago Closed 24 years ago

Crash on switching themes

Categories

(Core Graveyard :: Skinability, defect, P1)

Product:
Core Graveyard
Component:
Skinability
Platform:
x86
All
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: sk3iii, Assigned: hyatt)

References

Details

(4 keywords, Whiteboard: [nsbeta3+])

Attachments

(2 files)

Patch to provide robust child traversal in nsView::HandleEvent
1.84 KB, patch
Details | Diff | Splinter Review
Patch moving logic to nsViewManager2; should fix all view destruction issues during event handling
12.22 KB, patch
Details | Diff | Splinter Review 
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.
Keywords: crash, nsbeta3
Keywords: crash, nsbeta3
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.
Keywords: crash, nsbeta3
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?
Assignee: hangas → ben
Component: Themes → Skinability
QA Contact: pmac → BlakeR1234
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
Keywords: regression
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.
Keywords: topcrash
Summary: Crash on switching themes → Crash on switching themes [@ nsView::HandleEvent]
*** 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.
Keywords: pp
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
Keywords: approval, review

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+
Whiteboard: [nsbeta3+]
Nav triage team: changing to P1
Priority: P3 → 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.
Assignee: ben → kmcclusk
Component: Skinability → Views
QA Contact: BlakeR1234 → petersen
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.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Changing summary from:

Crash on switching themes [@ nsView::HandleEvent]

to 

Crash on switching themes using menu item

Reassigning to hyatt.
Assignee: kmcclusk → hyatt
Status: REOPENED → NEW
Summary: Crash on switching themes [@ nsView::HandleEvent] → Crash on switching themes using menu item
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.
Target Milestone: --- → M18
accepting for hyatt
Status: NEW → ASSIGNED
*** 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.
Component: Views → Skinability
Summary: Crash on switching themes using menu item → Crash on switching themes
*** Bug 51199 has been marked as a duplicate of this bug. ***
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
*** Bug 51214 has been marked as a duplicate of this bug. ***
Verified fixed 090308. Thanks Hyatt.
Status: RESOLVED → VERIFIED
*** 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)
Status: VERIFIED → REOPENED
QA Contact: petersen → BlakeR1234
Resolution: FIXED → ---
re-resolving as FIXED so I can verify upon my return...
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
vrfy fixed - mac, win32, linux (today's builds), via both the menu and the prefs
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: