115969 - Trunk, M097 crash after File->Exit; onunload calls window.open during shutdown [@ nsWindowSH::GetProperty]

Status: VERIFIED FIXED

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

Description

[John Morrison]

Hmm, I thought we had code that prevented us from opening windows once
Shutdown had begun. Apparently not (although damn ... I had to look for a long
time to find a non-porno site that did this so I could report this bug :-]).

Steps to reproduce:
0) delete (or rename) cookies.txt, so you're a "new" user.
1) start browser; load http://www.cnn.com/
2) click on the 'Sports' link in the upper left hand column 
   (goes to http://sportsillustrated.cnn.com/)
3) click on the 'Hockey' link in the upper left hand column
   (goes to http://sportsillustrated.cnn.com/hockey/)
4) Do File->Exit


Actual results:

  The browser window goes away on shutdown, but a popup ad begins to 
  come up and then crashes. Crash is in line 2782, nsDOMClassInfo.cpp, 
  although this should not be getting called, no?

      nsCOMPtr<nsIXPConnectWrappedNative> wrapper;
      sXPConnect->GetWrappedNativeOfJSObject(cx, JSVAL_TO_OBJECT(*vp),
                                             getter_AddRefs(wrapper));


  with sXPConnect == 0x00000000

This crashes in a win2k trunk build, pulled ~7pm 12/18 (after the redness 
ended).

I don't really know who should get this bug, but I'll take an initial guess 
at danm.

