49615 - Crash when clicking on [x] close in Profile dialogs

Status: VERIFIED FIXED

(SeaMonkey :: Startup & Profiles, defect, P2)

Description

[Henrik Gemal]

Start the Profile Manager and click Create Profile. Now click on the [x] in the 
upper right corner to cancel.
CRASH!

Build 20000819

Comment 1

[Henrik Gemal]

This also happens when pressing [x] in the Select Profile dialog...

Comment 2

[Grace Bush]

reproduced here

Comment 3

[Blake Ross]

Adding crash keyword to all open crashers

Comment 4

[johng]

nav triage team:
rare enough, we can live with it

Comment 5

[Andreas Franke (gone)]

Attachment: "where full" from an 8/30 debug build

Comment 6

[Andreas Franke (gone)]

Attachment: complete stack trace (without full)

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

not a FE issue. 

Comment 8

[Andreas Franke (gone)]

Attachment: patch 1: add a null check before dereferencing mContext

Comment 9

[Peter Trudelle]

->danm, adding patch keyword, don't need '+' to take it.

Comment 10

[Andreas Franke (gone)]

So the crash is happening here:
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsGlobalWindow.cpp#265

265  ::JS_ClearWatchPointsForObject((JSContext *) mContext->GetNativeContext(),
266                                 (JSObject *) mScriptObject);
 
because mContext is null. 

This code is part of this checkin by [e-mail]:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=nsGlobalWindow.cpp&filetype=match&who=brendan%25mozilla.org&whotype=match&sortby=Date&hours=2&date=explicit&mindate=08%2F08+20%3A30&maxdate=08%2F08+20%3A30&cvsroot=%2Fcvsroot

Comment 11

[Andreas Franke (gone)]

With patch 1 the crash seems to disappear. Is this The Right Thing to do?

Here is a list of all methods in nsGlobalWindow.cpp where I found mContext.
Null checks for mContext are quite frequent. Besides this bug's crash
in SetNewDocument I guess that there could be at least one other crash:
let mContext already be null and call SetContext with aContext==null.
Or is there any guarantee that this will never happen?


