8488 - JS calls following window.close() may crash browser

Status: VERIFIED WORKSFORME

(Core :: XUL, defect, P2)

Description

[Bill Law]

In the course of working on bug #8368, I saw that JS code of the form:

window.close();
window.focus();

might cause the browser to crash.  I say "might" because window.close() doesn't
really seem to work yet and I know work is still going on in this area.

However, I wanted to raise this issue so that we don't ignore the potential
problem and enable web pages to crash the browser.

See my comments in bug #8368.

The problem is that when the window is closed, nsWindow::mToolkit is set to
null.  Subsequent function invocations (which apparently can get through)
attempt to use mToolkit.

Note that a simple fix would be to test the mToolkit member of nsWindow prior to
referencing it.

Comment 1

[Peter Trudelle]

reassigning to danm as p2 for M8.  Please post to XPFE newsgroup as needed
to ensure that all potential users are aware of the workaround.

Comment 2

[Dan M]

Something's been changed on the JS side since this bug was filed.  When I close a window, while the
JS variable I had referencing that window can still be referenced and still claims to refer to a window
object, all its properties have been cleared out.  You can't call focus() on it at all, or anything else.
This is kind of disappointing and draconian, but it sure doesn't crash.

Comment 3

[Bill Law]

Here's how I produced the crash.  It still crashes.  The fact that this uses the
obsolescent toolkit core and content maybe won't be able to do it, etc. may make
this all a moot point.  But if you want to see it crash, apply this patch:

Index: openLocation.js
===================================================================
RCS file: /cvsroot/mozilla/xpfe/browser/src/openLocation.js,v
retrieving revision 1.5
diff -r1.5 openLocation.js
46a47
>     toolkit.CloseWindow( window );

Comment 4

[phillip]

clearing resolution

Comment 5

[Dan M]

oh, for the luv o' !!  Alright, if you keep other variables around referencing the window,
their properties won't be cleared when the window is closed.  This is probably a general
problem when using the toolkit to close a window, but applies equally well to non-toolkit
closures if two variables. are used.  I think the fix for this is going to be just awful.

Comment 6

[Bill Law]

Sorry to be the bearer of bad news.

Note that I didn't raise this issue to in any way try to defend the JS code in
question; I just didn't want goofy JS programmers like me to cause the browser
to crash. I think that's a reasonable concern.

Note also that the fix may not be so onerous; a simple check for null in the
method in question fixes this particular scenario (although there may be others
not so easily fixed).

Comment 7

[Dan M]

Delaying until M9. While as Bill says, fixing this one symptom is trivial, it's also a hack that leaves
us with two dozen other equally bad symptoms. I have hopes that the weak pointer code which
is planned to go in soon will help make a more efficient fix than my only other idea, which is prefixing
nearly every function in nsWindow with a check to see whether the object has been deconstructed.
  In the meantime, there's a workaround: in your javascript, don't call focus on a closed window.

Comment 8

[brendan-obsolete]

The native JS method for focus (generated by DOM idlc, IIRC) must defend against
the window being closed.  So must the 19-odd other JS native methods.  But there
should not be tests elsewhere, particularly in the numerous platform-specific
nsWindow impls.

The soonest you know that JS wants to do something to a window object, but that
its underlying native (by that I mean C++, not platform-specific) window data
structure is gone, you should bail out without error, but without intended
effect.  JS users have had a window.closed property they can check since 3.0, I
believe, but they still should be able to call window methods and get and try to
set window properties without crashing.

/be

Comment 9

[Peter Trudelle]

crash -> critical severity

Comment 10

[Peter Trudelle]

Mass-moving non-PDT+ bugs to M13

Comment 11

[Paul MacQuiddy]

giving me rest of phillips open qa contact bugs, sorry for spam

Comment 12

[Christopher Blizzard (:blizzard)]

This might have been fixed with the checkin I just made.  You might want to
check it out.

Comment 13

[Dan M]

I think I'm going to call this one fixed again, though I'll be waiting for Bill to show me
some new way to make it happen that I can't think of right now. Though I'd be interested
in knowing the details of Chris' abovementioned possible fix, the problem still seems fixed
at the JS level, as I mentioned last July.
  When a window is closed, all its properties are cleared. So there is no longer a focus method
to be called. Any attempt to reference it will be properly handled by a JS exception. This
same thing happens to other variables referring to the closed window as well as to the window
variable itself (which seems to have not been true last July, judging from comments above),
so you can't end-run the property cleanup by using another variable.
  Lastly, I believe one can't even use the toolkit core to close a window any more -- it seems
to finally be kind of dead, though I haven't tried *real* hard to resurrect it -- so if there were
specific problems from using the toolkit core to close a window, those are probably cleaned up
by serendipitous fiat.
  In short, I can't make a problem happen here any more, so I'm calling it fixed. I duck and
wait for Bill to show me wrong.

Comment 14

[Prashant Desale]

Bill, would you like to verify this one ?

Comment 15

[Bill Law]

Seeing as how the toolkit core is dead and I can't find anything amiss when
using window.close() followed by window.focus(), I think we're done here.

Comment 16

[Prashant Desale]

Thanks Bill.