38951 - accessing window properties on already closed window JS object causes silent failure

Status: RESOLVED WONTFIX

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

Description

[Christopher Blizzard (:blizzard)]

If you call any methods on an already closed window object the script will
silently fail.  Test scripts are attached.  To reproduce it click "New Window",
"Close Window", and then "Close Window" again.  There's no JS error like there
should be.  With some brendan gdb love here's a stack trace that's at
"js_Interpret at the out: label only if !ok && !cx->throwing."

Breakpoint 1, js_Interpret (cx=0x819d140, result=0xbfffe83c)
    at ../../../mozilla/js/src/jsinterp.c:3374
3374        if (!ok && cx->throwing) {
(gdb) where
#0  js_Interpret (cx=0x819d140, result=0xbfffe83c)
    at ../../../mozilla/js/src/jsinterp.c:3374
#1  0x405b6055 in js_Invoke (cx=0x819d140, argc=1, flags=2)
    at ../../../mozilla/js/src/jsinterp.c:702
#2  0x405b638c in js_InternalInvoke (cx=0x819d140, obj=0x80e8010, 
    fval=135168296, flags=0, argc=1, argv=0xbfffeae8, rval=0xbfffe9b4)
    at ../../../mozilla/js/src/jsinterp.c:775
#3  0x40589b57 in JS_CallFunctionValue (cx=0x819d140, obj=0x80e8010, 
    fval=135168296, argc=1, argv=0xbfffeae8, rval=0xbfffe9b4)
    at ../../../mozilla/js/src/jsapi.c:2794
#4  0x4074d4d7 in nsJSContext::CallEventHandler (this=0x819d108, 
    aTarget=0x80e8010, aHandler=0x80e8128, argc=1, argv=0xbfffeae8, 
    aBoolResult=0xbfffea38, aReverseReturnResult=0)
    at ../../../../mozilla/dom/src/base/nsJSEnvironment.cpp:787
#5  0x40793cbc in nsJSEventListener::HandleEvent (this=0x826c2a0, 
    aEvent=0x8110dbc)
    at ../../../../mozilla/dom/src/events/nsJSEventListener.cpp:154
#6  0x41068181 in nsEventListenerManager::HandleEventSubType (this=0x82814f0, 
    aListenerStruct=0x829ce40, aDOMEvent=0x8110dbc, aSubType=4, aPhaseFlags=7)
    at ../../../../mozilla/layout/events/src/nsEventListenerManager.cpp:703
#7  0x410687de in nsEventListenerManager::HandleEvent (this=0x82814f0, 
    aPresContext=0x81b19f8, aEvent=0xbfffefdc, aDOMEvent=0xbfffee94, aFlags=7, 
    aEventStatus=0xbffff48c)
---Type <return> to continue, or q <return> to quit---
    at ../../../../mozilla/layout/events/src/nsEventListenerManager.cpp:843
#8  0x41349418 in nsGenericElement::HandleDOMEvent (this=0x82b31f4, 
    aPresContext=0x81b19f8, aEvent=0xbfffefdc, aDOMEvent=0xbfffee94, aFlags=1, 
    aEventStatus=0xbffff48c)
    at ../../../../mozilla/layout/base/src/nsGenericElement.cpp:1073
#9  0x4113484d in nsHTMLInputElement::HandleDOMEvent (this=0x82b31d8, 
    aPresContext=0x81b19f8, aEvent=0xbfffefdc, aDOMEvent=0x0, aFlags=1, 
    aEventStatus=0xbffff48c)
    at ../../../../../mozilla/layout/html/content/src/nsHTMLInputElement.cpp:690
#10 0x410ca3e7 in PresShell::HandleEventInternal (this=0x823cb48, 
    aEvent=0xbfffefdc, aView=0x0, aStatus=0xbffff48c)
    at ../../../../../mozilla/layout/html/base/src/nsPresShell.cpp:3418
#11 0x410ca2dc in PresShell::HandleEventWithTarget (this=0x823cb48, 
    aEvent=0xbfffefdc, aFrame=0x826deb0, aContent=0x82b31e4, 
    aStatus=0xbffff48c)
    at ../../../../../mozilla/layout/html/base/src/nsPresShell.cpp:3399
