Closed Bug 413447 Opened 16 years ago Closed 16 years ago

nsXBLDocumentInfo can keep closed global window alive

Categories

(Core :: XBL, defect, P1)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: dbaron, Assigned: bent.mozilla)

Details

(Keywords: memory-leak)

Attachments

(2 files, 3 obsolete files)

It seems that nsXBLDocumentInfo objects can keep a closed global window alive.  I'm not sure how long this lasts -- it could just be for one extra cycle of cycle collection, or it could be nearly permanent.

Steps to reproduce:
 1. do a build with DEBUG_CC, but with the nsCycleCollector_DEBUG_shouldBeFreed call in content/base/src/nsGenericElement.cpp commented out
 2. start Firefox (redirecting the output to a file)
 3. open a new window with Ctrl-N
 4. close the new window with the window manager's [X] in the corner
 5. wait for a cycle collection to happen
 6. close Firefox

Actual results:
the cycle collection that happens after the second window was closed but before closing the first window fails to collect an nsGlobalWindow object for a closed window because of:

nsCycleCollector: nsXBLDocumentInfo 0x10952a0 was not collected due to 3
  external references (5 total - 2 known)
  An object expected to be garbage could be reached from it by the path:
    nsXBLDocumentInfo 0x10952a0
    nsGlobalWindow 0x166b0f0
  The 2 known references to it were from:
    JS Object (chrome://browser/content/places/toolbar.xml#places-bar) 0x2aaab79cdc00
    JS Object (chrome://browser/content/places/toolbar.xml#places-bar bad63 0x2aaab79cb9c0

(and because those two known references -- those JS objects -- are in turn reachable from elements)


I looked at nsXBLDocumentInfo's Traverse implementation, and I couldn't even figure out how to get from it to an nsGlobalWindow in a single step.  Figuring out how we're reaching it is probably the first step here...
Flags: blocking1.9?
Version: unspecified → Trunk
I think peter found it:

<peterv> dbaron: could it be http://mxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLPrototypeHandler.cpp#167 ?


sayrer also points out this may be related to bz's comments in bug 372769.
(In reply to comment #0)
>  4. close the new window with the window manager's [X] in the corner

I'm actually not sure whether I did that or Ctrl-W...
So why do cached handlers need to be associated with a global object?  If they really do, we probably need to clear the cached globals and probably the cached handlers when the window is closed.
(This is basically the question of whether (and how) the context passed to JS_CompileUCFunctionForPrincipals matters.)
As I recall, it basically doesn't...

Is there a reason we're not doing handlers the way we do ctors/dtors: compile once against the XBL global, then clone functions for elements as needed?
Oh, and the issues I saw in bug 372769 predate the handler checkin, so there may be more than one thing going on here.  :(
Like to understand this better for b3 if we can.   Suggested owner?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Johnny, do you know the XBL+JS code well enough here?
Assignee: nobody → jst
So I grabbed this from Johnny and have tried a few things with little to no results. I'm also having a hard time catching a nsXBLDocumentInfo in the CC debug spew - I'm seeing the nsXULTemplateQueryProcessorRDF issue far more often (bug 394514).

I wrote some stuff to clear the cached handler/global object held on to by the prototype handlers whenever we closed a window. Initially I only cleared the cached handler/global for the window that was going away but that didn't seem to ever find any matches. Then I expanded the code to clear all cached handlers/globals and i finally did see some getting cleared, but again they never matched the window that was closed.

Talking with Johnny more about this we both think that we can just make the prototype handler hold a nsWeakRef to the global object since it's only ever used for equality comparison. It doesn't need to hold an owning ref.

Any further thoughts?
Assignee: jst → bent.mozilla
Attached patch Use weak refs, v1 (obsolete) — Splinter Review
Attachment #299346 - Flags: review?(jst)
(In reply to comment #9)
> Talking with Johnny more about this we both think that we can just make the
> prototype handler hold a nsWeakRef to the global object since it's only ever
> used for equality comparison. It doesn't need to hold an owning ref.

That was my first thought too. We just need to make sure that it's only ever used for comparison, that we're not relying on the strong ref keeping that global object alive. I *think* it should be fine, someone else should be holding that global object alive if it is still needed.
BTW, if holding a weak ref works we should remove that Traverse method.
I've always worried about the "weak ref for comparison" thing.  What if the object dies, and a new one is allocated in its place?
If you're using nsISupportsWeakReference and nsIWeakReference (and calling QueryReferent each time), the pointer replay issue shouldn't be a problem.  Was that the plan?
Comment on attachment 299346 [details] [diff] [review]
Use weak refs, v1

That seems to be what the patch does.

sr=me with that traverse method killed.
Attachment #299346 - Flags: superreview+
Cycle collector stuff is still a bit unfamiliar for me, does this look correct?
Attachment #299346 - Attachment is obsolete: true
Attachment #299824 - Flags: superreview?(jonas)
Attachment #299824 - Flags: review?(jst)
Attachment #299346 - Flags: review?(jst)
Attachment #299824 - Flags: superreview?(jonas)
Attachment #299824 - Flags: superreview+
Attachment #299824 - Flags: review?(jst)
Attachment #299824 - Flags: review+
Fixed.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
So I'm still seeing this, although the links are a little longer now, for example:

nsCycleCollector: nsXBLDocumentInfo 0xc86220 was not collected due to 6
  external references (11 total - 5 known)
  An object expected to be garbage could be reached from it by the path:
    nsXBLDocumentInfo 0xc86220
    JS Object (Function - onxblblur) (global=2aaab7a0b880) 0x2aaab9149680
    JS Object (ChromeWindow) (global=2aaab7a0b880) 0x2aaab7a0b880
    nsGlobalWindow 0xb26850
  The 5 known references to it were from:
    JS Object (chrome://global/content/bindings/textbox.xml#textbox) (global=2aaab47349c0) 0x2aaaaaac4640
    JS Object (chrome://global/content/bindings/textbox.xml#input-box aaac4 (global=2aaab47349c0) 0x2aaab9173140
    nsBindingManager 0xc1faa0
    JS Object (chrome://global/content/bindings/textbox.xml#textbox c3276a4 (global=2aaac325d900) 0x2aaab981d580
    JS Object (chrome://global/content/bindings/textbox.xml#input-box c3276 (global=2aaac325d900) 0x2aaabb968180

Steps to reproduce:
start firefox
Press Ctrl+L to focus and select location bar
type mozilla in location bar (so autocomplete pops up)
hit down arrow a few times
press Esc to close autocomplete popup
press tab twice to focus content area again
press Ctrl+N to open a new window
close the first window with the [X] in the corner
wait for a cycle collection to happen
quit

I think we really need to move this cache to live off of the window, and we need to traverse it from the window.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch moves the xbl handler cache to nsGlobalWindow and 

With this patch I no longer see any nsXBLDocumentInfo objects in the CC output when using the STR from comment 18.

Unfortunately I think we leak the windows now... Still investigating that, it shouldn't be possible...
Attachment #300960 - Flags: review?(jst)
Attachment #299824 - Attachment description: Use weak refs, v2 → Use weak refs, v2 [checked in]
Comment on attachment 300960 [details] [diff] [review]
Cache XBL handlers on the window, v1

Nevermind, I found the problem.
Attachment #300960 - Flags: review?(jst)
Here we go. QueryInterface doesn't addref when returning nsXPCOMCycleCollectionParticipant but it does when returning nsCycleCollectionISupports. Duh.
Attachment #300960 - Attachment is obsolete: true
Attachment #300961 - Flags: review?(jst)
Target Milestone: --- → mozilla1.9beta4
(In reply to comment #21)
> QueryInterface doesn't addref when returning
> nsXPCOMCycleCollectionParticipant but it does when returning
> nsCycleCollectionISupports. Duh.
Uh, I hope that is documented somewhere.

Now also called in FreeInnerObjects.
Attachment #300961 - Attachment is obsolete: true
Attachment #302007 - Flags: review?(jst)
Attachment #300961 - Flags: review?(jst)
Comment on attachment 302007 [details] [diff] [review]
Cache XBL handlers on the window, v2

Looks good, r=jst.
Attachment #302007 - Flags: review?(jst) → review+
Attachment #302007 - Flags: superreview?(jonas)
Comment on attachment 302007 [details] [diff] [review]
Cache XBL handlers on the window, v2

Sorry for not getting to this earlier. Started to review this so many times I thought i was done, but kept getting sucked into other things.

>+++ mozilla.b61dd74c4555/dom/src/base/nsGlobalWindow.h	2008-02-07

>+  typedef nsDataHashtable<nsVoidPtrHashKey, void*> HandlerCacheType;
>+
>+  nsAutoPtr<HandlerCacheType> mCachedXBLPrototypeHandlers;

Bummer, if there was a way to check if Init() has been called yet you could inline this object and call Init() lazily. Unfortunately there doesn't seem to be :(

sr=me.
Attachment #302007 - Flags: superreview?(jonas) → superreview+
(In reply to comment #25)
> Bummer, if there was a way to check if Init() has been called yet you could
> inline this object and call Init() lazily. Unfortunately there doesn't seem to
> be :(

With raw JS/PLDHashTable you can zero-init and test .ops -- can you do that here?

/be
Yeah, all the nsTHashtable derivatives have IsInitialized functions so I'll just do that.
Fixed with the inline member.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
FYI, this seems to have caused a compile error with "Sun Studio CC" compiler:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Ports/1202839200.1202839735.12471.gz
(In reply to comment #29)
Got it, thanks.

Had to back this out for bogus test failures (example.com blocklisted!), need to reland.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: