Closed Bug 693527 Opened 13 years ago Closed 12 years ago

add Watchpoint support to the cycle collector

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: jruderman, Assigned: mccr8)

References

Details

(Keywords: memory-leak, testcase, Whiteboard: [MemShrink:P2])

Attachments

(3 files, 3 obsolete files)

Attached file testcase
Bug 639235 mentioned that watchpoints have something in common with WeakMap. Could this bug be related to the WeakMap leaks?
It is probably something similar.  I figured they probably leaked in the same way, but hadn't seen any reports.  I was hoping this would be fixed by Bug 653248, which changes watchpoints so they are marked gray when reachable only from gray roots, but unfortunately it does not.  Maybe we need to tell the cycle collector explicitly about these edges.
Hmm.  The window and document are being kept alive by an nsNodeInfoManager.  That is kind of odd.

0x11850e620 [nsNodeInfoManager]
    --[mDocument]-> 0x1185c7000 [nsDocument (xhtml) https://bug693527.bugzilla.mozilla.org/attachment.cgi?id=566125]
    --[mChildren[i]]-> 0x119ea75e0 [nsGenericElement (xhtml) html]
    --[mAttrsAndChildren[i]]-> 0x1188a5500 [nsGenericElement (xhtml) p]
    --[mSlots->mChildrenList]-> 0x1188a5980 [nsBaseContentList]
    --[Expando object]-> 0x1180653a0 [JS Object (DOM proxy binding expando object) (global=118059060)]
    --[parent, x]-> 0x1180568c8 [JS Object (HTMLParagraphElement) (global=118059060)]
    --[type_proto, XPCWrappedNativeProto::mJSProtoObject]-> 0x1180566b8 [JS Object (XPC_WN_ModsAllowed_NoCall_Proto_JSClass - HTMLParagraphEleme (global=118059060)]
    --[type_proto]-> 0x118056768 [JS Object (XPC_WN_ModsAllowed_NoCall_Proto_JSClass - HTMLElement) (global=118059060)]
    --[type_proto]-> 0x118056818 [JS Object (XPC_WN_ModsAllowed_NoCall_Proto_JSClass - Element) (global=118059060)]
    --[parent, XPCWrappedNativeScope::mGlobalJSObject]-> 0x118059060 [JS Object (Window) (global=118059060)]
    --[xpc_GetJSPrivate(obj)]-> 0x11867ce80 [XPCWrappedNative (Window)]
    --[mIdentity]-> 0x11a1dd878 [nsGlobalWindow]
    --[mOuterWindow]-> 0x12151bc78 [nsGlobalWindow]

0x11850e620 [nsNodeInfoManager]
    --[mDocument]-> 0x1185c7000 [nsDocument (xhtml) https://bug693527.bugzilla.mozilla.org/attachment.cgi?id=566125]

    Root 0x11850e620 is a ref counted object with 1 unknown edge(s).
    known edges:
       0x129844b48 [nsNodeInfo (xhtml) head] --[mOwnerManager]-> 0x11850e620
       0x129845098 [nsNodeInfo (xhtml) p] --[mOwnerManager]-> 0x11850e620
       0x129844ac0 [nsNodeInfo (xhtml) html] --[mOwnerManager]-> 0x11850e620
       0x129844ce0 [nsNodeInfo (xhtml) body] --[mOwnerManager]-> 0x11850e620
       0x129844a38 [nsNodeInfo ([none]) #document-type] --[mOwnerManager]-> 0x11850e620
       0x129844c58 [nsNodeInfo ([none]) #text] --[mOwnerManager]-> 0x11850e620
       0x1298449b0 [nsNodeInfo ([none]) #document] --[mOwnerManager]-> 0x11850e620
       0x129844bd0 [nsNodeInfo (xhtml) script] --[mOwnerManager]-> 0x11850e620
When you remove the "fuzzerisms" the code looks like this:

    var p = document.createElement("p");
    document.documentElement.appendChild(p);
    p.children.x = p;
    var f = function() { };
    p.watch("y", f);
    f.z = document.createElement("div");

Removing any of these lines eliminates the leak.  It looks like that non-GCMarkers are never told about watchpoints, so the cycle collector won't see the edge from p to f, so I think it won't see the div element in f.z.  Johnny said that the new div element that goes in f.z will have a strong reference to the nsNodeInfoManager, which would explain the unknown edge causing the leak.  IE there is a cycle from the document to p to f to f.x to the node info manager to the document, and the CC doesn't see the edge from p to f.

To fix this, we have to tell the cycle collector about watchpoint edges.  I think this can be fixed by treating them as fake weak map edges, where the "map" is marked black.  Should be straightforward...
Assignee: general → continuation
Depends on: 668855
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: Leak nsDocument and nsGlobalWindow with Object.prototype.watch → add Watchpoint support to the cycle collector
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
Attached file test case 2
Here's a variation of Jesse's test case where the cycle does not involve nsNodeInfoManager (which I think may be sensitive to some specifics of whether the info manager reference is strong or weak).  The cycle is p -> f -> d -> p.

    var p = document.createElement("p");
    p.children.x = p;
    var f = function() { };
    p.watch("y", f);
    var d = document.createElement("div");
    d.appendChild(p);
    f.loop = d;
Fairly straightforward.  I confirmed that this patch fixes the leak in test case 2.  I should still add a test for this.

The code mimics the GC marking code for watchpoints.  It is kind of a gross fix because it requires that the cycle collector iterate over compartments, even if there are no watchpoints.
Now with test!  I couldn't use Jesse's original test, because there the watchpoint stays alive as long as the document itself.  I confirmed that this current test leaks without my patch, and doesn't leak with the test.  Trying to observe the callback directly made the weak go away, which is weird, but whatever.

This patch is on top of my stack of patches for weak map cycle collector integration.
Attachment #566574 - Attachment is obsolete: true
Updated the patch to deal with write barriers.  Now that WeakMap-CC integration has landed, I'll hopefully get around to testing this in the next week or so.
Attachment #566598 - Attachment is obsolete: true
mccr8, sounds like this is close to being ready for review?
Yeah, sorry, I should refresh this and get it done.  I'll do that this week.
I confirmed that the attached test case fails without the patch (and leaks a lot...), and passes with it.  This patch refresh is just some minor unbitrotting.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ffb1f7901bd5
Attachment #576790 - Attachment is obsolete: true
Attachment #591850 - Flags: review?(jorendorff)
try looks reasonable.
Comment on attachment 591850 [details] [diff] [review]
tell the cycle collector about watchpoints

OK.

Someday we'll change the implementation of watchpoints to use something like
WeakMap<JSObject *, ObjectWatchpoints> where class ObjectWatchpoints contains a
HashMap<jsid, Watchpoint>.
Attachment #591850 - Flags: review?(jorendorff) → review+
Thanks.

(In reply to Jason Orendorff [:jorendorff] from comment #12)
> Comment on attachment 591850 [details] [diff] [review]
> Someday we'll change the implementation of watchpoints to use something like
> WeakMap<JSObject *, ObjectWatchpoints> where class ObjectWatchpoints
> contains a HashMap<jsid, Watchpoint>.

That would be nice.  I also have a long-term plan to move JS marking by the CC more into the JS engine, so we can keep the CC in the dark about things like that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9801e9475d3b

Thanks for the reminder, njn, I just lost track of this.
Target Milestone: --- → mozilla12
mccr8, can you briefly summarize what this patch did?  Did watchpoints cause some leaks?  (What are watchpoints?)
Here's some documentation for watchpoints: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/watch  "These two methods are implemented only in Gecko, and they're intended primarily for debugging use. In addition, using watchpoints has a serious negative impact on performance, which is especially true when used on global objects, such as window."

In general, cycles between C++ and JS are never broken unless the cycle collector knows about them.  Watchpoints are a form of weak reference, so they require special handling in the GC, and nobody ever told the CC about them. :(  But with the weak map + CC patch I landed before, we can view a watchpoint as a degenerate form of weak map with a single binding, where the map is alive.  As jorendorff said, watchpoints could be reimplemented using WeakMaps.

So yeah, if you make a watchpoint watch itself, or something like that, via a DOM node, then the GC would just keep it alive forever.  I don't know how common they are.  Jesse fuzzed this up.
https://hg.mozilla.org/mozilla-central/rev/9801e9475d3b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: