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

VERIFIED FIXED in mozilla1.0

Status

()

Core
DOM: Core & HTML
--
major
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: Simon Lucy, Assigned: Chak Nanga)

Tracking

({crash, dom0})

Trunk
mozilla1.0
x86
Windows 2000
crash, dom0
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

18 years ago
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).
(Reporter)

Comment 1

18 years ago
Incorrectly started as a security bug, re-assigning as a Dom Level 0.
Assignee: mstoltz → jst
Component: Security: General → DOM Level 0

Comment 2

18 years ago
Reporter what build id are you using?
Keywords: crash
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...
Assignee: jst → rpotts
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: dom0

Comment 4

18 years ago
edge
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → WORKSFORME
Target Milestone: --- → Future

Comment 5

18 years ago
edge
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Comment 6

18 years ago
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
c'mon, why not just land this patch? It can do no harm at all!
Target Milestone: Future → ---

Comment 8

17 years ago
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.
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 9

17 years ago
->chak

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

Assignee: rpotts → chak
Status: REOPENED → NEW

Comment 11

17 years ago
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.
Keywords: mozilla0.9.1, nsbeta1
(Assignee)

Comment 12

17 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 13

17 years ago
Created attachment 34086 [details] [diff] [review]
Patch to fix the crash

Comment 14

17 years ago
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
(Assignee)

Comment 15

17 years ago
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
(Assignee)

Comment 16

17 years ago
Created attachment 34152 [details] [diff] [review]
Patch which does not muck with the index in the loop - per Rick's suggestio
(Assignee)

Comment 17

17 years ago
Rick,

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

Thanks

Comment 18

17 years ago
looks good. (sr=rpotts)

-- rick
r=jst for what it's worth.
(Assignee)

Comment 20

17 years ago
Fix checked in...Thanks jst and rick for your help.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago17 years ago
Resolution: --- → FIXED

Comment 21

17 years ago
DOM0->desale
QA Contact: ckritzer → desale

Comment 22

17 years ago
Verified with 2001-06-05-11.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.