nsWindowSH::GetProperty(nsWindowSH * const 0x01ae76f8, 
nsIXPConnectWrappedNative * 0x01b00028, JSContext * 0x02c194a0, JSObject * 
0x01c57de8, long 0x00b25bdc, long * 0x0012f924, int * 0x0012f7cc) line 2786 + 
17 bytes
XPC_WN_Helper_GetProperty(JSContext * 0x00000001, JSObject * 0x01c57de8, long 
0x00b25bdc, long * 0x0012f924) line 785
js_Interpret(JSContext * 0x02c194a0, long * 0x0012f9b0) line 2891 + 90 bytes
js_Invoke(JSContext * 0x00000001, unsigned int 0x00000004, unsigned int 
0x00000002) line 849 + 10 bytes
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x02e38a88, 
nsXPCWrappedJS * 0x04c33298, unsigned short 0x0003, const nsXPTMethodInfo * 
0x0215f440, nsXPTCMiniVariant * 0x0012fc44) line 1216 + 16 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x02c33298, unsigned short 
0x0003, const nsXPTMethodInfo * 0x0215f440, nsXPTCMiniVariant * 0x0012fc44) 
line 430
PrepareAndDispatch(nsXPTCStubBase * 0x0215f440, unsigned int 0x00000003, 
unsigned int * 0x0012fcfc, unsigned int * 0x0012fcec) line 115 + 18 bytes
SharedStub() line 139
nsDocLoaderImpl::FireOnStateChange(nsDocLoaderImpl * const 0x01c58098, 
nsIWebProgress * 0x022cae84, nsIRequest * 0x0227ee40, int 0x00010010, unsigned 
int 0x804b0002) line 1110
nsDocLoaderImpl::doStopURLLoad(nsDocLoaderImpl * const 0x01c58098, nsIRequest * 
0x0227ee40, unsigned int 0x804b0002) line 715
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x022cae74, nsIRequest * 
0x0227ee40, nsISupports * 0x00000000, unsigned int 0x804b0002) line 568
nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x022cae74, nsIRequest * 
0x00000000, nsISupports * 0x00000000, unsigned int 0x804b0002) line 525 + 13 
bytes
nsLoadGroup::Cancel(nsLoadGroup * const 0x00000001, unsigned int 0x804b0002) 
line 248
nsDocLoaderImpl::Stop(nsDocLoaderImpl * const 0x00000000) line 293 + 14 bytes
nsDocLoaderImpl::Stop(nsDocLoaderImpl * const 0x00000001) line 289
nsDocLoaderImpl::Stop(nsDocLoaderImpl * const 0x00000001) line 289
nsDocLoaderImpl::Destroy(nsDocLoaderImpl * const 0x00b55f20) line 412
nsDocLoaderImpl::~nsDocLoaderImpl(nsDocLoaderImpl * const 0x01c58098) line 184
nsDocLoaderImpl::`scalar deleting destructor'() + 8 bytes
nsDocLoaderImpl::Release(nsDocLoaderImpl * const 0x00b55f20) line 206 + 40 
bytes
nsCOMPtr_base::assign_with_AddRef(nsCOMPtr_base * const 0x01c58098, nsISupports 
* 0x0032b050) line 74
PL_DHashTableEnumerate(PLDHashTable * 0x00000198, int (PLDHashTable *, 
PLDHashEntryHdr *, unsigned int, void *)* 0x1002ab28 
FreeServiceFactoryEntryEnumerate(PLDHashTable *, PLDHashEntryHdr *, unsigned 
int, void *), void * 0x00000000) line 601 + 15 bytes
nsComponentManagerImpl::FreeServices(nsComponentManagerImpl * const 0x01c58098) 
line 1714 + 13 bytes
NS_ShutdownXPCOM(nsIServiceManager * 0x00000000) line 553
main(int 0x00000001, char * * 0x00323c08) line 1605
WinMain(HINSTANCE__ * 0x00400000, HINSTANCE__ * 0x00400000, char * 0x00133488, 
HINSTANCE__ * 0x00400000) line 1612 + 21 bytes
MOZILLA! WinMainCRTStartup + 308 bytes

Comment 1

[John Morrison]

simpler test case http://jrgm/bugs/115969/window-onunload.html

<html>
<head>
<script>
    function newWindow () {
        window.open('http://www.mozilla.org/');
    }
    window.onunload = newWindow;
</script>
</head>
<body>
    <p>  do "File->Exit".   Crashes?   </p>
</body>
</html>

Comment 2

[Johnny Stenback (:jst)]

Hmm, what does 4x/IE do when you exit on a page with a onunload handler? Does it
even call the onunload handler, or does it just prevent opening new windows?
What about alert() n' other modal dialogs?

Comment 3

[greer]

Not too surprisingly, this one shows up 49 times in the Trunk topcrash data for 
the last 10 days (on Win and Linux). Marking as topcrash.

Comment 4

[John Morrison]

Huh? Really? This shows up as a top crash [wasn't me; I didn't submit any. Is
this just people crashing for kicks based on this bug report, or is it real
users stumbling into this (i.e., do the comments indicate any knowledge of 
this bug?)].

Anyways, to answer jst's question (at least for win32), nav4.x will 
successfully open the new window, closing the initial window but aborting 
the shutdown. 

For an alert fired onunload (http://jrgm/bugs/115969/alert-onunload.html), 
nav4.x will apparently refuse to show the alert and shutdown completes. 
Mozilla on the other hand, will show the alert, and not shutdown (no crash),
leaving nothing but the alert showing on screen (original window is closed).

Maybe amar can concoct the tests for other scenarios (what if multiple windows 
are open) and try them in 4.x/ie/moz for mac/linux too.

Comment 5

[Dan M]

Porn sites and CNN, eh?

Despite Nav4's terribly clever precedent, I say we should just turn off the
ability to open new windows in an onunload handler. I can't think of a
legitimate reason to allow that. As a user, if I said "close," damnit, you heard me.

I'm attaching a patch to do this. Note this doesn't affect "save before
closing?" dialogs, like Composer's, for instance. That happens before window
shutdown has actually begun. This patch prevents a window that's already
irreversibly deep in the throes of undergoing final dissolution from chucking
new windows at you from beyond the grave. Seems like a win.

Comment 6

[Dan M]

Attachment: disallow a window being destroyed from opening new windows

Comment 7

[jag (Peter Annema)]

Looks okay to me, so it's up to jst. You have my r=.

Comment 8

[Johnny Stenback (:jst)]

Hmm, from a code point of view, this looks fine, and I really do agree with you
Dan, I can't see any reasons for alowing this other than abusive and annoying ones. 

I'll sr, but I would like to have more input from people on this before we land
it, at least from Brendan, cc:ing.

Comment 9

[Brendan Eich [:brendan]]

I like simple, brute force and ignorance solutions.  I haven't reviewed the
patch closely, but I say "go for it".

I have no idea whether you'll get sued by porn industry lawyers :-).  I once
received unsolicited, unauthenticated email from someone claiming to be a lawyer
who seemed to be litigating against one or more porn-sites'
onunload=>window.open daisy-chaining.  More than that, deponent sayeth not!

/be

Comment 10

[Dan M]

  You're trying to scare me. This patch came to me in a vision which also
featured soaring eagles, a white buffalo, and a half-eaten submarine sandwich
floating on a bed of pretzels. I understand nothing of this, other than the
source is trustworthy and good.
  And it's in. No more hucking of new windows from beyond the grave. The world
is a better place.

Comment 11

[Johnny Stenback (:jst)]

ROFL :-)

Comment 12

[greer]

Haven't seen this on the trunk since the fix. VERIFIED per talkback.

Comment 13

[Jay Patel [:jay]]

Adding M097 to summary for future reference, this *was* a topcrasher in Mozilla
0.9.7.

Comment 14

[Dan M]

Comment on attachment 63327 [details] [diff] [review]
disallow a window being destroyed from opening new windows

this patch has been backed out, replaced by a better patch in bug 130719.