173308 - Browser crashes when javascript closes a window [@ nsDocShell::InternalLoad]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesse Houwing]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20020924
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20020924

Open the above url. Next click the 

"CLICK HERE TO CLOSE THIS WINDOW 
and contact our claims department immediately"

and see the browser crash. No talkback comes up.

In other browsers ti asks if it can close the last window, and after that it
opens a new window. This could be the problem, but I'm not using a debugbuild.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Ian Neal]

WFM on BuildID 2002100711 on Win2KSP2.
Doesn't crash for me, just closes the window with that page in. So if you have
more than one window open mozilla stays open, if only have the one window open
it closes mozilla.
The relevant line of code is 
<a onClick="javascript:self.close();" target="_blank" href="adv617/adv.htm">
which other browsers might process in a different order.

Comment 2

[Jesse Houwing]

I have tabs open, might this be the cause?

Comment 3

[Jesse Houwing]

Attachment: Talkback report (zipped)

Ok, installed 2002100808 and tried again. It still crashes. and now Talkback
does fire. I attached the report as a zipfile (was too large otherwise).

Comment 4

[greer]

Jesse, If you look in the components dir under your Mozilla installation for the
0924 build I expect that you will not see talkback.exe. That is why Talkback
didn't fire. Can you list the Talkback ID# for your most recent crash (10/08 build)?

Comment 5

[Jesse Houwing]

Well it was there, and still didn't fire.

even went back to the 20020924 build to test again, and talkback will not come
up (it does come up with the newer build).

Talkback id: TB12264495Y

Comment 6

[greer]

Product ID  MozillaTrunk
Build ID 2002100808
Trigger Time 2002-10-08 12:07:51
Platform Win32
Operating System Windows NT 5.0 build 2195
Module docshell.dll
URL visited
User Comments
Trigger Reason Access violation
Source File Name c:/builds/seamonkey/mozilla/docshell/base/nsDocShell.cpp
Trigger Line No. 4887
Stack Trace
over to docshell.

Incident #12264495
------------------
nsDocShell::InternalLoad
[c:/builds/seamonkey/mozilla/docshell/base/nsDocShell.cpp, line 4887]
nsWebShell::OnLinkClickSync
[c:/builds/seamonkey/mozilla/docshell/base/nsWebShell.cpp, line 659]
OnLinkClickEvent::HandleEvent
[c:/builds/seamonkey/mozilla/docshell/base/nsWebShell.cpp, line 477]
PL_HandleEvent [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 645]
PL_ProcessPendingEvents [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c,
line 578]
nsEventQueueImpl::ProcessPendingEvents
[c:/builds/seamonkey/mozilla/xpcom/threads/nsEventQueue.cpp, line 392] 

Comment 7

[Olivier Cahagne]

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

Comment 8

[Samir Gehani]

adt: nsbeta1-

Comment 9

[Andrew Schultz]

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

Comment 10

[Andrew Schultz]

reassign

Comment 11

[zack-weg]

Note the good testcase from bug 178268: http://www.flurdy.dsl.pipex.com/test.html

Both dupes have better summaries than this bug. It's very hard to find with its
current summary.

Can anybody reproduce without user_pref("browser.block.target_new_window", true) ?
I think you need it to reproduce.

Proposed fix: Add null pointer check for |mCurrentURI| in nsDocShell.cpp, line 5012
( http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#5012 ).

          if (mDisallowPopupWindows) {
              PRBool bIsChromeOrResource = PR_FALSE;
+             if (mCurrentURI)
+                 mCurrentURI->SchemeIs("chrome", &bIsChromeOrResource);
-             mCurrentURI->SchemeIs("chrome", &bIsChromeOrResource);
              if (!bIsChromeOrResource) {
                  aURI->SchemeIs("chrome", &bIsChromeOrResource);

I haven't tried this fix, but it seems to be obvious with Andrew's analysis in
bug 205506. |aURI| is non-null in the testcase and has already a null pointer
check in line 4919.

Comment 12

[Andrew Schultz]

Analysis from bug 205506:
stacktrace in attachment 123567 [details]

(gdb) frame 6
5012		    mCurrentURI->SchemeIs("chrome", &bIsChromeOrResource);
(gdb) p mCurrentURI
$1 = {mRawPtr = 0x0}


zack-weg: your patch fixes the crash for me and seems reasonable.  can you
generate a cvs diff and attach it here as a patch?

Comment 13

[Andrew Schultz]

Attachment: zack-weg's patch

Comment 14

[Adam Lock]

Comment on attachment 125602 [details] [diff] [review]
zack-weg's patch

r=adamlock

Comment 15

[Alec Flett]

Comment on attachment 125602 [details] [diff] [review]
zack-weg's patch

sr=alecf

Comment 16

[Andrew Schultz]

Adam: can you check this in?

Comment 17

[Adam Lock]

I will as soon as the tree opens

Comment 18

[Adam Lock]

Fix is checked in, many thanks for the patch.