33673 - GlobalWindowImpl from nsDocShell

Status: RESOLVED WORKSFORME

(Core :: XUL, defect, P3)

Description

[Bruce Mitchener]

mozilla-bin startup and shutdown. build from this afternoon. Solaris 2.6, gcc 
2.7.2.3, Purify.

         MLK: 196 bytes leaked at 0xb20840
         This memory was allocated from:
               malloc         [rtlib.o]
               __bUiLtIn_nEw  [libxpcom.so]
               __builtin_new  [rtlib.o]
               NS_NewScriptGlobalObject [nsGlobalWindow.cpp:3618]
                     
                     *aResult = nsnull;
                    
               =>    GlobalWindowImpl *global = new GlobalWindowImpl();
                     NS_ENSURE_TRUE(global, NS_ERROR_OUT_OF_MEMORY);
                  
                     return 
CallQueryInterface(NS_STATIC_CAST(nsIScriptGlobalObject*, global),
               nsDocShell::EnsureScriptEnvironment() [nsDocShell.cpp:2484]
                     if(mScriptContext)
                        return NS_OK;
                  
               =>    NS_NewScriptGlobalObject(getter_AddRefs(mScriptGlobal));
                     NS_ENSURE_TRUE(mScriptGlobal, NS_ERROR_FAILURE);
                  
                     mScriptGlobal->SetDocShell(NS_STATIC_CAST(nsIDocShell*, 
this));
               nsDocShell::GetScriptGlobalObject(nsIScriptGlobalObject**) 
[nsDocShell.cpp:1852]
               DocumentViewerImpl::Init(nsIWidget*,nsIDeviceContext*,const 
nsRect&) [nsDocumentViewer.cpp:458]
               nsDocShell::SetupNewViewer(nsIContentViewer*) 
[nsDocShell.cpp:2175]
               nsWebShell::SetupNewViewer(nsIContentViewer*) 
[nsWebShell.cpp:758]
               nsDocShell::CreateContentViewer(const 
char*,int,nsIChannel*,nsIStreamListener**) [nsDocShell.cpp:2052]
               nsWebShell::DoContent(const char*,int,const 
char*,nsIChannel*,nsIStreamListener**,int*) [nsWebShell.cpp:1423]
               nsDocumentOpenInfo::DispatchContent(nsIChannel*,nsISupports*) 
[nsURILoader.cpp:392]
               nsDocumentOpenInfo::OnStartRequest(nsIChannel*,nsISupports*) 
[nsURILoader.cpp:253]
               nsResChannel::OnStartRequest(nsIChannel*,nsISupports*) 
[nsResChannel.cpp:580]
               nsFileChannel::OnStartRequest(nsIChannel*,nsISupports*) 
[nsFileChannel.cpp:443]
               nsOnStartRequestEvent::HandleEvent() 
[nsAsyncStreamListener.cpp:198]
               nsStreamListenerEvent::HandlePLEvent(PLEvent*) 
[nsAsyncStreamListener.cpp:97]
               PL_HandleEvent [plevent.c:563]
               PL_ProcessPendingEvents [plevent.c:508]
               nsEventQueueImpl::ProcessPendingEvents() [nsEventQueue.cpp:314]
               event_processor_callback(void*,int,GdkInputCondition) 
[nsAppShell.cpp:141]
               our_gdk_io_invoke(_GIOChannel*,GIOCondition,void*) 
[nsAppShell.cpp:54]
               g_io_unix_dispatch [giounix.c:131]
               g_main_dispatch [gmain.c:647]
               g_main_iterate [gmain.c:854]
               g_main_run     [gmain.c:912]
               gtk_main       [gtkmain.c:475]
               nsAppShell::Run() [nsAppShell.cpp:303]
               nsAppShellService::Run() [nsAppShellService.cpp:365]
               main1(int,char**,nsISplashScreen*) [nsAppRunner.cpp:757]

Comment 1

[Mike Shaver (:shaver emeritus)]

Looks like this entrains 100k+ of other stuff, unsurprisingly.

I'll take a look.

Comment 2

[Bruce Mitchener]

Look in the URL field for a make-tree output from the refcount balancer.  It
expands to be 1 meg roughly.

Comment 3

[Bruce Mitchener]

Also: http://www.cybersight.com/~bruce/global2.tree.gz .. 10k, expands to 100k,
ignoring the balanced subtrees.

Comment 4

[Bruce Mitchener]

Offhand, it looks to me like a circular ref between the GlobalWindowImpl and the
nsJSContext, similar to the mess in bug #28570.

Comment 5

[Mike Shaver (:shaver emeritus)]

Waterbabe?  Any thoughts?

(I gotta get set up to play mah-jongg again.)

Comment 6

[Mike Shaver (:shaver emeritus)]

So.

Looks from this like nsJSContext might be at fault here, and we leak as many of
those as we leak GlobalWindowImpls.

1       nsXULKeyListenerImpl::LocateAndExecuteKeyBinding()
  1       nsXULKeyListenerImpl::HandleEventUsingKeyset()
  | 1       NS_NewScriptGlobalObject()
  |   1       unsigned int CallQueryInterface<nsIScriptGlobalObject>()
  |     1       GlobalWindowImpl::QueryInterface()
  |       1       GlobalWindowImpl::AddRef()
  1       nsXULKeyListenerImpl::HandleEventUsingKeyset()
  | 1       NS_CreateScriptContext()
  |   1       nsJSContext::InitContext()
  |     1       GlobalWindowImpl::GetScriptObject()
  |       1       NS_NewScriptWindow()                    
  |         1       GlobalWindowImpl::AddRef()
  -1      nsXULKeyListenerImpl::HandleEventUsingKeyset()
    -1      nsCOMPtr<nsIScriptGlobalObject>::~nsCOMPtr()
      -1      GlobalWindowImpl::Release()

...and maybe the XULPrototype stuff is leaking the JSContext?

1       XULContentSinkImpl::CloseContainer()
  2       nsXULPrototypeScript::Compile()
  | 1       nsXULPDGlobalObject::GetContext()
  | | 1       NS_CreateScriptContext()
  | |   1       nsJSEnvironment::GetNewContext()
  | |     1       nsJSContext::AddRef()
  | 1       nsXULPDGlobalObject::GetContext()
  |   1       unsigned int ns_if_addref<nsIScriptContext *>()
  |     1       nsJSContext::AddRef()
  -1      nsXULPrototypeScript::Compile()
    -1      nsCOMPtr<nsIScriptContext>::~nsCOMPtr()
      -1      nsJSContext::Release()

So that's pretty weird: we take two shots at nsXULPDGlobalObject::GetContext
(the first creates, the second returns-and-refs; all is well), but we only
release from the nsCOMPtr destructor once.  From reading
http://lxr.mozilla.org/seamonkey/source/rdf/content/src/nsXULElement.cpp#4469 I
can't see any way for us to call GetContext without it being handled by the
nsCOMPtr, which means that we should be showing two ::Releases(). 
http://lxr.mozilla.org/seamonkey/source/rdf/content/src/nsXULPrototypeDocument.cpp#490
looks fine, too.

What am I missing?

(Adding Hyatt and Brendan, because they love this stuff.  No, really.  Assigning
to Waterson for now, because it's really not Travis' bug.)

Comment 7

[Chris Waterson]

Is this a recurring leak? Or one time only? If it's a one-time leak, is it this?

http://lxr.mozilla.org/seamonkey/source/rdf/content/src/nsXULKeyListener.cpp#365

Comment 8

[Mike Shaver (:shaver emeritus)]

Hrm.  Maybe, yeah.  We leak two nsJSContexts and one GlobalWindowImpl, so
there's still one nsJSContext unaccounted for.

I'll try flipping that #define (bug 27739 is fixed now, yay), and see what
happens.

Thanks, Chris.

Comment 9

[Mike Shaver (:shaver emeritus)]

Well, we still leak a GlobalWindowImpl, but we don't leak any nsJSContexts
(yay!).

I'll poke at the tree some more after my nap.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

It looks to me like the reference not being released is the one created in
GlobalWindowImpl::GetScriptObject in the line:
170       res = NS_NewScriptWindow(aContext, NS_STATIC_CAST(nsIDOMWindow*,this),
171                              nsnull, &mScriptObject);
( http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#170 )

NS_NewScriptWindow addrefs its second argument:
2618     // assign "this" to the js object
2619     ::JS_SetPrivate(jscontext, global, aSupports);
2620     NS_ADDREF(aSupports);
( http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsJSWindow.cpp#2601 )

I think that reference is supposed to be garbage collected... see the more
clearly commented code in
http://lxr.mozilla.org/seamonkey/source/rdf/content/src/nsXULPrototypeDocument.cpp#433

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I compared refcount logs from loading (using the PageCycler) only about:blank
(which doesn't leak a GlobalWindowImpl) and test0 (which does), and followed the
lines where the numbers were different until I found the difference.  The
release that occurred in about:blank and didn't with test0 was within JS garbage
collection:

-1      nsDocShell::Destroy(void)+0x0000030D
  -1      nsCOMPtr<nsIScriptContext>::operator=(nsIScriptContext *)+0x0000001F
    -1      nsCOMPtr<nsIScriptContext>::assign_with_AddRef(nsISupports
*)+0x00000038
      -1     
nsCOMPtr<nsIScriptContext>::assign_assuming_AddRef(nsIScriptContext
*)+0x0000003E
        -1      nsJSContext::Release(void)+0x00000094
          -1      nsJSContext::~nsJSContext(void)+0x0000006F
            -1      JS_DestroyContext+0x0000001B
              -1      js_DestroyContext+0x0000019B
                -1      js_ForceGC+0x0000004D
                  -1      js_GC+0x000007C3
                    -1      js_FinalizeObject+0x000000CE
                      -1      NS_CreateScriptContext+0x00002285
                        1       nsJSUtils::nsGenericFinalize(JSContext *,
JSObject *)+0x00000055
                        | 1       GlobalWindowImpl::QueryInterface(nsID const &,
void **)+0x0000027C
                        |   1       GlobalWindowImpl::AddRef(void)+0x00000072
                        -1      nsJSUtils::nsGenericFinalize(JSContext *,
JSObject *)+0x00000084
                        | -1      GlobalWindowImpl::Release(void)+0x0000006E
                        -1      nsJSUtils::nsGenericFinalize(JSContext *,
JSObject *)+0x000000A7
                          -1      GlobalWindowImpl::Release(void)+0x0000006E

The two relevant logs are (when my computer is on) at:
http://dbaron.student.harvard.edu/u/david/leaks/bloaturls/GlobalWindowImpl-good2.balance
http://dbaron.student.harvard.edu/u/david/leaks/bloaturls/GlobalWindowImpl-bad2.balance

I took a refcount log of the 5 nsJSContexts in the case where there was a leak,
and three were destroyed at the correct place within nsDocShell::Destroy (and
the other two within a release of an nsXULPDGlobalObject).

So does anybody know why the GlobalWindowImpl wouldn't be garbage-collected in
some cases

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Do you think this leak is responsible for the nsXPC* that are leaked?  (Those,
in turn, seem to leak the nsStringBundleService (with a hashtable with 2K of
nsStringKey and nsStr), and maybe the RDFServiceImpl.)  Or should I file a
separate bug?

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

It seems that during the last call to js_GC, there's a call to js_RemoveRoot,
with the following stack trace:

#0  js_RemoveRoot (rt=0x80bbf18, rp=0x83ef960) at jsgc.c:206
#1  0x4017fdf5 in JS_RemoveRootRT (rt=0x80bbf18, rp=0x83ef960) at jsapi.c:1061
#2  0x4064ef9d in nsXPCWrappedNative::Release (this=0x83ef950) at
xpcwrappednative.cpp:78
#3  0x4064fa61 in nsXPCWrappedNative::~nsXPCWrappedNative (this=0x83f9958,
__in_chrg=3) at xpcwrappednative.cpp:366
#4  0x4064ef55 in nsXPCWrappedNative::Release (this=0x83f9958) at
xpcwrappednative.cpp:71
#5  0x4064f031 in nsXPCWrappedNative::JSObjectFinalized (this=0x83f9958,
cx=0x8335ca8, obj=0x84058d8) at xpcwrappednative.cpp:95
#6  0x40656ae0 in WrappedNative_Finalize (cx=0x8335ca8, obj=0x84058d8) at
xpcwrappednativejsops.cpp:690
#7  0x401d0b2a in js_FinalizeObject (cx=0x8335ca8, obj=0x84058d8) at
jsobj.c:1489
#8  0x401adfc7 in js_GC (cx=0x8335ca8) at jsgc.c:1032
#9  0x401ad7a5 in js_ForceGC (cx=0x8335ca8) at jsgc.c:796
#10 0x4018a47f in js_DestroyContext (cx=0x8335ca8, gcmode=JS_FORCE_GC) at
jscntxt.c:195
#11 0x4017f583 in JS_DestroyContext (cx=0x8335ca8) at jsapi.c:770
#12 0x40649cb6 in xpcPerThreadData::Cleanup (this=0x822fd20) at
xpcthreadcontext.cpp:224
#13 0x4064a309 in xpcPerThreadData::CleanupAllThreads () at
xpcthreadcontext.cpp:395
#14 0x4062f971 in nsXPConnect::~nsXPConnect (this=0x81fdf20, __in_chrg=3) at
nsXPConnect.cpp:195
#15 0x4062f490 in nsXPConnect::Release (this=0x81fdf20) at nsXPConnect.cpp:42
#16 0x4062fb35 in nsXPConnect::ReleaseXPConnectSingleton () at
nsXPConnect.cpp:279
#17 0x40647e9d in xpcModuleDtor (self=0x80bbca8) at xpcmodule.cpp:63
#18 0x400f7e85 in nsGenericModule::Shutdown (this=0x80bbca8) at
nsGenericFactory.cpp:143
#19 0x400f7bf0 in nsGenericModule::~nsGenericModule (this=0x80bbca8,
__in_chrg=3) at nsGenericFactory.cpp:122
#20 0x400f7d20 in nsGenericModule::Release (this=0x80bbca8) at
nsGenericFactory.cpp:125
#21 0x40103400 in nsDll::Shutdown (this=0x8060ca8) at xcDll.cpp:443
#22 0x400f9720 in nsFreeLibrary (dll=0x8060ca8, serviceMgr=0x0, when=3) at
nsNativeComponentLoader.cpp:374
#23 0x400f98a4 in nsFreeLibraryEnum (aKey=0x8060cf8, aData=0x8060ca8,
closure=0xbffff5f4) at nsNativeComponentLoader.cpp:426#24 0x400bdfe6 in
_hashEnumerate (he=0x8060d98, i=19, arg=0xbffff5d0) at nsHashtable.cpp:99
#25 0x40235b71 in PL_HashTableEnumerateEntries (ht=0x805db98, f=0x400bdfb0
<_hashEnumerate(PLHashEntry *, int, void *)>, arg=0xbffff5d0) at plhash.c:413
#26 0x400be508 in nsHashtable::Enumerate (this=0x805db78, aEnumFunc=0x400f9858
<nsFreeLibraryEnum(nsHashKey *, void *, void *)>, closure=0xbffff5f4) at
nsHashtable.cpp:237
#27 0x400fad26 in nsNativeComponentLoader::UnloadAll (this=0x805fb20, aWhen=3)
at nsNativeComponentLoader.cpp:943
#28 0x400f65a6 in nsComponentManagerImpl::UnloadLibraries (this=0x805c4e8,
serviceMgr=0x0, aWhen=3) at nsComponentManager.cpp:1908
#29 0x400f243a in nsComponentManagerImpl::Shutdown (this=0x805c4e8) at
nsComponentManager.cpp:302
#30 0x400b761e in NS_ShutdownXPCOM (servMgr=0x0) at nsXPComInit.cpp:622

