64270 - [crash]crash on window.close when another window is referenced

Status: VERIFIED FIXED

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

Description

[Simon Lucy]

In testing the security capability code with Beonex 0.6 I found in choosing the 
Exit (without another Window) link that the browser crashed with a write to 0. 

The function called is dienice() and before it calls window.close it assigns 
finisher to window, where finisher is the window created onload (its also 
created as a dependent window).

Comment 1

[Simon Lucy]

Incorrectly started as a security bug, re-assigning as a Dom Level 0.

Comment 2

[Keyser Sose]

Reporter what build id are you using?

Comment 3

[Johnny Stenback (:jst)]

This is a bug in the docshell (or webshell, actually), seems like some webshells
are removed from the child list while we're fireing the unload events and we end
up dereferencing a null pointer. This patch fixes the crash but I think some
more logic might be needed since with this patch it's possible that some
webshells don't get the unload notification depending on how the children are
removed from the list.

Index: docshell/base/nsWebShell.cpp
===================================================================
RCS file: /cvsroot/mozilla/docshell/base/nsWebShell.cpp,v
retrieving revision 1.509
diff -u -r1.509 nsWebShell.cpp
--- nsWebShell.cpp      2000/12/30 19:21:37     1.509
+++ nsWebShell.cpp      2001/01/05 05:58:45
@@ -234,8 +234,11 @@
   PRInt32 i, n = mChildren.Count();
   for (i = 0; i < n; i++) {
     nsIDocShell* shell = (nsIDocShell*) mChildren.ElementAt(i);
-    nsCOMPtr<nsIWebShell> webShell(do_QueryInterface(shell));
-    rv = webShell->FireUnloadEvent();
+
+    if (shell) {
+      nsCOMPtr<nsIWebShell> webShell(do_QueryInterface(shell));
+      rv = webShell->FireUnloadEvent();
+    }
   }

   return rv;

Reassigning to rpotts so that he can decide if this is correct or if more logic
is needed here...

Comment 4

[Judson Valeski]

edge

Comment 5

[Judson Valeski]

edge

Comment 6

[ckritzer (gone)]

Hmmnnn....

Okay, so if I can find a top 100 site (or two) which do this sort of thing,
would y'all consider it !fringe? And if I can't, I'll accept your rating of
"future", and we'll re-examine later.  

Sound fair?

-Kritzer

Comment 7

[Johnny Stenback (:jst)]

c'mon, why not just land this patch? It can do no harm at all!

Comment 8

[rpotts (gone)]

If we are removing DocShells from the children list while it is being walked, we
might end up missing one of the DocShells in the iteration :-(

Presumably, only the "current" webshell is being removed.

Comment 9

[Chak Nanga]

->chak

Rick/Johnny : What's the best way to handle the issue which Rick brings up 
above?

Comment 10

[Johnny Stenback (:jst)]

Something like this could work:

http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.cpp#514

Comment 11

[Prashant Desale]

This bug could be probable candidate for nsbeta1 because
1] Its crash with high visibility to users.
2] The main purpose of window.close() method is getting defeated in this 
particular situation. 

nominating for nsbeta1. Adding keywords mozilla0.9.1, nsbeta1.

Comment 12

[Chak Nanga]

Since i have'nt heard from anyone if there's a better way to handle this issue 
i'm going to submit a patch based on jst's last suggestion.

Comment 13

[Chak Nanga]

Attachment: Patch to fix the crash

Comment 14

[rpotts (gone)]

Hi Chak,
Your patch looks good to me...  (r=rpotts)

The only thing I wonder is whether it's worth trying to fix up the index if 
(shell != mChildren.ElementAt(i)).

Right now, there are several other places in nsDocShell where we just do the 
check to make sure that (shell != null) but we don't try to fix up the index :-(  
So when the index gets skewed "bad things" could happen...

I think that long-term the right solution is to use a different data structure 
to hold the child DocShells...  Clearly the nsVoidArray is not working very well 
:-)  Perhaps having the children linked together (via a linked list) would make 
it easier to deal with situations where links get removed while the list is 
being traversed.

What do you think?
-- rick

Comment 15

[Chak Nanga]

Rick,

How about i just check in the (shell != null) part and not muck with the index - 
like in the rest of the places.

Once we figure out the proper way we can make all the changes at once?

Thanks
Chak

Comment 16

[Chak Nanga]

Attachment: Patch which does not muck with the index in the loop - per Rick's suggestio

Comment 17

[Chak Nanga]

Rick,

If the latest patch is OK could you please sr= too?

Thanks

Comment 18

[rpotts (gone)]

looks good. (sr=rpotts)

-- rick

Comment 19

[Johnny Stenback (:jst)]

r=jst for what it's worth.

Comment 20

[Chak Nanga]

Fix checked in...Thanks jst and rick for your help.

Comment 21

[ckritzer (gone)]

DOM0->desale

Comment 22

[Prashant Desale]

Verified with 2001-06-05-11.