If we want to do more compartmental GC, we need to be clearing the mark bits for compartments that aren't collected. Otherwise, the cycle collector isn't able to collect cycles that touch those compartments. However, there's a tricky problem here. Let's say we have this configuration: Compartment C1: contains object A that points to B Compartment C2: contains object B Now let's say that originally A and B were both gray. Imagine we decide to collect C1, but not C2. It's possible that A could be marked black -- maybe it was found by the conservative stack scanner. In that case, we have an edge from a black object to a gray object. In theory, this isn't supposed to happen. This patch handles that problem by searching for such edges and clearing the mark bits in compartment C2 (which is what we normally do now). It should be very rare for this to happen.

Comment on attachment 613027 [details] [diff] [review] patch What if C2 contains edges to a third compartment C3?

Comment on attachment 613027 [details] [diff] [review] patch Yeah, you're right. That's annoying. I guess I can fix it.

Is this going to just bleed into everything? If every compartment has edges to and from chrome, then you'll just end up clearing all the mark bits.

(In reply to Andrew McCreight [:mccr8] from comment #3) > Is this going to just bleed into everything? If every compartment has edges > to and from chrome, then you'll just end up clearing all the mark bits. Perhaps the best thing to do is if we find anything then just unmark all the uncollected compartments. We expect this to be a rare situation, so I think this is fine.

Ah, so the initial situation of having a (new) black-gray edge that spans compartments should be rare. That makes sense, though I haven't investigated that myself.

Here's a new patch. It would be useful to get info on how common these edges are. Maybe with telemetry or something.

Comment on attachment 613039 [details] [diff] [review] patch v2 Actually, maybe I'll test this some more.

(In reply to Bill McCloskey (:billm) from comment #0) > Now let's say that originally A and B were both gray. Imagine we decide to > collect C1, but not C2. It's possible that A could be marked black -- maybe > it was found by the conservative stack scanner. > > In that case, we have an edge from a black object to a gray object. In > theory, this isn't supposed to happen. This can only happen if after an initial full GC we want to run a series of compartment GC before running the CC, right? If so, why not to mark propagate the black bit recursively if we ever find a gray object when navigating from the compartment roots? The complication with that approach is what to to do with a gray edge into the compartment. Currently AFAIK we do not distinguish such edges treating anything reachable from outside as black.

Fixed a few bugs in the previous version.

Comment on attachment 628138 [details] [diff] [review] patch v3 Review of attachment 628138 [details] [diff] [review]: ----------------------------------------------------------------- Here are my comments that you probably don't want. ::: js/src/jsgc.cpp @@ +3093,5 @@ > + * an edge to an uncollected compartment: it's possible that the source and > + * destination of the cross-compartment edge should be gray, but the source > + * was marked black by the conservative scanner. To avoid the black->gray > + * edge, we completely clear the mark bits of the destination object's > + * compartment. This is safe because the destination compartment isn't being The second-to-last sentence of this comment isn't quite precise any more. The code doesn't clear the mark bits of the destination object's compartment, it clears the mark bits of all non-collected compartments. Maybe it would make sense to move that comment down to the |if (foundBlackGray)) section? Does UnmarkGray actually go through wrappers? I guess I'm not sure exactly how that works. Does an object actually contain a raw pointer to the other compartment, or does it point to the wrapper... somehow...? And in the latter case, the JS_TraceChildren knows how to look for it? @@ +3094,5 @@ > + * destination of the cross-compartment edge should be gray, but the source > + * was marked black by the conservative scanner. To avoid the black->gray > + * edge, we completely clear the mark bits of the destination object's > + * compartment. This is safe because the destination compartment isn't being > + * collected. Without bug 742841, this will miss cross-compartment black-gray edges involving the debugger, right? @@ +3111,3 @@ > } > + > + if (foundBlackGray) { Is this actually rare? In other words, if you browse around a little, do you tend to hit it? @@ +3113,5 @@ > + if (foundBlackGray) { > + for (CompartmentsIter c(rt); !c.done(); c.next()) { > + if (!c->isCollecting()) > + c->arenas.unmarkAll(); > + } So the idea here is that if you see any black-gray edge, you just clear all mark bits, thus replicating our current behavior?

I'll fix the comment. This is intended to be based on top of bug 742841 (note the dependency). UnmarkGray will cross through compartments without a care in the world. A cross-compartment wrapper is just a Proxy object whose private pointer points into the other compartment. So JS_TraceChildren treats it like any other pointer. At one point I added JS_ASSERT(!foundBlackGray) and I browsed around for a few minutes. I never hit it. However, that's not the most rigorous testing in the world.

(In reply to Bill McCloskey (:billm) from comment #11) > I'll fix the comment. This is intended to be based on top of bug 742841 > (note the dependency). Great. I figured you'd realized that, but I thought I'd mention it. And I failed to notice the dependency. > UnmarkGray will cross through compartments without a care in the world. A > cross-compartment wrapper is just a Proxy object whose private pointer > points into the other compartment. So JS_TraceChildren treats it like any > other pointer. Ah, great. I figured it must work, but I had a moment of panic there. > At one point I added JS_ASSERT(!foundBlackGray) and I browsed around for a > few minutes. I never hit it. However, that's not the most rigorous testing > in the world. Sure. It might be interesting to push to try with something that will dump to the log when you trip it, then look at logs. I can do that if you'd like.

I fixed the comment. I also pushed a version of this to tryserver. It asserts if we ever find black/gray cross-compartment edges. https://tbpl.mozilla.org/?tree=Try&rev=810d24073d57