143 void GlobalWindowImpl::CleanUp()
144 {
145   if (mContext)
146     mContext->RemoveReference(&mScriptObject, mScriptObject);
147   mContext = nsnull;            // Forces Release
...


226 NS_IMETHODIMP GlobalWindowImpl::SetContext(nsIScriptContext* aContext)
227 {
228   // if setting the context to null, then we won't get to clean up the
229   // named reference, so do it now
230   if (!aContext) {
231     NS_WARNING("Possibly early removal of script object, see bug #41608");
232     mContext->RemoveReference(&mScriptObject, mScriptObject);
233   }
234 
235   mContext = aContext;
236   return NS_OK;
237 }


239 NS_IMETHODIMP GlobalWindowImpl::GetContext(nsIScriptContext ** aContext)
240 {
241   *aContext = mContext;
242   NS_IF_ADDREF(*aContext);
243   return NS_OK;
244 }


246 NS_IMETHODIMP GlobalWindowImpl::SetNewDocument(nsIDOMDocument* aDocument)
...
265   ::JS_ClearWatchPointsForObject((JSContext *) mContext->GetNativeContext(),
266                                  (JSObject *) mScriptObject);
...
305         if (mContext && mScriptObject) {
306 //      if (mContext && mScriptObject && aDocument) {
307 //      not doing this unless there's a new document prevents a closed
window's
308 //      JS properties from going away (that's good) and causes everything,
309 //      and I mean everything, to be leaked (that's bad)
310           ::JS_ClearScope((JSContext *) mContext->GetNativeContext(),
311                           (JSObject *) mScriptObject);
312         }
...
321   if (mContext) {
322     // Add an extra ref in case we release mContext during GC.
323     nsCOMPtr<nsIScriptContext> kungFuDeathGrip = mContext;
324     kungFuDeathGrip->GC();
325   }
...
329   if (mDocument && mContext)
330     mContext->InitContext(this);
...


335 NS_IMETHODIMP GlobalWindowImpl::SetDocShell(nsIDocShell* aDocShell)
...
344   if (!aDocShell && mContext) {
345     if (mScriptObject) {
...
349       ::JS_SetProperty((JSContext *) mContext->GetNativeContext(),
350                        (JSObject *) mScriptObject, "closed", &val);
351       // hand off our reference to mContext
352       mContext->SetRootedScriptObject(mScriptObject);
353       mContext->RemoveReference(&mScriptObject, mScriptObject);
354     }
355     mContext = nsnull;          //


416 NS_IMETHODIMP GlobalWindowImpl::HandleDOMEvent(...)
...
429   nsCOMPtr<nsIScriptContext> kungFuDeathGrip2(mContext);
...


3557 PRBool GlobalWindowImpl::RunTimeout(nsTimeoutImpl *aTimeout)
....
3568   if (nsnull == mContext) {
3569     return PR_TRUE;
3570   }
....
3576   nsIScriptContext *tempContext = mContext;
....
3580   cx = (JSContext *) mContext->GetNativeContext();
....
3653         rv = mContext->EvaluateString(...)
....
3671         rv = mContext->CallEventHandler(...)
....


3792 void GlobalWindowImpl::DropTimeout(nsTimeoutImpl *aTimeout,
3793                                    nsIScriptContext *aContext)
....
3800   if (!aContext)
3801     aContext = mContext;
....

Comment 12

[Dan M]

  Checked in Andreas' patch, which was quite correct -- it doesn't hurt to check 
mContext before using, there. So, it doesn't crash any more, but there are still 
problems later on in this same function. I think the whole function just wants to 
abort if the window has already been closed and cleaned up (as it has in this 
case.)

--- ./dom/src/base/nsGlobalWindow.cpp.1.329	Wed Aug 30 20:43:13 2000
+++ ./dom/src/base/nsGlobalWindow.cpp	Wed Aug 30 20:43:07 2000
@@ -251,6 +251,10 @@
       doc->GetPrincipal(getter_AddRefs(mDocumentPrincipal));
   }
 
+  NS_ASSERTION(!aDocument || mDocShell, "setting new document in closed 
window");
+  if (!mDocShell)
+    return NS_OK;
+
   // Always clear watchpoints, to deal with two cases:
   // 1.  The first document for this window is loading, and a miscreant has
   //     preset watchpoints on the window object in order to attack the new

makes all assertions and obvious unhappiness stop happening. Wants to be thought 
about a bit and run over for leaks before it's checked in, though.

Comment 13

[Peter Trudelle]

put on rtm radar to triage with owner.

Comment 14

[Dan M]

patch below. awaiting the appropriate collection of blessings.

--- ./dom/src/base/nsGlobalWindow.cpp.1.352	Wed Oct  4 15:42:07 2000
+++ ./dom/src/base/nsGlobalWindow.cpp	Wed Oct  4 15:39:46 2000
@@ -305,6 +305,13 @@
     return NS_OK;
   }
 
+  /* No mDocShell means we've already been partially closed down.
+     When that happens, this method crashes and in fact is
+     just nonsensical. So abort. (Doesn't happen under normal
+     circumstances, but bug 49615 describes a case.) */
+  if (!mDocShell)
+    return NS_OK;
+
   SetStatus(nsString());
   SetDefaultStatus(nsString());
 

Comment 15

[Dan M]

Scratch the above patch. This one is less invasive and still prevents the crash.

--- ./dom/src/base/nsGlobalWindow.cpp.1.352	Wed Oct  4 16:06:27 2000
+++ ./dom/src/base/nsGlobalWindow.cpp	Wed Oct  4 16:05:38 2000
@@ -305,8 +305,14 @@
     return NS_OK;
   }
 
-  SetStatus(nsString());
-  SetDefaultStatus(nsString());
+  /* No mDocShell means we've already been partially closed down.
+     When that happens, setting status isn't a big requirement,
+     so don't. (Doesn't happen under normal circumstances, but
+     bug 49615 describes a case.) */
+  if (mDocShell) {
+    SetStatus(nsString());
+    SetDefaultStatus(nsString());
+  }
 
   if (mDocument) {
     nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument));

Comment 16

[Chris Waterson]

Looks ok to me; let's get jst to look at it as well...

Comment 17

[Johnny Stenback (:jst)]

Looks good to me too, r=jst

Comment 18

[Peter Trudelle]

P2.  Ready to land, assuming Waterson's 'looks okay' is equivalent to "a=waterson".

Comment 19

[Phil Peterson]

PDT marking [rtm++]

Comment 20

[Dan M]

fixed trunk & netscape rtm branch

Comment 21

[Grace Bush]

no crashes on Win build2000100609MN6/ or Linux 2000100610MN6

Comment 22

[Asa Dotzler [:asa]]

Verified Fixed on trunk builds
linux 101708 RedHat 6.2
win32 101704 NT 4

Setting bug to Verified and removing vtrunk keyword