239563 - crash if window.close() called in javascript such as 'onMouseDown' [@ nsEventStateManager::SendFocusBlur ]

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Phrite]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8

In a popup window, clicking this button will cause firefox to crash:
<button OnMouseDown="javascript:window.close();">Cancel</button>

Reproducible: Always
Steps to Reproduce:
1. Create popup window in javascript
2. click the button made by: <button 
OnMouseDown="javascript:window.close();">Cancel</button>

Actual Results:  
Under Windows: Access violation, dissasembly yields it to be a null reference
exception. 

Expected Results:  
closed the popup

Comment 1

[Tuukka Tolvanen (sp3000)]

confirmed linux seamonkey 1.7b and firefox cvs 20040331

Comment 2

[Tuukka Tolvanen (sp3000)]

Attachment: testcase

Comment 3

[Tuukka Tolvanen (sp3000)]

Attachment: stack

Comment 4

[Hermann Schwab]

Talkback TB11798W
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7b) Gecko/20040316

Comment 5

[Brian Ryner (not reading)]

The cause of the crash is pretty straightforward... the script event handler
(called when nsHTMLButtonElement::HandleDOMEvent calls the superclass
HandleDOMEvent) unhooks the pres shell from the pres context, then the button
element calls SetContentState() which calls SendFocusBlur() which expects it to
be there.

This particular problem could obviously be fixed by null checking in
EventStateManager::SendFocusBlur.  However, it really seems like the right time
to check for destruction is right after we do something that could cause
destruction, like call a script event handler.  That could be a pretty
good-sized change though.  Anyone have any opinions?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Would it make any sense to put destruction off on a PLEvent when window.close()
is called instead of doing it synchronously?

Comment 7

[Brian Ryner (not reading)]

I've considered that before; it would also allow us to eliminate
kungFuDeathGrips on various objects around dispatching an event and some other
null checks we do after dispatching events.

We'd probably need to synchronously call some method on the window object so
that it doesn't respond to any further method calls...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm... GlobalWindowImpl::Close() already posts a close event unless the caller
is chrome (why unless??).  Except if there is a JS context on the stack, we
close off the JS context's termination function instead of off the event.  We
should probably close off the event unconditionally.

So perhaps the thing to do is:

1)  Set a termination function that will call
    nsIScriptGlobalObject::SetScriptsEnabled(PR_FALSE) (to prevent JS stuff in
    the window from running).
2)  Close for real off the event.
3)  (maybe) add some mIsClosed checks in various places.

Comment 9

[Tuukka Tolvanen (sp3000)]

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

Comment 10

[Olivier Cahagne]

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

Comment 11

[Jerry Lampi]

No solution, but some more clues:

When a main page opens a child window containing an applet and the applet closes
the child window:
1) When the applet in child window calls:
	JSObject win = JSObject.getWindow(this);
	win.eval("setTimeout(\"window.close()\",200);");
The window closes and everything is OK.

2) When the applet in child window calls:
	JSObject win = JSObject.getWindow(this);
	win.eval("window.close();");
The window closes BUT FireFox is dead as described in this bugzilla.


When a main page opens a child containing HTML that closes the child: 
1. Click on linked text (N.B.: class closeButton is CSS simulating the look of a
close button). OK. No crash. 
	<a class="closeButton" href="#null" onclick="javascript:window.close();">Close</a>

2. Click on a linked image. OK. No crash. 
	<a href="#null" onclick="javascript:window.close();"><img src="closeButton.gif"
border="0"/></a> 

3. Click on a scripted image. OK. No crash. 
	<img src="closeButton.gif" onclick="javascript:window.close();"/> 

4. Click on a button that is not inside a form.  FAILS crash. 
	<p><input type="button" value="close" onclick="javascript:window.close();"/></p> 

5. Click on a button inside a form.  FAILS crash. 
	<form><input type="button" value="close"
onclick="javascript:window.close();"/></form> 

6. Click on a button inside a form that calls a function.  FAILS crash.  
	<form><input type="button" value="close" onclick="closeFun();"/></form>

7. Click on a button inside a form that calls a delayed function.  FAILS crash. 
	<form><input type="button" value="close" onclick="closeFunDelay();"/></form>

Comment 12

[Boris Zbarsky [:bzbarsky]]

One other interesting thought...  What if the mousedown JS were in an iframe,
and instead of calling close() it removed the iframe from its parent document? 
As I recall, that currently causes immediate destruction of the window and
document in the iframe; that ought to lead to crashes similar to this, right?

Comment 13

[Phil Norcross]

Here's a pretty simple work-around.
When the document loads, redefine the listener that fires
when you push a "Close" button.
And write the listener to immediately stop event propagation.
//-----------------------------------------------------
function fixAtLoad(){
   var ara=document.getElementsByTagName("input");
   for(i=0;i<ara.length;i++){
      if((ara[i].getAttribute("type").toLowerCase()=="button")
         &&(ara[i].getAttribute("value").toLowerCase()=="close")){
            ara[i].onclick=null;
            ara[i].addEventListener("mousedown",closeButton,true);
}}}//if, for, fix
//-----------------------------------------------------
function closeButton(e){
   e.preventDefault();
   e.stopPropagation();
   window.close();
}//closeButton
//-----------------------------------------------------

Comment 14

[Brian Ryner (not reading)]

Attachment: patch per boris's suggestion

This fixes the crash for me... look ok?

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 174316 [details] [diff] [review]
patch per boris's suggestion

Actually I'd like jst to look at this too.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase per comment 12

Brian, your patch doesn't fix this testcase, does it?

Comment 17

[Brian Ryner (not reading)]

(In reply to comment #16)
> Created an attachment (id=174340) [edit]
> Testcase per comment 12
> 
> Brian, your patch doesn't fix this testcase, does it?

No. Maybe to fix that we should add similar code in nsSubDocumentFrame::Destroy
(in particular, to defer the call to nsDocumentViewer::Hide()).

Comment 18

[Boris Zbarsky [:bzbarsky]]

What if instead of removing the iframe I just changed its display or position
(while keeping it in the same place), and forced out the style change (causing a
frame reconstruct)?  If we keep the old viewer around, wouldn't that mean that
we end up dispatching events after the mousedown to the wrong (about to die)
frame tree?  (Maybe this is not an issue; I'm just being an evil tester here).

Comment 19

[Brian Ryner (not reading)]

Attachment: "safe" patch

Let's just null check as needed until we can unify presshell and prescontext.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174408 [details] [diff] [review]
"safe" patch

r+sr=bzbarsky

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 174408 [details] [diff] [review]
"safe" patch

a=asa for checkin to 1.8b

Comment 22

[Daniel Veditz [:dveditz]]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174316 [details] [diff] [review]
patch per boris's suggestion

Seems reasonable...

Comment 24

[Brian Ryner (not reading)]

This was checked in, forgot to mark it fixed.

Comment 25

[Brian Ryner (not reading)]

Oops, my bad.  This was _not_ checked in previously.  I just checked it in now.

Someone with appropriate permissions, please clear the blocking flag.

Comment 26

[Hideo Oshima]

Thanks. Today's trunk build doesn't have this problem.
But in the case browser.link.open_newwindow is 0, 
Mozilla still crashes.
Is this another bug?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Possibly, yes.  File separately, cc me and bryner?  Post talkback incident ID if
you have one?

Comment 28

[Hideo Oshima]

Today's build doesn't crash with any value
of browser.link.open_newwindow.
Other bug may fix also this bug.
Thanks.