663532 - cycle collector fails to invoke JS GC except at shutdown

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

The cycle collector can't be run until the JS GC is run.  To fix this, nsCycleCollector::BeginCollection ensures that nsXPConnect::Collect has been called at least once before the CC runs.  But it looks like this call doesn't actually do anything.

Bug 625302 adds a hook to invoke just the cycle collector from JS, for the purpose of fuzzing.  It also adds a test to mochitest that tests this feature.  When you run it alone (via python obj-dbg/_tests/testing/mochitest/runtests.py --chrome --test-path=dom/tests/mochitest/chrome/test_cyclecollector.xul ), it causes an abort in nsXPConnect:

###!!! ABORT: Cannot cycle collect if GC has not run first!: file /Users/amccreight/mz/tm2/js/src/xpconnect/src/nsXPConnect.cpp, line 483

I did some experimenting, and it looks like the call to js_GC hits this statement and returns:

> if (JSGCCallback callback = rt->gcCallback) {
>     if (!callback(cx, JSGC_BEGIN) && gckind != GC_LAST_CONTEXT) {
>         return;
>     }
> }

The JS GC never runs, so when the CC gets to nsXPConnect::BeginCycleCollection it aborts.

As this never seems to happen under normal circumstances, my guess is that whatever heuristic the CC uses to decide to run basically guarantees that the JS GC is invoked first by other means.

If we want to guard against this (normally never seen) situation, we could somehow check after we do |rt->Collect()| in the cycle collector to make sure the GC was actually run, and just |return false;| if it fails, thus just skipping the CC without actually killing the browser.  This probably isn't a very good solution from a fuzzing perspective, as the CC isn't actually being called.

Of course, it would be nicer to actually figure out why the CC isn't being invoked.  I think callback is some kind of thing in XPConnect, but I couldn't figure out what was going on there.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The callback is defined in js/src/xpconnect/src/xpcjsruntime.cpp as XPCJSRuntime::GCCallback. For JSGC_BEGIN, it looks like it doesn't GC if we're not on the main thread.

As long as nsCycleCollector::BeginCollection is always run on the main thread, I guess we're safe. Does the test run it off the main thread?

Either way, it wouldn't hurt to assert that CCs are always called from the main thread.

Comment 2

[Andrew McCreight [:mccr8]]

I think the CC is run in a separate thread to avoid trashing the cache.  In fact, if you look at nsCycleCollectorRunner::Run() there's this assertion:

 NS_ASSERTION(NS_IsCycleCollectorThread() && !NS_IsMainThread(),
                     "Wrong thread!");

So I guess it isn't always (ever?) being run in the main thread.

Comment 3

[Andrew McCreight [:mccr8]]

smaug's patch calls nsJSContext::CycleCollectNow, which looks like it always uses nsCycleCollectorRunner, so that explains that problem.  It looks like the nsCycleCollectorRunner::Collect expects to be run from the main thread, but then signals to the separate cycle collector thread to actually do the CC, and it is off in that other thread that the JS GC is actually done.  If the JS GC needs to be run on the main thread, the code that calls the JS GC could probably be lifted into nsCycleCollector::PrepareForCollection so it would be run on the main thread.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: move rt->Collect from BeginCollection to PrepareForCollection

I moved the call to rt->Collect() from BeginCollection() to PrepareForCollection(), which should ensure that it is run on the main thread.  After doing so, the test in Bug 625302 passes.

I'm not sure if I should add a check in BeginCollection to return cleanly if the JS GC hasn't been run.  I don't know if it is a problem in practice.

The other problem is that my patch (which moved a block of code up a bit) made diff barf, so I need to figure out how to massage my settings (my diff settings for hg are currently |git = 1   showfunc = 1    unified = 8|) so it isn't so awful.

I also need to test it on more than just the single test from the other bug...

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: move rt->Collect from BeginCollection to PrepareForCollection

Now with less awful diff!  The block I moved should be unchanged.  I added an assert to the top of PrepareForCollection that checks that we're in the main thread, but didn't add any graceful fallback if the GC doesn't run.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: move rt->Collect from BeginCollection to PrepareForCollection

Forgot to add the patch header thing.

This passed a try run, except the Win Debug build was red.  It then ran and passed all Win Debug tests, so I'm not sure what was going on.  I'm rebuilding Win Debug now, as I'm assuming it was probably just a build system hiccup.

Thanks for the help on this, Bill.  This was pretty easy to fix after your comment.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 538910 [details] [diff] [review]
move rt->Collect from BeginCollection to PrepareForCollection

Review of attachment 538910 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

::: xpcom/base/nsCycleCollector.cpp
@@ +2467,2 @@
>  {
> +    NS_ASSERTION(NS_IsMainThread(), "Must run rt->Collect() from the main thread.");

Nit: I'd make this say "PrepareForCollection must be called on the main thread".

Comment 8

[Andrew McCreight [:mccr8]]

Thanks.  Okay, that makes sense.  I'd still like to document somewhere that rt->Collect() needs to be called from the main thread.  Maybe I could throw in a comment down near rt->Collect()?

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Extra clarifying comments are always triple A+ approved :)

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: move rt->Collect from BeginCollection to PrepareForCollection

Addressed bent's comments, carrying forward his review of the old patch.

The win32 debug build was green on the try server.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/174a1d29c93e

Comment 12

[Andrew McCreight [:mccr8]]

This patch leaks.

Comment 13

[Andrew McCreight [:mccr8]]

Sorry for the bugspam from fiddling with the name.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 538990 [details] [diff] [review]
move rt->Collect from BeginCollection to PrepareForCollection

Requesting approval as this is needed for bug 660778, which fixes a topcrash.

Comment 15

[Andrew McCreight [:mccr8]]

http://hg.mozilla.org/releases/mozilla-beta/rev/7ed5464e4198

Comment 16

[AndreiD[QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

I was trying to see if this is fixed for Fx6 since the flag "status-firefox6" is set on "fixed", but I couldn't.
Is there a test case or any steps / guidelines for this bug that can be used to verify the fix? Thanks

Comment 17

[Andrew McCreight [:mccr8]]

Unfortunately, not really.  Part of the reason this stayed around so long was that it was hard to see.  Bug 625302 provides a way to test it (that was what prompted this fix), but that only landed a few weeks ago, and thus won't be in Firefox 6 or 7.

Comment 18

[(no longer active)]

It was not clear whether this landed before the last Aurora merge or not, so now we don't exactly know whether this exists on Beta or not.  Somebody from the js team should confirm.

Comment 19

[Andrew McCreight [:mccr8]]

Should be, it has a target milestone of 7 set.  I guess it is probably hard to tell looking at the code because the followup Bug 664506 partially undid the changes in this patch.  That landed in m-c after this patch, so it is probably easier to just look for that one.  That patch added the method GCIfNeeded to xpcom/base/nsCycleCollector.cpp which is currently present in Aurora.

Comment 20

[LegNeato]

(In reply to Andrew McCreight [:mccr8] from comment #19)
> Should be, it has a target milestone of 7 set.  I guess it is probably hard
> to tell looking at the code because the followup Bug 664506 partially undid
> the changes in this patch.  That landed in m-c after this patch, so it is
> probably easier to just look for that one.  That patch added the method
> GCIfNeeded to xpcom/base/nsCycleCollector.cpp which is currently present in
> Aurora.

I see GCIfNeeded in mozilla-beta 7, marking as fixed there.

Comment 21

[u279076]

qa-: no QA fix verification needed