When I put in (to test my theory) an extra call to JS_GC(cx) at the beginning of
js_DestroyContext, this leak went away.  (But most of the nsXPC* stuff did not.)

Comment 14

[Bruce Mitchener]

shaver suggested handing this over to brendan if it is a JS GC problem.

Comment 15

[Brendan Eich [:brendan]]

I don't think this is a JS GC problem.  An extra GC after the one that ran a 
finalizer that removed a root is likely to collect what that root pointed at 
during the previous GC's mark phase.  But since everyone seems to want me to 
have this bug, I'll take it.  You'll see....

/be

Comment 16

[Andreas Franke (gone)]

This bug is not about a crash, but anyway...
A stack that is similar to David's can be found in bug 40775.
If anyone is going to touch JS GC roots, I suggest to talk with dveditz.

Comment 17

[Brendan Eich [:brendan]]

Similar stacks are meaningless -- a coincidence -- in this case.

/be

Comment 18

[rusty.lynch]

I'm not sure if this is the same bug, but a GlobalWindowImpl is leaked for every
alert dialog.  I have a test case at:

  * http://www.linux86.com/testcases/alert.html

I came across this by looking through one of the boehm gc logs referenced on the
porkjockes mailing list messages. The entire 25M html log can be viewed at:

  *
http://www.linux86.com/boehm_logs/2000-06-16-pda.tucows.com-palm-prod_sched.html-leaks.html
 
or, to just view the root leak for GlobalWindowImpl go to:

  * http://www.linux86.com/boehm_logs/pda.tucows.com/GlobalWindowImpl_204.html

Comment 19

[Warren Harris]

=> Rusty @ Intel

Comment 20

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Is there anything in this bug still to be fixed?

Comment 21

[Michael La Guardia]

Cc'ing Valeski.  Jud could you make the call about whether this is nsbeta3 + or
-?  Thanks.

Comment 22

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I'm going to mark this bug as WORKSFORME, since there was no reponse to my
comment a month ago.

I and others have fixed a number of leaks of GlobalWindowImpl objects related to
leaked JS roots since this bug was filed.  It's not clear which of the bugs this
was, but I don't think we leak any GlobalWindowImpl on a simple
startup/shutdown.  There are none leaked in the bloat stats on tinderbox.

I believe the concerns I raised about removal of roots during JS_DestroyContext
have been fixed by brendan's changes on July 7 to fix bug 44376.

Any new bugs on leaked GlobalWindowImpl objects should be filed separately.