113121 - Crash on closing a tab when lots of tabs open - Trunk [@ nsView::GetChild]

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[David Gerard]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.6+) Gecko/20011130
BuildID:    2001113003

Occasionally, when I have a lot of windows open, this build will crash
on closing a tab.

Sorry that this report is a bit vague. But I've got three Talkbacks,
so considered it was worth a bug report.

Reproducible: Sometimes
Steps to Reproduce:
1. Have a lot of tabs open.
2. Close one.

Actual Results:  Dr Watson, and Talkback kicks in.

Expected Results:  Close the tab and continue  browsing :-)

Talkback IDs:
TB38741942X
TB38742018Q
TB38776188Q

The commment on the first one (TB38741942X) talks about a crash on
loading some Flash. But it was a lots-of-tabs window like the others.

Comment 1

[Itamar]

is 30 a lot of tabs.
WFM with 30 tabs
I have
Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.6+) Gecko/20011201

Comment 2

[timeless]

.

Comment 3

[John Morrison]

stack trace is 

nsView::GetChild [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 851] 
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 361] 
nsViewManager::DispatchEvent 
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1887] 
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 84] 
nsWindow::DispatchEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 849] 
nsWindow::DispatchWindowEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 866] 
nsWindow::DispatchKeyEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 2531] 
nsWindow::OnChar [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, 
line 2663] 
nsWindow::ProcessMessage 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3240] 
nsWindow::WindowProc 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 1114] 
USER32.dll + 0x124c (0x77e7124c) 
0x00110001 

-> views (although the actual line number noted doesn't make a lot of sense to 
me).

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Probably caused by Kevin's change to speed up event handling with the
mChildRemoved flag.

Hey, Kevin, is it possible that the following could happen?
1) An event arrives, we set mChildRemoved = PR_FALSE on some view V, we pass the
event to some child
2) The child removes itself (or some other child) from its parent, setting
mChildRemoved = PR_TRUE on V
3) The child also generates a nested event targeted at an ancestor of V
4) Event processing again sets mChildRemoved = PR_FALSE on V, but it now has no
children and does nothing
5) We leave nested event processing and return to outer event processing
6) We test mChildRemoved on V and the result is PR_FALSE, so we try to follow
pKid's nextSibling and die
Dunno if this is the problem here, but it sounds like it COULD happen

Comment 5

[Kevin McCluskey (gone)]

The only synchronous event that could be generated while traversing the view
hierarchy is a paint message and that is handled separately from the
nsView::HandleEvent code. In addition, the VM protects against re-entrant
painting requests.

  NS_ASSERTION(!(PR_TRUE == mPainting), "recursive painting not permitted");
  if (mPainting) {
    mRecursiveRefreshPending = PR_TRUE;
    return;
  }  

All other events must be scheduled and later processed when control returns the
toolkit's message pump dispatch.


I think the problem is I was not checking the aHandled flag after the event
dispatch.  The old code did not call GetChild until after it had checked the
!aHandled flag. Checking the aHandled flag protects against the case where the
view is cycling through it's children and it is destroyed as a result of
dispatching an event to one of it's children. 

old code:

    for (PRInt32 cnt = 0; cnt < numkids && !aHandled; cnt++) 
    {
      nsView *pKid = GetChild(cnt);


So this should fix it

  if (PointIsInside(*pKid, x, y))
  {
   ...
        pKid->HandleEvent(event, 0, aStatus, PR_FALSE, aHandled);
   ...
  }
 
  if (!aHandled) {
        if (mChildRemoved) {
          // a child has been removed as the result of the HandleEvent
          // so pick up at the same relative position in the child list.
          // We may end up skipping over a child depending on which child
          // or the number of children removed from the list, but this should
          // not be an issue since it is unusual to have the views destroyed
          // while traversing this list and the only negative effect will be
          // that the event is dispatched to a view underneath the intended view.
          pKid = GetChild(cnt);
        } else {
          pKid = pKid->GetNextSibling();
        } 
      }
  }

Comment 6

[Kevin McCluskey (gone)]

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

Comment 7

[Kevin McCluskey (gone)]

Attachment: Patch - check aHandled after event dispatch before accessing the view's children

Comment 8

[Marc Attinasi]

Comment on attachment 60209 [details] [diff] [review]
Patch - check aHandled after event dispatch before accessing the view's children

sr=attinasi

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 60209 [details] [diff] [review]
Patch - check aHandled after event dispatch before accessing the view's children

r=roc+moz

Comment 10

[Kevin McCluskey (gone)]

Fix checked in.

Comment 11

[Jay Patel [:jay]]

Verified Fixed.  Adding topcrash keyword and Trunk [@ nsView::GetChild] to
summary.  This *was* a topcrasher with recent MozillaTrunk builds...last crash
reported was with build 2001120309.