132609 - js window.close() called from event handler does not do that

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[chado]

I'm not sure about Compornent.  
Though bug 121586 has been VERIFIED as WORKSFORME, there is still remaining the 
case to fail closing window.  When I try to close the window, opened by js, 
from keypress event, window is not closed. 

Here is a testcase:
http://www2.wbs.ne.jp/%7Echado/work/js_window_close/js_window_close_3.html

Steps to reproduce:
1) Open the url above in Mozilla browser.  For instructions to follow, open 
   screen space on left-top.  
2) Click "open test window" link.  300 x 300 sized new test-window will be 
   opened by js.
3) Click "close test window" link on first window.  Function including 
   window.close() will be called, then test-window will be closed.  
   (this is the part of fixed in bug 121586)
4) Repeat step 2.  
5) Click "enable keypress" link on first window.  EventListner will start 
   to get keypress event.  
6) Press "x" key while test-window has focus.  Event handler will call the 
   function including window.close(), used in step 3.  

Expected result: test-window should be closed.  
Actual result:  test-window is still there.   

Additional info: 
   In step 6, pressing "w" key, instead of "x" key, will excute window.close() 
   enclosed with setTimeout.  Whether delayed time is not zero or be, 
   test-window will now be closed.  So, current workaround for me is to enclose 
   the window.close() with setTimeout.  
   (strange, is there any serialization problem or some...?)
   Same on click event.  NC4x works as expected.  

Builds I have tested:
  works: Win98 milestones 0.9, 0.9.1, and Mac9 N6.01.
  do not works: Win98 milestones 0.9.2, 0.9.3, 0.9.4, 0.9.5, 
                Mac9  milestones 0.9.6, 0.9.7, 0.9.8, 0.9.9, 
                Mac9  N6.1J, N6.2.1J, 
                Win98 2002-03-21-08-trunk and 
                Mac9  2002-03-21-08-trunk.

seems to be regression from far away.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: The testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

confirming bug

Comment 3

[Johnny Stenback (:jst)]

Attachment: Call nsIScriptContext::ScriptEvaluated() after event handler is fired if there's no JS on the stack.

The problem here is that when we're closing a window through window.close() we
don't really close the window until the script that executed windnow.close() is
done executing and in the case where window.close() called from an event
handler that was added with addEventListener() we don't end up telling the
script context that a "script" is done executing once the event handler is
called. This patch fixes this problem by making the event listener manager tell
the script context that a script is done executing if there's no JS on the
stack when the event handler is called.

Comment 4

[Johnny Stenback (:jst)]

Heikki, Jband, would you guys r/sr?

Comment 5

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

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

Comment 6

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

Comment on attachment 76650 [details] [diff] [review]
Call nsIScriptContext::ScriptEvaluated() after event handler is fired if there's no JS on the stack.

r=heikki, but I would prefer if you inverted the logic regarding the parameter
name, i.e. use |js_running|, and later you can test |if (!js_running)|

Comment 7

[Johnny Stenback (:jst)]

Attachment: Same as above, only different (per heikki's request)

Comment 8

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

Comment on attachment 76674 [details] [diff] [review]
Same as above, only different (per heikki's request)

r=heikki

Comment 9

[John Bandhauer]

Comment on attachment 76674 [details] [diff] [review]
Same as above, only different (per heikki's request)

sr=jband

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 76674 [details] [diff] [review]
Same as above, only different (per heikki's request)

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 11

[Jaime Rodriguez, Jr.]

adt1.0.0-. Let's take a look at this for MachV RTM.

Comment 12

[saari (gone)]

topembed- not an embedding blocker, but it would be a Good Thing to have so embed

Comment 13

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 14

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 15

[Dawn Endico]

Comment on attachment 76674 [details] [diff] [review]
Same as above, only different (per heikki's request)

a=chofmann,brendan
please check in to 1.0 branch asap to get this in rc3.

Comment 16

[Johnny Stenback (:jst)]

Fixed on branch too.

Comment 17

[chado]

I don't know why this has been marked as FIXED, but trunk builds still have 
this problem.
e.g. mac9 2002-05-20-03-trunk

Comment 18

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

Argh, this wasn't landed on the trunk! Reopening.

Comment 19

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

Really this time...

Comment 20

[Johnny Stenback (:jst)]

"Oops" on my part for that, fixed on the trunk now.

Comment 21

[Prashant Desale]

Verified on 2002-05-21-08-trunk.

Comment 22

[Prashant Desale]

verified1.0.0

Comment 23

[Jaime Rodriguez, Jr.]

Posthumus adt1.0.0+, pls get ADT approval prior to checking in to the 1.0 branch.