#12 0x41071304 in nsEventStateManager::CheckForAndDispatchClick (
    this=0x828f408, aPresContext=0x81b19f8, aEvent=0xbffff590, 
    aStatus=0xbffff48c)
    at ../../../../mozilla/layout/events/src/nsEventStateManager.cpp:1646
#13 0x4106ea08 in nsEventStateManager::PostHandleEvent (this=0x828f408, 
    aPresContext=0x81b19f8, aEvent=0xbffff590, aTargetFrame=0x826deb0, 
---Type <return> to continue, or q <return> to quit---
    aStatus=0xbffff48c, aView=0x8286760)
    at ../../../../mozilla/layout/events/src/nsEventStateManager.cpp:760
#14 0x410ca54c in PresShell::HandleEventInternal (this=0x823cb48, 
    aEvent=0xbffff590, aView=0x8286760, aStatus=0xbffff48c)
    at ../../../../../mozilla/layout/html/base/src/nsPresShell.cpp:3438
#15 0x410ca039 in PresShell::HandleEvent (this=0x823cb48, aView=0x8286760, 
    aEvent=0xbffff590, aEventStatus=0xbffff48c, aHandled=@0xbffff430)
    at ../../../../../mozilla/layout/html/base/src/nsPresShell.cpp:3353
#16 0x416b8e87 in nsView::HandleEvent (this=0x8286760, event=0xbffff590, 
    aEventFlags=8, aStatus=0xbffff48c, aHandled=@0xbffff430)
    at ../../../mozilla/view/src/nsView.cpp:761
#17 0x416b8e10 in nsView::HandleEvent (this=0x82886c0, event=0xbffff590, 
    aEventFlags=8, aStatus=0xbffff48c, aHandled=@0xbffff430)
    at ../../../mozilla/view/src/nsView.cpp:745
#18 0x416b8e10 in nsView::HandleEvent (this=0x821b0a0, event=0xbffff590, 
    aEventFlags=28, aStatus=0xbffff48c, aHandled=@0xbffff430)
    at ../../../mozilla/view/src/nsView.cpp:745
#19 0x416cbc33 in nsViewManager2::DispatchEvent (this=0x821c098, 
    aEvent=0xbffff590, aStatus=0xbffff48c)
    at ../../../mozilla/view/src/nsViewManager2.cpp:1372
#20 0x416b6904 in HandleEvent (aEvent=0xbffff590)
    at ../../../mozilla/view/src/nsView.cpp:68
#21 0x40956f46 in nsWidget::DispatchEvent (this=0x82901c0, aEvent=0xbffff590, 
---Type <return> to continue, or q <return> to quit---
    aStatus=@0xbffff528)
    at ../../../../mozilla/widget/src/gtk/nsWidget.cpp:1438
#22 0x40956b7c in nsWidget::DispatchWindowEvent (this=0x82901c0, 
    event=0xbffff590) at ../../../../mozilla/widget/src/gtk/nsWidget.cpp:1329
#23 0x40957000 in nsWidget::DispatchMouseEvent (this=0x82901c0, 
    aEvent=@0xbffff590) at ../../../../mozilla/widget/src/gtk/nsWidget.cpp:1465
#24 0x40958965 in nsWidget::OnButtonReleaseSignal (this=0x82901c0, 
    aGdkButtonEvent=0x81652e8)
    at ../../../../mozilla/widget/src/gtk/nsWidget.cpp:2227
#25 0x4095e760 in nsWindow::HandleGDKEvent (this=0x82901c0, event=0x81652e8)
    at ../../../../mozilla/widget/src/gtk/nsWindow.cpp:1117
#26 0x4094e471 in dispatch_superwin_event (event=0x81652e8, window=0x82901c0)
    at ../../../../mozilla/widget/src/gtk/nsGtkEventHandler.cpp:937
