Last Comment Bug 625302 - Add JS API to trigger cycle collection (without garbage collection)
: Add JS API to trigger cycle collection (without garbage collection)
Status: VERIFIED FIXED
[sg:want]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: ---
Assigned To: Olli Pettay [:smaug] (TPAC)
:
Mentors:
Depends on: 663532
Blocks: 677358
  Show dependency treegraph
 
Reported: 2011-01-12 23:46 PST by Jesse Ruderman
Modified: 2011-08-22 01:13 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
patch (4.22 KB, patch)
2011-05-12 06:55 PDT, Olli Pettay [:smaug] (TPAC)
no flags Details | Diff | Splinter Review
patch (5.38 KB, patch)
2011-06-15 07:21 PDT, Olli Pettay [:smaug] (TPAC)
jst: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-01-12 23:46:21 PST
The patch in bug 624549 made it possible for the cycle collector to run alone, but there is no way for tests (e.g. fuzzers) to make this happen explicitly.

Please add an API for triggering cycle collection alone. (Preferably on Components, not on nsDOMWindowUtils, so I can trigger cycle collection without having a window reference.)
Comment 1 Jesse Ruderman 2011-01-12 23:47:50 PST
Is it likely that CC not immediately preceded by GC will cause problems (e.g. crashes) due to violating CC assumptions)? If so, it's important to be able to test.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2011-01-24 15:56:05 PST
Not going to hold the release for this, but I'd approve a patch for this any time.
Comment 3 Olli Pettay [:smaug] (TPAC) 2011-05-12 06:27:55 PDT
Patch coming.
Comment 4 Olli Pettay [:smaug] (TPAC) 2011-05-12 06:54:07 PDT
Um, actually no.
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/nsXPConnect.cpp#476 has appeared at some point.
Comment 5 Olli Pettay [:smaug] (TPAC) 2011-05-12 06:55:12 PDT
Created attachment 531922 [details] [diff] [review]
patch

This is anyway the patch, but cann't be used reliably because of that
nsXPConnect thing.
Comment 6 Olli Pettay [:smaug] (TPAC) 2011-05-12 07:09:59 PDT
Ben, why exactly is the NS_RUNTIMEABORT needed there?
Comment 7 Andrew McCreight [:mccr8] 2011-05-12 07:52:34 PDT
The CC looks at mark bits that the JS GC produces, so it has to be run after the GC has been run at least once.  That said, the CC checks at the start of its run (before nsXPConnect::BeginCycleCollection) and runs the GC if it hasn't been run yet, so I'm not sure what is happening.
Comment 8 Andrew McCreight [:mccr8] 2011-05-12 08:22:08 PDT
Is nsXPConnect::BeginCycleCollection being called from nsCycleCollector::BeginCollection?  Could the way that the CC invokes the GC not increment the count that nsXPConnect::BeginCycleCollection looks at?
Comment 9 Peter Van der Beken [:peterv] 2011-05-12 08:31:56 PDT
(In reply to comment #7)
> That said, the CC checks at the start of
> its run (before nsXPConnect::BeginCycleCollection) and runs the GC if it
> hasn't been run yet, so I'm not sure what is happening.

AFAIK that's not true, where is that happening?
Comment 10 Andrew McCreight [:mccr8] 2011-05-12 08:46:12 PDT
In nsCycleCollector::BeginCollection, which invokes nsXPConnect::Collect(), which invokes JS_GC.

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp#2565
Comment 11 Peter Van der Beken [:peterv] 2011-05-12 11:08:24 PDT
But that's only for "synchronous" cycle collections, which shouldn't be happening?
Comment 12 Peter Van der Beken [:peterv] 2011-05-12 11:10:35 PDT
(In reply to comment #11)
> But that's only for "synchronous" cycle collections, which shouldn't be
> happening?

The call to nsCycleCollector::BeginCollection should be coming from http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp#3322 which passes false for aForceGC.
Comment 13 Andrew McCreight [:mccr8] 2011-05-12 11:21:28 PDT
Right, but if the GC hasn't been run at all, shouldn't mFirstCollection be true, and thus aForceGC will be set to true?
Comment 14 Peter Van der Beken [:peterv] 2011-05-12 13:31:52 PDT
Ah, yes. Smaug, are you actually hitting the abort?
Comment 15 Olli Pettay [:smaug] (TPAC) 2011-05-12 13:37:48 PDT
Yes, I did hit the abort when I was running the chrome test.
Comment 16 Andrew McCreight [:mccr8] 2011-05-12 13:51:18 PDT
I also hit it when I tried it out.  I haven't quite worked out debugging a Mochitest yet so I'm not sure exactly what is happening. ;)
Comment 17 Andrew McCreight [:mccr8] 2011-05-13 17:21:34 PDT
I did some printf debugging.  BeginCollection behaves the way I expected, and gets into the block that calls Collect().  I'll try looking into whether the collector is actually called or not inside Collect.
Comment 18 Andrew McCreight [:mccr8] 2011-05-25 14:59:00 PDT
The JS GC is not actually triggering.  The cycle collector's invocation of the JS GC gets as far as js_GC in jsgc.cpp, but then it returns from this block of code:

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

gckind is a constant GC_NORMAL that is not equal to GC_LAST_CONTEXT, so the callback must be returning false.  Maybe the callback is DOMGCCallback?  I don't know enough about the JS GC to figure out how this works, or why the callback decided not to run the GC.  I looked at it a little bit, but I couldn't really make much sense of it.
Comment 19 Andrew McCreight [:mccr8] 2011-06-13 13:44:53 PDT
My patch in Bug 663532 should fix this, or at least, when applied test_cyclecollector.xul passes.  So when that lands, you should be good to go.
Comment 20 Olli Pettay [:smaug] (TPAC) 2011-06-15 07:17:18 PDT
Testing if this works now...
Comment 21 Olli Pettay [:smaug] (TPAC) 2011-06-15 07:21:32 PDT
Created attachment 539506 [details] [diff] [review]
patch

Seems to work
Comment 22 Andrew McCreight [:mccr8] 2011-06-15 09:17:24 PDT
As an aside, test_cyclecollector.xul tests the problem in Bug 663532 when run alone, but only if it is run by itself.  If it is in the middle of a bunch of other tests, then the JS GC will probably have run by other means, and the manual JS GC trigger won't be tested.
Comment 23 Olli Pettay [:smaug] (TPAC) 2011-06-28 04:05:41 PDT
review ping.
Comment 24 Olli Pettay [:smaug] (TPAC) 2011-07-26 10:25:20 PDT
http://hg.mozilla.org/mozilla-central/rev/121904a67387
Comment 25 Mihaela Velimiroviciu (:mihaelav) 2011-08-19 05:47:36 PDT
Can anyone please provide some guidelines to verify this fix? Is it necessary to create any specific environment to test it?

Thanks!
Comment 26 Olli Pettay [:smaug] (TPAC) 2011-08-19 05:59:07 PDT
The patch has a test for this, and the test is being run all the time,
so would say that is enough verify that this is fixed.

Though, one could argue that there should be still some way to test that
GC doesn't run when CC is called.
Comment 27 Mihaela Velimiroviciu (:mihaelav) 2011-08-22 01:13:46 PDT
Considering comment26 setting resolution to VERIFIED-FIXED

Note You need to log in before you can comment on or make changes to this bug.