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

VERIFIED FIXED in mozilla1.0

Status

()

VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: chado_moz, Assigned: jst)

Tracking

({embed, regression, topembed-})

Trunk
mozilla1.0
embed, regression, topembed-
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [HAVE FIX] [ADT3 RTM],custrtm-)

Attachments

(3 attachments)

(Reporter)

Description

17 years ago
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.
confirming bug
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

17 years ago
Created attachment 76650 [details] [diff] [review]
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.
(Assignee)

Comment 4

17 years ago
Heikki, Jband, would you guys r/sr?
Severity: minor → normal
Status: NEW → ASSIGNED
Keywords: 4xp, mozilla1.0, nsbeta1
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla1.0
*** Bug 107321 has been marked as a duplicate of this bug. ***
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)|
Attachment #76650 - Flags: review+
(Assignee)

Comment 7

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

r=heikki
Attachment #76674 - Flags: review+

Comment 9

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

sr=jband
Attachment #76674 - Flags: superreview+
Keywords: nsbeta1 → adt1.0.0, nsbeta1+

Comment 10

17 years ago
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
Attachment #76674 - Flags: approval+

Comment 11

17 years ago
adt1.0.0-. Let's take a look at this for MachV RTM.
Keywords: adt1.0.0 → adt1.0.0-, topembed
Whiteboard: [HAVE FIX] → [HAVE FIX] [ADT3 RTM]

Comment 12

17 years ago
topembed- not an embedding blocker, but it would be a Good Thing to have so embed
Keywords: topembed → embed, topembed-

Comment 13

17 years ago
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email adt@netscape.com.  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1-

Comment 14

17 years ago
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email adt@netscape.com.  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1+
Keywords: nsbeta1- → nsbeta1+
Keywords: adt1.0.0- → adt1.0.0

Comment 15

17 years ago
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.
(Assignee)

Comment 16

17 years ago
Fixed on branch too.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Keywords: fixed1.0.0
Resolution: --- → FIXED
(Reporter)

Comment 17

17 years ago
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
Argh, this wasn't landed on the trunk! Reopening.
Really this time...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

17 years ago
"Oops" on my part for that, fixed on the trunk now.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 21

17 years ago
Verified on 2002-05-21-08-trunk.
Status: RESOLVED → VERIFIED

Comment 22

17 years ago
verified1.0.0
Keywords: verified1.0.0

Comment 23

17 years ago
Posthumus adt1.0.0+, pls get ADT approval prior to checking in to the 1.0 branch.
Keywords: adt1.0.0 → adt1.0.0+

Updated

17 years ago
Whiteboard: [HAVE FIX] [ADT3 RTM] → [HAVE FIX] [ADT3 RTM],custrtm-
You need to log in before you can comment on or make changes to this bug.