The default bug view has changed. See this FAQ.

add Watchpoint support to the cycle collector

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: mccr8)

Tracking

(Blocks: 1 bug, {mlk, testcase})

Trunk
mozilla12
mlk, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 566125 [details]
testcase

Bug 639235 mentioned that watchpoints have something in common with WeakMap. Could this bug be related to the WeakMap leaks?
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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
(Assignee)

Comment 3

6 years ago
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
(Assignee)

Updated

6 years ago
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]

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 4

6 years ago
Created attachment 566368 [details]
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;
(Assignee)

Comment 5

6 years ago
Created attachment 566574 [details] [diff] [review]
tell the cycle collector about watchpoints

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.
(Assignee)

Comment 6

6 years ago
Created attachment 566598 [details] [diff] [review]
tell the cycle collector about 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
(Assignee)

Comment 7

5 years ago
Created attachment 576790 [details] [diff] [review]
tell the cycle collector about watchpoints

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?
(Assignee)

Comment 9

5 years ago
Yeah, sorry, I should refresh this and get it done.  I'll do that this week.
(Assignee)

Comment 10

5 years ago
Created attachment 591850 [details] [diff] [review]
tell the cycle collector about watchpoints

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)
(Assignee)

Comment 11

5 years ago
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+
(Assignee)

Comment 13

5 years ago
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?)
(Assignee)

Comment 15

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.