34130 - Security Manager crashes when opened.

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P3)

Comment 1

[Doug Turner (:dougt)]

Clicking on the lock icon, or selecting Tasks -> Personal Manager -> Security 
Manager will crash todays mozilla build. 

I get an assert here:

NTDLL! 77f7629c()
gc_root_marker(JSHashEntry * 0x035d1660, int 71, void * 0x00e81fe0) line 663 + 
35 bytes
JS_HashTableEnumerateEntries(JSHashTable * 0x010045f0, int (JSHashEntry *, int, 
void *)* 0x002cf746 gc_root_marker(JSHashEntry *, int, void *), void * 
0x00e81fe0) line 363 + 15 bytes
js_GC(JSContext * 0x033bc3e0) line 787 + 21 bytes
js_ForceGC(JSContext * 0x033bc3e0) line 678 + 9 bytes
JS_GC(JSContext * 0x033bc3e0) line 1161 + 9 bytes
nsJSContext::GC(nsJSContext * const 0x032daba0) line 913 + 13 bytes
GlobalWindowImpl::SetNewDocument(GlobalWindowImpl * const 0x032d9410, 
nsIDOMDocument * 0x00000000) line 258
DocumentViewerImpl::~DocumentViewerImpl() line 363
DocumentViewerImpl::`scalar deleting destructor'(unsigned int 1) + 15 bytes
DocumentViewerImpl::Release(DocumentViewerImpl * const 0x02981bf0) line 303 + 
154 bytes
nsCOMPtr<nsIContentViewer>::assign_assuming_AddRef(nsIContentViewer * 
0x00000000) line 447
nsCOMPtr<nsIContentViewer>::assign_with_AddRef(nsISupports * 0x00000000) line 
818
nsCOMPtr<nsIContentViewer>::operator=(nsIContentViewer * 0x00000000) line 557
nsDocShell::SetupNewViewer(nsDocShell * const 0x0337e2a0, nsIContentViewer * 
0x0382a070) line 2221
nsWebShell::SetupNewViewer(nsWebShell * const 0x0337e2a0, nsIContentViewer * 
0x0382a070) line 749 + 13 bytes
nsDocShell::CreateContentViewer(nsDocShell * const 0x0337e2a0, const char * 
0x0012fb48, int 0, nsIChannel * 0x03936370, nsIStreamListener * * 0x0012fb88) 
line 2109 + 24 bytes
nsWebShell::DoContent(nsWebShell * const 0x0337e3b0, const char * 0x0012fb48, 
int 0, const char * 0x10086dc8 gCommonEmptyBuffer, nsIChannel * 0x03936370, 
nsIStreamListener * * 0x0012fb88, int * 0x0012fb2c) line 1413 + 41 bytes
nsDocumentOpenInfo::DispatchContent(nsIChannel * 0x03936370, nsISupports * 
0x00000000) line 399 + 109 bytes
nsDocumentOpenInfo::OnStartRequest(nsDocumentOpenInfo * const 0x039379c0, 
nsIChannel * 0x03936370, nsISupports * 0x00000000) line 253 + 16 bytes
nsHTTPServerListener::FinishedResponseHeaders() line 801 + 48 bytes
nsHTTPServerListener::OnDataAvailable(nsHTTPServerListener * const 0x03986040, 
nsIChannel * 0x032afa64, nsISupports * 0x03936370, nsIInputStream * 0x03985f9c, 
unsigned int 0, unsigned int 862) line 309 + 8 bytes
nsOnDataAvailableEvent::HandleEvent(nsOnDataAvailableEvent * const 0x03987ee0) 
line 382 + 47 bytes
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x03987e90) line 97 + 12 bytes
PL_HandleEvent(PLEvent * 0x03987e90) line 563 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x01000c90) line 508 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x95b608cc, unsigned int 49292, unsigned int 0, 
long 16780432) line 1018 + 9 bytes
USER32! 77e71820()

then it will abnormally terminate.

Comment 2

[John Bandhauer]

Attachment: give this patch a try.

Comment 3

[Doug Turner (:dougt)]

Your fix works for the AlertCheck() dialogs, but this I still am crashing when I 
open up the Security Advisor.

Comment 4

[Doug Turner (:dougt)]

I am dying on:
JS_ASSERT(root_points_to_gcArenaPool);

david, chris, do you know what this could be?

Comment 5

[John Bandhauer]

How to I reproduce this? I installed psm from iPlanet using Nav 4.72 and it runs 
fine there. When I click on the lock icon in mozilla it just hangs in socket io.

Comment 6

[Doug Turner (:dougt)]

sure.

the easy way is to

1. with mozilla go here:
http://docs.iplanet.com/docs/manuals/psm/psm-mozilla/index.html

2. scroll to the bottom of the page, and click the 'Install' button for your 
platform.

Now, try it.

Comment 7

[John Bandhauer]

Oh, I see. The problem I was having seems to be that I can't run Nav 4.x and 
Mozilla at the same time while using psm.

So, I see that the pointer in questionhas a value of 0x00000020.

Either there is a root still with an address in an object that is now defunct or 
someone is putting a bogus pointer in a place where they have told Js there will 
be a gcthing.

I can dig some.

Comment 8

[Chris Waterson]

After about four bajillion assertions (XXX not threadsafe!) I was able to get to 
optionslink just fine.

Comment 9

[Doug Turner (:dougt)]

waterson, click on the lock icon.

Comment 10

[Chris Waterson]

hmm. i crash trying to leave a secure page. never mind.

Comment 11

[John Bandhauer]

The root's name is "nsXBLBinding::mScriptObject" so it comes from:
http://lxr.mozilla.org/seamonkey/source/layout/xbl/src/nsXBLBinding.cpp#1058

Comment 12

[Doug Turner (:dougt)]

Could the crash when leaving could be cause by the warning dialog that appeared 
and you dismissed??


Here is more information:

I am opening the window with these parameter:

menubar=no,height=%d,width=%d

note the menubar attribute.  I do not crash anymore when I replace it with 
something like:

chrome,dialog=no,all

I do crash, but mouse click do not respond correctly (ie,.they go to different 
windows.)

Comment 13

[David Hyatt]

If a XUL element weren't having its doc properly set to nsnull before it gets 
destroyed, then both its refs and the XBL binding refs would not be removed.  
Maybe I'm somehow not clearing out all binding refs, but the place to check in 
nsXBLBinding would be RemoveScriptReferences.

Comment 14

[Daniel (Leaf) Nunes]

I can verify that this still happens with 1.259 of nsGlobalWindow.cpp from
jband, at least on linux.

Comment 15

[Doug Turner (:dougt)]

hyatt, RemoveScriptReferences is not even hit.  The exit happen while opening 
the new window.

Comment 16

[Doug Turner (:dougt)]

Hyatt, over to you.  

I think that you are using the mScriptObject multiple times (in different 
jscontexts?) without addreffing.  

Comment 17

[Patrick C. Beard]

I'm seeing this on the Mac, from a different code path. I'm going to look into it 
tonight.

Comment 18

[Patrick C. Beard]

nsXBLBinding objects are getting released without ever removing their roots. That 
is clear. I added the following code, to remove roots in the destructor if 
necessary:

Index: mozilla/layout/xbl/src/nsXBLBinding.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/xbl/src/nsXBLBinding.cpp,v
retrieving revision 1.27
diff -c -2 -r1.27 nsXBLBinding.cpp
*** nsXBLBinding.cpp	2000/03/31 10:27:12	1.27
--- nsXBLBinding.cpp	2000/04/01 06:38:00
***************
*** 48,51 ****
--- 48,52 ----
  #include "nsINameSpace.h"
  #include "nsJSUtils.h"
+ #include "nsIJSRuntimeService.h"
  
  // Event listeners
***************
*** 333,338 ****
--- 334,361 ----
  }
  
+ /**
+  * Removes a root inadvertently left over.
+  * This is really an error condition, but I've written
+  * this to prevent the JS GC from asserting.
+  */
+ static nsresult removeRoot(void* aSlot)
+ {
+ 	const char kJSRuntimeServiceProgID[] = "nsJSRuntimeService";
+ 	nsresult rv;
+ 	NS_WITH_SERVICE(nsIJSRuntimeService, runtimeService, 
kJSRuntimeServiceProgID, &rv);
+ 	if (NS_FAILED(rv)) return rv;
+ 	JSRuntime* rt;
+ 	rv = runtimeService->GetRuntime(&rt);
+ 	if (NS_FAILED(rv)) return rv;
+ 	return (JS_RemoveRootRT(rt, aSlot) ? NS_OK : NS_ERROR_FAILURE);
+ }
+ 
  nsXBLBinding::~nsXBLBinding(void)
  {
+   if (mScriptObject) {
+     removeRoot(&mScriptObject);
+     mScriptObject = nsnull;
+   }
+ 
    delete mAttributeTable;
  
***************
*** 958,961 ****
--- 981,985 ----
    if (mScriptObject) {
      aContext->RemoveReference((void*) &mScriptObject, mScriptObject);
+     mScriptObject = nsnull;
    }
  

Comment 19

[Doug Turner (:dougt)]

that is it: it fixes this bug.  can you check this in?  

Comment 20

[David Hyatt]

Hang on.  I think I know the real problem...

Comment 21

[David Hyatt]

beard, this is good error code to have.  You should add an assertion in the 
destructor that will assert if we get in there and mScriptObject is non-null.

I know why this is happening down one particular path (this path), but we should 
leave the assert in so that we'll be able to discover and patch the other 
bad paths that will no doubt crop up.

Comment 22

[Bernd Mielke]

Attachment: A new assert before the gc root marker

Comment 23

[Bernd Mielke]

To provoke this assert before the gc_root_marker. I opened mozilla on Win98 
build from  the tree (6:00 PST) and when the dialup dialog went up I closed the 
dialog without connecting to the net. Please note also how the webshells went 
up.

Comment 24

[Doug Turner (:dougt)]

david,  I do not want this broken for monday which we have a patch which seams 
to fix this problem.  What is your status.  What is wrong with the patch that 
beard attached?

Comment 25

[David Hyatt]

Nothing is wrong.  I said in the above comment "beard, this is good error code 
to have."  Check it in.  The only change I made was to add an assertion in the 
destructor so we can see a stack trace of the final release that frees the 
binding.

Comment 26

[David Hyatt]

Excellent.  Thanks for the trace, mielke.  It's asserting in the exact place I 
expected it to.  I have a fix that should obviate the need for beard's checkin, 
although I think beard's checkin should still go in to catch any other paths 
that cause the problem in the future (if there are any).

I'll just handle checking both beard's and my code in later today.  Will be 
fixed by Monday.

Comment 27

[Doug Turner (:dougt)]

I checked in beard's patch:

Checking in nsXBLBinding.cpp;
/cvsroot/mozilla/layout/xbl/src/nsXBLBinding.cpp,v  <--  nsXBLBinding.cpp
new revision: 1.29; previous revision: 1.28
done

Marking as fixed.  Hyatt, if there are other problems, please open up new bugs 
as now my security dialogs work.  

Comment 28

[John Unruh]

Verified fixed.

Comment 29

[John Unruh]

Mass changing Security:Crypto to PSM

Comment 30

[John Unruh]

Mass changing Security:Crypto to PSM