53763 - A crash occurs after closing event dialog and source has been replaced in frame. [@ nsEventStateManager::SendFocusBlur]

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Chris Petersen]

Build: 2000092208
Platforms: All
Expected result: After closing the dialog, the source in the second frame should 
be replaced. No crash should occur.

What I got: After second frame src is replace, a onblur dialog appears. Click OK 
in the dialog results in a crash.

Steps to reproduce:

1) Load attached frameset test case.

2) The frameset consists of two col frames.

3) In the second column, select one of the list items in the select menu. This 
select menu has a onblur event assigned.

4) Click on the link in the first frame.

5) Two things happen here: The sorce is replaced in the second frame and a onblur 
event is processed. A alert dialog should appear. Click OK.

6) This should result in crash.

Note:The key is to replace the content src in the second frame to reproduce this 
problem.

Comment 1

[Chris Petersen]

If just click in the first frame (but not on the link) , the dilaog will appear 
and can be closed without crashing. The source in the second frame must be 
replaced.

Comment 2

[Chris Petersen]

Attachment: stdlog file from Macsbug

Comment 3

[Chris Petersen]

Attachment: A frameset with two frames

Comment 4

[Chris Petersen]

Call stack info under Win 98:
 
 Call Stack:    (Signature = 0x40400000 669631f9) 
     
   0x40400000 
                                                  
     
   PresShell::~PresShell 
                                                 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 1268]
     
   PresShell::`scalar deleting destructor' 
                                                  
     
   nsTextInputListener::Release 
                                                 
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsGfxTextControlFrame2.cpp, 
line 225]
     
   nsCOMPtr_base::~nsCOMPtr_base 
                                                 
[d:\builds\seamonkey\mozilla\xpcom\base\nsCOMPtr.cpp, line 50]
     
   nsEventStateManager::SendFocusBlur 
                                                 
[d:\builds\seamonkey\mozilla\layout\events\src\nsEventStateManager.cpp, line 
2687]
     
   nsEventStateManager::SetContentState 
                                                 
[d:\builds\seamonkey\mozilla\layout\events\src\nsEventStateManager.cpp, line 
2484]
     
   nsGenericHTMLElement::HandleDOMEventForAnchors
                                                 
[d:\builds\seamonkey\mozilla\layout\html\content\src\nsGenericHTMLElement.cpp, 
line 1132]
     
   nsHTMLAreaElement::HandleDOMEvent 
                                                 
[d:\builds\seamonkey\mozilla\layout\html\content\src\nsHTMLAreaElement.cpp, line 
235]
     
   nsGenericDOMDataNode::HandleDOMEvent 
                                                 
[d:\builds\seamonkey\mozilla\layout\base\src\nsGenericDOMDataNode.cpp, line 790]
     
   nsCommentNode::HandleDOMEvent 
                                                 
[d:\builds\seamonkey\mozilla\layout\base\src\nsCommentNode.cpp, line 383]
     
   PresShell::HandleEventInternal 
                                                 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 4262]
     
   PresShell::HandleEvent 
                                                 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 4192]
     
   nsView::HandleEvent 
                                                 
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 379]
     
   nsView::HandleEvent 
                                                 
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 352]
     
   nsView::HandleEvent 
                                                 
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 352]
     
   nsViewManager2::DispatchEvent 
                                                 
[d:\builds\seamonkey\mozilla\view\src\nsViewManager2.cpp, line 1429]
     
   HandleEvent 
                                                 
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 68]
     
   nsWindow::DispatchEvent 
                                                 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 618]
     
   nsWindow::DispatchWindowEvent 
                                                 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 635]
     
   nsWindow::DispatchMouseEvent 
                                                 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3813]
     
   ChildWindow::DispatchMouseEvent 
                                                 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4021]
     
   nsWindow::ProcessMessage 
                                                 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 2910]
     
   nsWindow::WindowProc 
                                                 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 884]
     
   KERNEL32.DLL + 0x363b (0xbff7363b) 
                                                  
     
   KERNEL32.DLL + 0x242e7 (0xbff942e7) 
                                                  
     
   0x00688b3e 

Comment 5

[Eric Pollmann]

Attachment: reduced non-frame test case

Comment 6

[Eric Pollmann]

I also see assertions and occasional crashers using the same test case but
putting the link and select in a standard document (no frames).  The problem
seems to be processing events on (ns)frames that have gone away.  Should we kill
the offending event?  Add yet another Kung Fu Deathgrip (tm)?  Passing this to
the events gurus.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Nominating for RTM. Crash is bad, and we already have some ideas on how to fix 
it.

Comment 8

[Nisheeth Ranjan]

Marking [rtm need info] and re-assigning to heikki.  I agree that we should
investigate this for rtm.

Comment 9

[Nisheeth Ranjan]

Really re-assigning to heikki this time.  :-)

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

This is a potential duplicate of bug 54868.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Well, this is not a dup after all. Have to see if the if we could use the same
principle that fixes bug 54868, though...

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix: make sure presshell gets released before viewmanager

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The first proposed fix makes sure that presshell gets released before
viewmanager, which will destroy stuff that presshell is accessing in its
destructor. This fixes the crash.

There is a problem, though. Using the frameset testcase, when you click the link
in the left frame it should update the document in the right frame. It does, but
there is a catch: the select element STAYS ON THE PAGE until you dismiss the
alert dialog. So in effect, you have data from two pages visible at the same
time even though only one page should be displayed.

NN 4.2 does not replace the document in the right frame until the dialog is
dismissed, after dismissal NN actually opens the right frame iin new window. IE
does not replace the right frame at all, even though it displays the alert dialog.

Robert, how is this possible, and can there be other adverse effects?

joki, how could we know that the right frame was replaced and stop the blur
event from bubbling after the user dismisses the alert? I believe this would be
the other way to stop this crash.

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

s/NN 4.2/NN 4.7/

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Hmm, actually it appears that the new frame gets an incomplete paint. Try the
frameset test, and when you have the dialog up you see there is the incorrect
SELECT element in the right frame. Move some window over & out that area and you
will see it is correctly repainted with background color. You will not see this
weird effect if your new document to the right frame is "longer" than the
previous document.

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

joki, any possible adverse effect that I am not doing anything extra to kill the
blur event?

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Does anyone know of a real site that causes this crash? Chris?

Comment 18

[Chris Petersen]

Actually, I don't have a commerical site that reproduces this problem. This bug
was found when running our existing HTML tests in a frameset.

Comment 19

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

The same underlying problem was causing bugs in dynamic theme switching: frames 
--- and even the entire view manager --- were being destroyed in the view 
manager's event handler.

I think the long-term solution is for the view manager to collect all potential 
target views and perform view->frame mapping over them all, then hand off the 
entire resulting list into nsPresShell. nsPresShell should then hand the list 
down into nsEventStateManager for frame->content translation, and only then do 
we actually process events. Thus if frames are destroyed or whatever, the events 
will still be correctly processed.

I can't really help you with this workaround. I don't know much about 
nsPresShell. But I'm not surprised that weird stuff happens when the frame tree 
and view manager disappear halfway through handling an event.

Comment 20

[Nisheeth Ranjan]

I'm afraid, I'm going to have to rtm- this bug based on the feedback from Chris.
 Heikki, please keep this bug in mind.  In case, QA report a ship stopper crash
bug with the same track trace, we can check this fix in to the rtm branch.

Please go ahead and check this fix into the trunk once you've gotten it reviewed
according to Mozilla tree rules.  Thanks!

Comment 21

[Scott Collins]

At Heikki request, I'm just annotating this bug with the comment I added to
related bug #54233.

Just a clarification of the language here: the order of destruction is a
requirement and is guaranteed by C++'s definition.  See in particular section
6.6 Jump statements [stmt.jump], paragraph 2, which reads (in part)

  On exit from a scope (however accomplished), destructors (12.4) are called
  for all constructed objects with automatic storage duration (3.7.2) (named
  objects or temporaries) that are declared in that scope, in the reverse
  order of their declaration. [...]

This is a promise.  All compilers will destroy named variables local to a scope
on exit from that scope in the reverse order or their declaration.  This promise
is similar to the promises about the order of destruction of member variables
and of the elements of an array.  So we _can_ rely on this to fix order
dependent situations, and this is even a recommendation in the |nsCOMPtr| user
guide.  Quoting from paragraph 2 of the section "How do I ... |Release()| an
|nsCOMPtr| before it goes out of scope?"

  You should note, though, that there is a small performance penalty for
  [|Release()|ing an |nsCOMPtr| by assigning |0| into it before destruction].
  The |nsCOMPtr| will still exercize logic in its destructor to attempt to
  |Release()| the value it has at that time.  The optimal solution is to
  arrange the lifetime of your |nsCOMPtr| to correspond to exactly how long
  you want to hold the reference. [followed by an example]

So I designed it with this specific use in mind.

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Nisheeth, I am seeing SendFocusBlur in talkback data, and I believe this is the
bug. Search for "SendFocusBlur" in QuickFind and you will get 69 incidents (as
of right now).

Looking at the reports of what people were doing, it appears as they have this
situation that they have a dialog up caused by some event, and by the time they
submit the dialog the contents have been deleted -> crash.

Most often this seems to happen when the password manager asks if you want to
remember the password for this site. For example logging into
http://my.netscape.com can cause crash (exactly the same situation as in
pollmann's reduced test case). Talkback reports say they were trying to log into
AOL, some forums and MSN pages.

Comment 23

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Adding topcrash keyword since I found this in talkback, and it was also in the
condensed summary posted to NS internal...

Comment 24

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Linux does not have the paint problem.

Comment 25

[joki (gone)]

Looks good to me.  r:joki

Comment 26

[Jay Patel [:jay]]

adding [@ nsEventStateManager::SendFocusBlur] for topcrash tracking and here are 
some entries from talkback for this crash:

nsEventStateManager::SendFocusBlur 1664fa4d
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/events/src/nsEventSta
teManager.cpp line 2732
        Build: 2000101309 CrashDate: 2000-10-13 UptimeMinutes: 150  Total: 150 
        OS: Windows NT  4.0 build 1381
        URL: Vewing Microsoft Page
        Comment: Vewing Microsoft Page
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=19035711
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=19035711

    nsEventStateManager::SendFocusBlur c013dc20
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/events/src/nsEventSta
teManager.cpp line 2732
        Build: 2000101309 CrashDate: 2000-10-13 UptimeMinutes: 6  Total: 157 
        OS: Windows NT  4.0 build 1381
        URL: Vewing Microsoft Page
        Comment: Vewing Microsoft Page
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=19036159
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=19036159

    nsEventStateManager::SendFocusBlur 7c76592a
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/events/src/nsEventSta
teManager.cpp line 2732
        Build: 2000101509 CrashDate: 2000-10-15 UptimeMinutes: 1  Total: 1 
        OS: Windows 98  4.10 build 67766222
        URL: http://aolmail.aol.com/
        Comment: Attempting to access secure page.
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=19137049
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=19137049

Comment 27

[vidur (gone)]

sr=vidur. By introducing the local reference to the view manager, we're
guaranteeing that view manager/presshell destruction happens in the same order
as in the common case, so the change seems reasonable and safe. A similar change
was necessary in the presshell reflow event code. Post NS6, we should reconsider
the current ownership model - this and the reflow event changes should be
considered as short-term band-aids only.

Comment 28

[Nisheeth Ranjan]

Marking rtm+.

Comment 29

[Peter Trudelle]

PDT says rtm++, please land on branch ASAP.

Comment 30

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed on branch & trunk.

Comment 31

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The paint problem is bug 57190.

Comment 32

[Chris Petersen]

Fixed on the Mac Oct 20th branch build. Need to test on Linux and Windows Oct 2O 
th builds.

Comment 33

[Chris Petersen]

Fixed in the Oct 20th Windows branch build.

Comment 34

[Asa Dotzler [:asa]]

Verified Fixed on Mozilla trunk builds
linux 102708 RedHat 6.2
win32 102704 NT 4
mac 102708 Mac OS9
Also Verified on 10/27 branch bits on all 3 platforms
Setting bug to Verified and removing vtrunk keyword