1087799 - Handle cycle collector reentrance better

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

For some reason, this has started happening a lot on TBPL, but the stack looks familiar.

What appears to be happening is this:
- We're in the middle of an incremental CC.
- We decide to GC.
-- Thus, we need to finish the incremental CC. Eventually, run Unlink().
--- This runs Promise::cycleCollection::Unlink(), which runs random JS.
---- This triggers a new GC.
----- We're still in the middle of an ICC, so finish the current ICC.
------ We detect reentrance, so we return immediately.
------ The assertion fires because we have not finished the CC.

I think this is sort of okay right now, because we never touch JS in Unlink, but it is kind of getting into a sketchier situation with bug 1052793, because then we store pointers to zones.  Though maybe if we're careful that will be okay... I'm not quite sure of the security implications, so I'm hiding it for now.

Comment 1

[Andrew McCreight [:mccr8]]

One possible safer solution would be to have the CC root |whiteNodes|, and implement some kind of trace hook that scans everything looking for JS.  It would be a little slow, but hopefully this isn't too common.  I'm not really sure how that would work for entire JS zones, but I guess we could root all globals in the zone, because that's all we really look at.

Comment 2

[Boris Zbarsky [:bzbarsky]]

>--- This runs Promise::cycleCollection::Unlink(), which runs random JS.

Note that we're working on stopping that insanity.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Note that we're working on stopping that insanity.

Being able to just ban GC entirely during Unlink() would make Terrence happy. :)

Comment 4

[Andrew McCreight [:mccr8]]

I looked at this, and I'm pretty sure we're not touching GC objects after we might GC.  However, there are some improvements we can make to enforce this more thoroughly.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Loosen the invariant in nsCycleCollector::FinishAnyCurrentCollection().

If an Unlink() method ends up running JS, it can cause a GC, which will make us reenter the CC,
which will not do anything because we're already in a CC. Therefore, FinishAnyCurrentCollection()
won't finish the CC. This is safe because the CC only touches things it actually holds alive via
the Root() method.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Rename count to numWhiteNodes in nsCycleCollector::CollectWhite().

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Do not include any JS things in the list of white nodes.

Root() does not actually root JS things, so if some other class's Unlink() method ends
up calling the GC, whiteNodes will end up containing dead pointers. (This is safe right
now because the Unlink and Unroot methods do not do anything to JS things.) It is less
error prone to simply never store those pointers.

Also, add some asserts to enforce that we never call any of the white-object methods
for JS things.

Comment 8

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=571af010f313

This try run had a stricter version that nulled out mPointers for things in the graph that we didn't add to whiteNodes, but that seems like overkill.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Should probably dupe bug 1062012 forward.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #9)
> Should probably dupe bug 1062012 forward.

Ah, thanks, I knew there was another bug floating around somewhere.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8511296 [details] [diff] [review]
part 1 - Loosen the invariant in nsCycleCollector::FinishAnyCurrentCollection().

A bit odd setup, but should work.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8511298 [details] [diff] [review]
part 3 - Do not include any JS things in the list of white nodes.

Oh, this should speed up unlink a bit.

Comment 15

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #13)
> Oh, this should speed up unlink a bit.
You'd think so, but when I tried this before I couldn't measure any speedup.

Comment 16

[Andrew McCreight [:mccr8]]

Thanks for the fast reviews!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1a2afa35face
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/80ddb4e48db5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf5fe2afc94

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1a2afa35face
https://hg.mozilla.org/mozilla-central/rev/80ddb4e48db5
https://hg.mozilla.org/mozilla-central/rev/1cf5fe2afc94