Created attachment 8691615 [details] [diff] [review] remove_setFoundBlackGrayEdges-v0.diff The comment that sets it in ShouldMarkCrossCompartment claims that this can happen because of conservative scanning. If set, we later memset the mark bitmap. There is, of course, a comment telling us that it's totes safe, but without going into any substantial detail about how or why. We no longer have conservative scanning, so I think we can just assert here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5571c83b80a4
Comment on attachment 8691615 [details] [diff] [review] remove_setFoundBlackGrayEdges-v0.diff Uh oh. Try is not green. It seems the flimsy rationalizations in the comments were not the complete story!
(In reply to Terrence Cole [:terrence] from comment #1) Heh. My guess is that a write barrier can mark the source black. This comment is old and possibly predates incremental marking.
(In reply to Jon Coppeard (:jonco) from comment #2) > (In reply to Terrence Cole [:terrence] from comment #1) > Heh. My guess is that a write barrier can mark the source black. This > comment is old and possibly predates incremental marking. And quite possibly compartmental GC as well! It's not a barrier actually. What's going on is: * GC[n] marks |src| and |target| GREY * GC[n+1] is a compartmental GC containing the compartment of |src|, but not containing the compartment of |target| * GC[n+1] finds the cross compartment edge from |src| to |target| via tracing * |src| is BLACK because it was reset to WHITE at the start of GC[n+1] and subsequently traced * |target| is still |GREY| because it is not part of the GC *** KABOOM *** How we solve this right now is to clobber the mark bits and force the CC to re-do an all-compartments collection before it can run again. This worries me: we don't actually solve the problem so we're likely to get right back into the same situation again. I think a better solution would be to actually enforce the invariants. One way to do this would be to exposeToActiveJS somehow on the extra-gc |target|. The downside here is that we (probably) would get to grey marking and would proceed to re-mark this edge grey if it were in the GC. It would stay black and not be collectable by the CC until another GC refreshed the state. Andrew, do you have thoughts?
Can we just clobber the mark bits on all compartments we aren't collecting? Do we already do that? (I assume we treat compartments we aren't collecting as black for the purposes of holding onto things in the compartments we are collecting?) Then we could run the CC safely without collecting all compartments; we just won't collect anything in the non-collected compartments.
(In reply to Andrew McCreight [:mccr8] from comment #4) > Can we just clobber the mark bits on all compartments we aren't collecting? > Do we already do that? That's effectively what we do already, if we find that this situation has occured: in endSweepPhase() we clear all the mark bits for uncollected compartments if we found and black to gray edges. (In reply to Terrence Cole (PTO from 26 Nov to 7 Dec) from comment #3) > One way to do this would be to exposeToActiveJS somehow on the extra-gc > |target|. That sounds like it would work but would involve doing an unbounded amount of marking non-incrementally outside the confines of the selected zones we are collecting. Instead, could we have a separate fixup operation invoked by the CC that updates the marking state by recursively marking black from all black to gray cross compartment edges?
(In reply to Jon Coppeard (:jonco) from comment #5) > Instead, could we have a separate fixup operation invoked > by the CC that updates the marking state by recursively marking black from > all black to gray cross compartment edges? The reason we currently trigger a full GC is that once there is a black-gray edge anywhere potentially somebody could read that gray pointer out of the black object, then write it into some other black object, creating new black-gray edges elsewhere at unknown locations. Maybe a write barrier could deal with that.
Created attachment 8793005 [details] [diff] [review] expose_black_to_gray_edges_instead_of_resetting_everything-v1.diff This patch wasn't too difficult, although I'll wait on the try run before proclaiming anything specific about its other merits.
Looks like we're missing an "explicit" and one of our assertions is over-zealous. We can't actually assert at the end of the cycle that the black->gray target is still gray as it might have been marked later by something else.
Created attachment 8793037 [details] [diff] [review] expose_black_to_gray_edges_instead_of_resetting_everything-v2.diff Fixing the initial round of errors turned up by the try run.
Comment on attachment 8793037 [details] [diff] [review] expose_black_to_gray_edges_instead_of_resetting_everything-v2.diff Review of attachment 8793037 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8793037 - Flags: review?(jcoppeard) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/5cbb0c32eebf Expose black to gray cross compartment edges instead of resetting the GC; r=jonco
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
status-firefox45: affected → ---
status-firefox50: --- → affected
status-firefox51: --- → affected
You need to log in before you can comment on or make changes to this bug.