#27 0x4094e0e4 in handle_gdk_event (event=0x81652e8, data=0x0)
    at ../../../../mozilla/widget/src/gtk/nsGtkEventHandler.cpp:782
#28 0x402da4db in gdk_event_dispatch () from /usr/lib/libgdk-1.2.so.0
#29 0x4030b186 in g_main_dispatch () from /usr/lib/libglib-1.2.so.0
#30 0x4030b751 in g_main_iterate () from /usr/lib/libglib-1.2.so.0
#31 0x4030b8f1 in g_main_run () from /usr/lib/libglib-1.2.so.0
#32 0x4022f5b9 in gtk_main () from /usr/lib/libgtk-1.2.so.0
#33 0x804961e in main (argc=2, argv=0xbffff864)
    at ../../../../../mozilla/embedding/browser/gtk/tests/TestGtkEmbed.cpp:96
#34 0x4047a9cb in __libc_start_main (main=0x8049590 <main>, argc=2, 
---Type <return> to continue, or q <return> to quit---
    argv=0xbffff864, init=0x80490ec <_init>, fini=0x804ab5c <_fini>, 
    rtld_fini=0x4000ae60 <_dl_fini>, stack_end=0xbffff85c)
    at ../sysdeps/generic/libc-start.c:92
(gdb) print *script
$1 = {code = 0x816431c ";", length = 94, main = 0x816431c ";", 
  version = JSVERSION_1_5, atomMap = {vector = 0x8163ae0, length = 10}, 
  filename = 0x8163ac8 "file:///tmp/test.js", lineno = 18, depth = 4, 
  notes = 0x8165020 "Πο`\017Τ\013Σg\aά\027 ", trynotes = 0x8165000, 
  principals = 0x812b4a0, object = 0x0}
(gdb) print script->filename
$2 = 0x8163ac8 "file:///tmp/test.js"
(gdb) print script->linen
There is no member named linen.
(gdb) print script->lineno
$3 = 18
(gdb) print js_CodeSpec[op]
$4 = {name = 0x40610302 "getprop", token = 0x0, length = 3 '\003', 
  nuses = 1 '\001', ndefs = 1 '\001', prec = 11 '\013', format = 34}
(gdb) print pc[1]
$5 = 0 '\000'
(gdb) print pc[2]
$6 = 5 '\005'
(gdb) print *(JSString*)((long)script->atomMap[5]->entry.key - 4)
Structure has no component named operator[].
(gdb) p *(JSString*)((long)script->atomMap.vector[5]->entry.key-4)
$7 = {length = 5, chars = 0x81e30f8}
(gdb) x/10c $y.chars
Attempt to extract a component of a value that is not a structure.
(gdb) x/10c $7.chats
There is no member named chats.
(gdb) x/10c $7.chars
0x81e30f8:      99 'c'  0 '\000'        108 'l' 0 '\000'        111 'o' 0
'\000'115 's' 0 '\000'
0x81e3100:      101 'e' 0 '\000'

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: test html

Comment 2

[Christopher Blizzard (:blizzard)]

Attachment: test js

Comment 3

[leger]

Putting on [dogfood-] radar.

Comment 4

[Prashant Desale]

I can see it on Win-95 too.
Attaching one combined testcase created out of above two testcases and with 
alerts, to check on win.

Comment 5

[Prashant Desale]

Attachment: testcase with alerts.

Comment 6

[Christopher Blizzard (:blizzard)]

dogfood-?  Doesn't even netcenter depend on window.closed?

Comment 7

[Brendan Eich [:brendan]]

I can't believe this isn't dogfood or nsbeta2+ -- ekrock, what do you think?

/be

Comment 8

[ekrock's old account (dead)]

Clearing [dogfood-] to trigger re-evaluation. Marking nsbeta2. Yes, this is the 
kind of scary DOM0 backward compatibility bug I was referring to at the 
leads meeting; per the discussion we had at the meeting and the beta 2 criteria, 
this kind of bug should be marked nsbeta2. We need the window object implemented 
with full backward compatibility.

Comment 9

[Christopher Blizzard (:blizzard)]

Here's a scary example of why this bug in particular is bad:

If you call window.close() followed by an 'if (window.closed) {...}' from
another part of your script it will break your script, silently exiting the
function that called it.  It's bad(tm).

I know people have webpages that do this.

Comment 10

[Phil Peterson]

[dogfood-] [nsbeta2+]

Comment 11

[Nisheeth Ranjan]

Just spoke to Johnny.  He will check in a patch that shows a warning in the JS 
console when properties on an already closed window JS object are accessed.  The 
real fix for this bug is high risk and we will wait till beta 3 for that.  
Johnny will mark this bug nsbeta2- when he checks in his patch.

Nominating bug for nsbeta3...

Comment 12

[Johnny Stenback (:jst)]

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

Comment 13

[Johnny Stenback (:jst)]

I just checked in a partial fix for this problem, simply adding the JS error
message turned out to be tricky since when a window is closed in mozilla the
whole window is torn down, including the script context, the document and just
about everything else around the window. Due to the lack of a script context the
failure was silent.

The reason for the failure here was that when a property or method on a closed
window was accessed mozilla did a security check to check if the caller has
access to the window. This access failed because the security check gets the
principal from the document, but since there's no longer a document in the
window the security manager is unabel to get the principal and fails. As a fix
for this problem I made the window hold on to the pricipal of the last document
in the window, this way the security check succeeds even after the document no
longer exists, and the privilegies for the window are based on the last document
the window contained before it was closed. With the fix DOM0 properies on a
closed window, i.e. window.closed n' friends, are accessible.

Marking nsbeta2- since the immediate problem is fixed and changing the whole
window teardown process is not a low risk change, especially WRT mem leaks...

Comment 14

[Nisheeth Ranjan]

Does anybody on the cc list have an idea of use cases out there that won't work 
because of this bug.  Johnny and I are considering futuring this bug and want to 
make as informed a decision as possible.

Comment 15

[Christopher Blizzard (:blizzard)]

I had a test case for it when I was testing some window.open() and
window.close() code.  It's easy to write.

Comment 16

[Brendan Eich [:brendan]]

You've got to be kidding me.  I'll get back to you with "use cases" (real URLs, 
quotes from Danny Goodman books, etc.), but this is basic functionality that 
must work.  What's more, the completely wrong destruction of the window's script 
object on close leads to assertbotches and mystery bugs that plague us still -- 
please read bug 43466.

This is not a bug we can blow off, trust me.

/be

Comment 17

[Alec Flett]

adding myself to cc (this bug is screwing with 43466, which I reported)

Comment 18

[ekrock's old account (dead)]

Agree with brendan. We need to fix this and provide backward compatibility. Note 
blizzard's comment above.

If necessary, please Future other bugs that are lower priority than DOM0 
backward compatibility (XHTML-related fixes, for example) in order to get this 
nsbeta3+. Thanks!

Comment 19

[Nisheeth Ranjan]

Marking this nsbeta3+ per Brendan and Eric Krock's comments...

Comment 20

[ekrock's old account (dead)]

This bug is extremely hard to fix and carries the risk of creating memory leaks
if we make any mistakes. Content developers can work around the lack of support
for this by getting the needed info from the document *before* the window is
closed and then closing it. We don't have evidence in this bug report that a
large number of sites depend on this functionality. Therefore, we are FUTUREing
this bug since the Netscape engineer it's assigned to is overburdened. If you
feel this decision should be changed, please provide evidence for the extent of
actual usage on the web. helpwanted, [nsbeta3-].

Comment 21

[Brendan Eich [:brendan]]

Bah, I'll fix this.  It's ridiculous to expect people to rewrite perfectly valid 
scripts to copy variables back to openers, especially if there's no good event 
handler from which to do so (dialogs get closed by users all the time).  This is 
a bad 4xp bug.

/be

Comment 22

[sean echevarria]

spam: cc self

Comment 23

[Brendan Eich [:brendan]]

danm and I are close to a fix of this bug and 43466 (which got nsbeta3+'d while
this one did not, go figger).

/be

Comment 24

[Brendan Eich [:brendan]]

I've been working on higher priority stuff, but still hope to get to this and a 
few like it.

/be

Comment 25

[selmer (gone)]

Marking [rtm need info] since you claim to be working on this.  The train has
almost left the station and there is no patch attached to this bug yet...

Comment 26

[Brendan Eich [:brendan]]

Dammit, the right fix results in a gigundo-leak.  Someone has a strong ref that
should be weak.  Removing rtm nomination, setting mozilla0.9 milestone.

/be

Comment 27

[Brendan Eich [:brendan]]

Need to fix this for Mozilla 1.0 at the latest, and I don't want to let it
linger unattended.  I'll get to it after netscape6/mozilla0.6 topcrash-like bugs
are not first priority.

/be

Comment 28

[Brendan Eich [:brendan]]

Out of time, sorry.

Comment 29

[Brendan Eich [:brendan]]

Load balancing.

/be

Comment 30

[Brendan Eich [:brendan]]

I won't get to this during 0.9.4 in all likelihood.  Nominating for 1.0.

/be

Comment 31

[Brendan Eich [:brendan]]

Moving out.

/be

Comment 32

[Brendan Eich [:brendan]]

Footprint/perf takes precedence.

/be

Comment 33

[Brendan Eich [:brendan]]

jst, any help or ideas?

/be

Comment 34

[Johnny Stenback (:jst)]

Oh, this bug again :-) So there's a fundamental problem in mozilla that makes it
hard to use closed window objects from JS. The problem is that once a window is
closed, we'll clear the window's scope and throw away the document. We used to
throw exceptions when this was done since we couldn't get a principal from the
closed window (since that was always gotten from the document, which was no
longer around), but now the window code caches the principal from the last
document that was shown in the window, so a principal should be accessible even
after a window has been closed now. Fixing this would take some rather scary
plumbing in the window/document/docshell/documentviewer teardown code, and that
code is extremely fragile leak-wise, so it won't be fun.

To do this right, we would need to make the document in a window stay around
after window.close() is called, and we'd have to make sure that everything is
properly cleaned up once all JS objects with references to the closed window are
collected (by the GC). Doable, but nobody *really* knows all the circular strong
references n' all that's involved in this code...

Comment 35

[Brendan Eich [:brendan]]

I hesitate to ask, but what do other browsers, IE, Opera, and Konqueror for
example, do?

/be

Comment 36

[Brendan Eich [:brendan]]

Unless I hear "other browsers do this right, Moz must too" soon, I'm going to
Future this bug.

/be

Comment 37

[Christopher Blizzard (:blizzard)]

The last time that I tested this, it did the right thing on other browsers.  I
don't have access to those machines at the moment (I'm on the road) but I can
when I get home later today.

Comment 38

[Bob Clary [:bc] (inactive)]

Attachment: testcase

Comment 39

[Bob Clary [:bc] (inactive)]

window.closed works for me in 2002-02-16-08/win2k trunk build.
open testcase, it will open a new window. click on the link in the test case to
see that window.closed is false. close the new window and click on the link to
see window.closed is true.

Comment 40

[Johnny Stenback (:jst)]

Accessing window.closed on a closed window in mozilla should work, but the
document in the closed window is gone in mozilla where the document is
accessable in other browsers, IIRC.

Comment 41

[Bob Clary [:bc] (inactive)]

I just modified the test case to alert the window document of the closed window
and get an exception in IE55/win2k.

Comment 42

[Brendan Eich [:brendan]]

How about window-level variables created by scripts?  Can those be accessed
after close on other browsers?  I believe 4.x let you do that, 2.x and 3.x for sure.

/be

Comment 43

[Bob Clary [:bc] (inactive)]

IE 5.5/win2k throws an exception when accessing window level user defined vars
in a closed window. 

Opera 6/win2k however does not throw an exception and does return the original
values, but Opera's implementation of JS is horked to say the least. ;-)

Nav 4.79/win2k returns undefined for the properties.

Comment 44

[Brendan Eich [:brendan]]

A bold decision.

/be