549806 - disentangle cycle collector and JS GC

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andreas Gal :gal]

CC and GC engage in a intricate dance with subtle JSGCCallback dependencies in order to avoid establishing a refcnt for JSObjects. Instead, we unroot globals and see what is alive without them.

(Rest of the description copied from bug 549793, which was the result of a discussion with dbaron).

This bug touches all the relevant parts of the context global clearing.

https://bugzilla.mozilla.org/show_bug.cgi?id=458099

I think one of the mistakes is that we clear the globals prior to the JS GC
run. I assume this is an optimization attempt in order to avoid rescanning the
JS GC heap again, however, we have to do that anyway to collect the cycle
collector edges. Another potential benefit is that we immediately dispose of JS
objects participating in a cycle the cycle collector drops. However, we pay a
couple hefty fines for this:

1. CC becomes non-interruptible and non-abortable. We have to sweep the XPCOM
heap to discover JSObjects that are held live through XPCOM and then mark them
since we bypassed this marking by clearing out the context globals.
2. (1) muddles the boundary between GC and CC and results in a "GC marking
done, but not really" state where the JS GC thinks its done marking, but the CC
will revive additional objects.

I would like to undo this optimization attempt and run a regular JS GC and then
CC only on the XPCOM side. We still have to explore the JS heap to a degree to
identify cycles, but the only cycle resolution we engage in is removing roots
of cycles at the XPCOM->JSHeap boundary. Once that root is dropped the next JS
GC will clear the cycle on the JS side, unwinding the cycle on the XPCOM side.
This has a number of benefits:

1. Simple, clearly defined GC protocol. Marking, Sweep, Done. No intermediate
and mark after IsAboutToBeFinalized calls.
2. CC becomes interruptible and abortable, i.e. if the user moves the mouse or
presses a key or we get a network update, we just stop CCing and do it later.
With this we can CC more often, but essentially with 0 latency (minus battery
life issues, but that's for stuart to worry about).
3. The JS engine can start sweeping its own heap once its done marking,
concurrently to cycle collection since cycle collection will no longer revive
dead objects (or try to touch dead objects). We spend more time sweeping than
marking, so we might be able to hide the time of CC in the sweep time.

A bit of archeology shows that way back we decided to intertwine GC and CC to
avoid a redundant scan/mark. That's basically exactly what I propose we try
now, since we can make it concurrent and interruptible. We will need some
experimenting with this to see whether comment #1 is faster, or the decision
back in 2007 was right after all. Doesn't look super hard to hack that up
though (at least to he point where we can benchmark it), since its mostly
undoing changes.

"So if I understand correctly XPCOM objects would use the API from bug 379220
to
keep JS objects alive. Instead of running the JS GC and use gcThingCallback,
we'd call JS_TraceRuntime to get refcounts for JS objects and we'd use tracing
to reconstruct the graph (like in attachment 261948 [details] [diff] [review] [details]). If that's
correct, then
we would duplicate a bunch of work between the JS GC (which we'll still need to
run) and the refcounting trace?"

https://bugzilla.mozilla.org/show_bug.cgi?id=377884#c9

Comment 1

[Andreas Gal :gal]

To decide whether this makes sense at all we need to know how expensive it is to determine the refcnt for all JSObjects. That sparked the avalanche of patches that resulted in the current quantum entanglement. I will hack up a patch and measure that.

Comment 2

[Igor Bukanov]

(In reply to comment #0)
> I think one of the mistakes is that we clear the globals prior to the JS GC
> run. I assume this is an optimization attempt in order to avoid rescanning the
> JS GC heap again, however, we have to do that anyway to collect the cycle
> collector edges. 

This is not an optimization. The globals stored in JSContext::globalObject are not roots. For example, if there is no JS code running on any thread, there should be no JS roots. Yet JS_GC() will mark the globals for API compatibility. Hence that hack to skip then aside during the CC and only GC-mark them if CC finds them alive.

Comment 3

[Igor Bukanov]

(In reply to comment #0)
> I would like to undo this optimization attempt and run a regular JS GC and then
> CC only on the XPCOM side.

Again, if CC runs only when there is no thread that executes the JS, then the would be no need to do any integration work with JS GC besides allowing to navigate the GC heap so the CC can detect edges between XPCOM objects that goes into JS heap. The trouble starts when the is JS code somewhere. Then the JS roots on the stack should prevent any cycle that goes via them to be collected. 

Still we can avoid the JS_GC() if the cycle collector would simply learn about the current JS roots and treat them specially. But this roots can be trivially discovered via calls js_TraceRuntime().

Comment 4

[Andreas Gal :gal]

Could you elaborate on the compatibility requirement there?

Comment 5

[Andreas Gal :gal]

Attachment: patch

The attached patch determines the precise refcnt for every JSObject that we come across. refcnt 0 and 1 are held in the bits only, > 1 is held in an 4mb array that is used as a hash table with accepted collisions. Collisions cause the refcnt to be overestimated, which causes the JSObject to be considered always live by the cycle collector (since mInternalRefcnt < mRefcnt). Collisions should be relatively rare. The 4mb array is large enough to represent all objects in our default 32mb heap with few collisions (32mb heap are sufficient for 1mil objects).

The overhead for getting the precise refcnts is 9% for the test case in the bug over a GC without refcnt calculation, plus we need an extra 4mb memory (temporarily). The CC itself also uses a ton of memory, so the latter might not be a big deal. Remains to be seen.

Based on sharking gmail and chrome experiments, T_CC + T_GC == 2 * T_GC, so if we can get the refcnts in 1.1 * T_GC and then run the cycle collector in parallel to the GC sweeping, we might come out ahead.

Waiting for Igor to elaborate on the API compatibility issue. API compatibility. Never far away when something is slow.

Comment 6

[Igor Bukanov]

(In reply to comment #4)
> Could you elaborate on the compatibility requirement there?

THe JS engine promises that JSContext::globalObject is a root. But it should not be one for the CC purposes if JSContext::globalObject is referenced from a possible dead cycle participant.

Comment 7

[Igor Bukanov]

(In reply to comment #5)
> Created an attachment (id=429984) [details]
> patch
> 
> The attached patch determines the precise refcnt for every JSObject that we
> come across.

The CC does not need to know the reference count for JS objects! It needs to know only the number of edges that goes into a possible cycle participant from JS objects that are reachable from other cycle participants and not reachable from JS roots. As such there is no need to do any reference counting of JS objects, it is sufficient just to learn the JS objects that are reachable from the cycle participants and then enumerate them to account the edges into the XPCOM world. Such data structure can be efficiently implemented using one flag bit per object and a word per JSGCArena to link all GC arenas that contains objects referenced from the XPCOM world.

> Based on sharking gmail and chrome experiments, T_CC + T_GC == 2 * T_GC, so if
> we can get the refcnts in 1.1 * T_GC and then run the cycle collector in
> parallel to the GC sweeping, we might come out ahead.

If the CC runs with some thread running JS script, it must learn about any potential cycle participant XPOM object that is reachable from JS roots. Without using JS_GC the code would just duplicates the efforts. Now the price for that is an extra marking that has to be done after the cycle collector has detected live XPCOM objects and the following JS finalization that increases the CC pause. If this extra marking + finalization contributes significantly to the CC time, then we can consider avoiding it like abandoning that stage. So could you check the timing split between the initial JS_GC invocation, the cycle collection and the final GC marking and finalization?

Comment 8

[Andreas Gal :gal]

What do you mean with extra marking? The terms are flying around a bit here. Lets try to define what we mean:

mark = mark phase of the JS GC
sweep = sweep phase of the JS GC
walk = walking the JS heap graph using the JSTracer API

Lets avoid the term "participant" since its used to name something else in the implementation. Could you rephrase or clarify this sentence (if you need more terms please define them):

"Without using JS_GC the code would just duplicates the efforts. Now the price
for that is an extra marking that has to be done after the cycle collector has
detected live XPCOM objects and the following JS finalization that increases
the CC pause."

The proposed process is to do a JS_GC mark phase, collect all the information the CC needs, and then proceed to sweep in parallel to CC. At this point marking is over. No more marking. By nobody. Not the JS engine. Not the CC. Sweeping has started. To late to mark. Everything is already marked. All the CC can do at this point is break/unlink roots, which then will unravel during the next JS_GC run. Cycles involving JS Objects are not immediately collected. Since cycles are long-lived, this mild delay until the cycle gets collected should be immaterial.

I will do some measurements vs distribution between mark and sweep. Last time I checked, sweep time still dominates.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Waiting for Igor to elaborate on the API compatibility issue. API
> compatibility.

API, SchmAPI. See JSOPTION_UNROOTED_GLOBAL, which is part of the API. It's used:

https://mxr.mozilla.org/mozilla-central/search?string=JSOPTION_UNROOTED_GLOBAL&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

> Never far away when something is slow.

True often enough, still a cheap shot and logically untenable. When tracing misfires due to a bug in jstracer.cpp, something can be very slow, but API compatibility is far away.

Please think about API like you would any working relationship with a large number of people. Not a promise forever but a contingent set of arrangements that should not change all the time, but that can be improved by adding better APIs, then removing worse ones -- remember the branch callback.

I see Igor's point: it is not necessary to compute all JS objects' refcounts. That is a sure loser if cycles involve relatively few JS objects.

/be

Comment 10

[Igor Bukanov]

(In reply to comment #8)
> What do you mean with extra marking? The terms are flying around a bit here.
> Lets try to define what we mean:
> 
> mark = mark phase of the JS GC
> sweep = sweep phase of the JS GC
> walk = walking the JS heap graph using the JSTracer API
> 
> Lets avoid the term "participant" since its used to name something else in the
> implementation. Could you rephrase or clarify this sentence (if you need more
> terms please define them):
> 
> "Without using JS_GC the code would just duplicates the efforts. Now the price
> for that is an extra marking that has to be done after the cycle collector has
> detected live XPCOM objects and the following JS finalization that increases
> the CC pause."

The CC does not need to consider all XPCOM objects. It can consider only objects for which the reference count decreased from 2 or more. Only those object can form a dead cycle. These are the objects that I called "possible cycle participant". But the right term here is "gray objects".

Currently the CC collection involves (not necessary in that order):

1. Initial GC marking to discover live JS objects that may reference XPCOM things.
2. CC walking from gray objects to discover all inner edges.
3. Color as unreachable the gray objects that are not reachable from live JS and XPCOM objects.
4. GC marking to mark JS things that are reachable from gray objects found live.
5. GC sweep

In my previous comment I wanted to know how much time 4 and 5 takes since I though that phase 1 is inevitable. But now i realized that the phase 1 currently also marks strings and doubles. If we replace that by just marking the objects (that is, the only things that can refer into XPCOM), that may cut down the time of the phase 1 significantly. But that suggests to replace 1 with special-purpose flagging of objects (we borrow one of the currently wasted bits in JSGCArena) and then eliminate 4 and 5.

Comment 11

[Andreas Gal :gal]

> I see Igor's point: it is not necessary to compute all JS objects' refcounts.
> That is a sure loser if cycles involve relatively few JS objects.

Absolutely no intentions on actually doing that in production, but it was used to justify a design decision and I wanted to put a price tag on it.

Comment 12

[Andreas Gal :gal]

> In my previous comment I wanted to know how much time 4 and 5 takes since I
> though that phase 1 is inevitable. But now i realized that the phase 1
> currently also marks strings and doubles. If we replace that by just marking
> the objects (that is, the only things that can refer into XPCOM), that may cut
> down the time of the phase 1 significantly. But that suggests to replace 1 with
> special-purpose flagging of objects (we borrow one of the currently wasted bits
> in JSGCArena) and then eliminate 4 and 5.

Yeah, that's exactly what I was thinking about. markBitmap has 3 flags left per JSObject. We could let a customer marker twiddle those. I think I would prefer sticking the entire mechanics into js_GC() with a simple API (MarkGray, IsGray or something like that, instead of calling back into the CC for every object we walk by and calling back into JS to set the mark bit).

Comment 13

[Andreas Gal :gal]

Ok, Igor and I discussed this a bit further in irc. The additional marking can occur during the regular GC run, using the extra markBitmap bit. We start setting the other color in addition to the regular mark bit when we trace the rt->gcExtraRootsTraceOp part of the JS heap. The CC can start with the same roots and then follow this extra color to discover the relevant parts of the graph.

Comment 14

[Andreas Gal :gal]

Attachment: mark objects added through rt->gcExtraRootsTraceOp as gray (in addition to the standard mark color, black).

Not meant to be reviewed, just a sketch.

Comment 15

[Igor Bukanov]

(In reply to comment #12)
> Yeah, that's exactly what I was thinking about. markBitmap has 3 flags left per
> JSObject. We could let a customer marker twiddle those. I think I would prefer
> sticking the entire mechanics into js_GC() with a simple API (MarkGray, IsGray
> or something like that, instead of calling back into the CC for every object we
> walk by and calling back into JS to set the mark bit).

Is the idea to replace the following code in js_CallGCMarker :

    if (!IS_GC_MARKING_TRACER(trc)) {
        trc->callback(trc, thing, kind);
        goto out;
    }

with something like:

    if (!IS_GC_MARKING_TRACER(trc)) {
        if (CC_MARKING_TRACER(trc)) {
            if (kind != JSTRACE_OBJECT && kind != JSTRACE_XML)
                goto out;
            MarkAsCCLive();
        } else if (CC_WALKING_TRACER(trc)) {
            if (kind != JSTRACE_OBJECT && kind != JSTRACE_XML)
                goto out;
            if (IsCCLive())
                goto out;
            MarkAsCCGrayReachable();
         } else {
            trc->callback(trc, thing, kind);
         }
         goto out;
    }

?

Comment 16

[Igor Bukanov]

(In reply to comment #14)
> Created an attachment (id=430139) [details]
> mark objects added through rt->gcExtraRootsTraceOp as gray (in addition to the
> standard mark color, black).

Currently if the normal GC marking starts, it has to be followed by a sweep since this will invalidate the mark bits left from the previous GC. Those bits are used to decide whether to run the finalizer during the sweep phase. So if we are going to skip the sweep for the CC, we must either restore the bits after the CC or use different bits.

Comment 17

[Igor Bukanov]

(In reply to comment #16)
> Currently if the normal GC marking starts, it has to be followed by a sweep
> since this will invalidate the mark bits left from the previous GC.

Hm, this is only true for doubles. For objects this is not true since the finalizer uses the free list membership to check if the GC cell is free or contains to be finalized object.

Comment 18

[Andreas Gal :gal]

Attachment: sketch for discussion, incomplete

Comment 19

[Andreas Gal :gal]

Attachment: patch

Comment 20

[Andreas Gal :gal]

Attachment: process delayed markings before switching colors

Comment 21

[Andreas Gal :gal]

Attachment: patch

added an assert

Comment 22

[Andreas Gal :gal]

MarkRoots() tries to walk dead objects. That can't be good.

#0  0x00007fff86adefe6 in __kill ()
#1  0x0000000100014720 in RealBreak () at ../../../xpcom/base/nsDebugImpl.cpp:415
#2  0x0000000100014733 in Break (aMsg=0x7fff5fbf5900 "###!!! ASSERTION: attempting to traverse dead JS Object: 'JS_IsAboutToBeFinalized(cx, p)', file ../../../../../js/src/xpconnect/src/nsXPConnect.cpp, line 698") at ../../../xpcom/base/nsDebugImpl.cpp:515
#3  0x0000000100014cb3 in NS_DebugBreak_P (aSeverity=1, aStr=0x1014fa450 "attempting to traverse dead JS Object", aExpr=0x1014fa430 "JS_IsAboutToBeFinalized(cx, p)", aFile=0x1014f9978 "../../../../../js/src/xpconnect/src/nsXPConnect.cpp", aLine=698) at ../../../xpcom/base/nsDebugImpl.cpp:360
#4  0x000000010004c75e in nsXPConnect::Traverse (this=0x10433cb30, p=0x11d97c300, cb=@0x7fff5fbf5fc0) at ../../../../../js/src/xpconnect/src/nsXPConnect.cpp:698
#5  0x0000000101248623 in GCGraphBuilder::Traverse (this=0x7fff5fbf5fc0, aPtrInfo=0x115d33af0) at ../../../xpcom/base/nsCycleCollector.cpp:1476
#6  0x00000001012486be in nsCycleCollector::MarkRoots (this=0x104896000, builder=@0x7fff5fbf5fc0) at ../../../xpcom/base/nsCycleCollector.cpp:1698
#7  0x0000000101248bac in nsCycleCollector::BeginCollection (this=0x104896000) at ../../../xpcom/base/nsCycleCollector.cpp:2645
#8  0x0000000101248c20 in nsCycleCollector_beginCollection () at ../../../xpcom/base/nsCycleCollector.cpp:3235
#9  0x000000010004d3cc in XPCCycleCollectGCCallback (cx=0x10508f000, status=JSGC_MARK_END) at ../../../../../js/src/xpconnect/src/nsXPConnect.cpp:388
#10 0x0000000103900261 in js_GC (cx=0x10508f000, gckind=GC_NORMAL) at ../../../js/src/jsgc.cpp:3184
#11 0x000000010389e1e7 in JS_GC (cx=0x10508f000) at ../../../js/src/jsapi.cpp:2294
#12 0x000000010004d2f1 in nsXPConnect::Collect (this=0x10433cb30) at ../../../../../js/src/xpconnect/src/nsXPConnect.cpp:461
#13 0x0000000101248d61 in nsCycleCollector::Collect (this=0x104896000, aTryCollections=5) at ../../../xpcom/base/nsCycleCollector.cpp:2522
#14 0x0000000101248efb in nsCycleCollector::Shutdown (this=0x104896000) at ../../../xpcom/base/nsCycleCollector.cpp:2754
#15 0x0000000101248f2b in nsCycleCollector_shutdown () at ../../../xpcom/base/nsCycleCollector.cpp:3257
#16 0x00000001011cea8e in mozilla::ShutdownXPCOM (servMgr=0x0) at ../../../xpcom/build/nsXPComInit.cpp:824
#17 0x00000001011ced57 in NS_ShutdownXPCOM_P (servMgr=0x10410b7c8) at ../../../xpcom/build/nsXPComInit.cpp:728
#18 0x0000000100020514 in ScopedXPCOMStartup::~ScopedXPCOMStartup (this=0x7fff5fbfe660) at ../../../toolkit/xre/nsAppRunner.cpp:1055
#19 0x0000000100027157 in XRE_main (argc=1, argv=0x7fff5fbfea30, aAppData=0x104300080) at ../../../toolkit/xre/nsAppRunner.cpp:3533
#20 0x00000001000011fd in main (argc=1, argv=0x7fff5fbfea30) at ../../../browser/app/nsBrowserApp.cpp:158

Comment 23

[Andreas Gal :gal]

Attachment: patch

Patch partially debugging with igor. No longer crashes, but still leaks.

Comment 24

[Andreas Gal :gal]

We leak a bunch of stuff at shutdown, but no Window object. This smells like an existing but to me that the new interaction with the GC exposes. We no longer automatically drop JSObjects as roots. Instead, we rely on the wrapper being Unlink()-ed. If that's missing, the cycle remains intact.

Leaked URLs:
  file:///Users/gal/workspace/tracemonkey-repository/debug-build/dist/MinefieldDebug.app/Contents/MacOS/chrome/browser.jar
  file:///Users/gal/workspace/tracemonkey-repository/debug-build/dist/MinefieldDebug.app/Contents/MacOS/chrome/toolkit.jar
  http://screwdriver/
  http://screwdriver/
  http://test.bclary.com/
  http://test.bclary.com/
  https://bug494186.bugzilla.mozilla.org/
  https://bug494186.bugzilla.mozilla.org/
  resource://gre-resources/ua.css
  resource://gre-resources/html.css
  chrome://global/content/bindings/videocontrols.xml#videoControls
  chrome://xbl-marquee/content/xbl-marquee.xml#marquee-horizontal
  chrome://xbl-marquee/content/xbl-marquee.xml#marquee-vertical
  chrome://global/content/xul.css
  chrome://global/content/bindings/general.xml#root-element
  chrome://global/content/bindings/button.xml#button
  chrome://global/content/bindings/button.xml#button-periodic-redraw
  chrome://global/content/bindings/button.xml#button-repeat
  chrome://global/content/bindings/button.xml#menu
  chrome://global/content/bindings/button.xml#menu-button
  chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton
  chrome://global/content/bindings/toolbarbutton.xml#menu
  chrome://global/content/bindings/toolbarbutton.xml#menu-button
  chrome://global/content/bindings/browser.xml#browser
  chrome://global/content/bindings/editor.xml#editor
  chrome://global/content/bindings/general.xml#iframe
  chrome://global/content/bindings/notification.xml#notificationbox
  chrome://global/content/bindings/notification.xml#notification
  chrome://global/content/bindings/general.xml#image
  chrome://global/content/bindings/checkbox.xml#checkbox
  chrome://global/content/bindings/radio.xml#radiogroup
  chrome://global/content/bindings/radio.xml#radio
  chrome://global/content/bindings/groupbox.xml#groupbox
  chrome://global/content/bindings/groupbox.xml#caption
  chrome://global/content/bindings/toolbar.xml#toolbox
  chrome://global/content/bindings/toolbar.xml#toolbar
  chrome://global/content/bindings/toolbar.xml#toolbardecoration
  chrome://global/content/bindings/toolbar.xml#toolbardecoration
  chrome://global/content/bindings/toolbar.xml#toolbardecoration
  chrome://global/content/bindings/toolbar.xml#toolbarpaletteitem
  chrome://global/content/bindings/toolbar.xml#toolbarpaletteitem-palette
  chrome://global/content/bindings/toolbar.xml#menubar
  chrome://global/content/bindings/menu.xml#menu-menubar
  chrome://global/content/bindings/menu.xml#menu-menubar-iconic
  chrome://global/content/bindings/menu.xml#menu
  chrome://global/content/bindings/menu.xml#menu-iconic
  chrome://global/content/bindings/menu.xml#menuitem
  chrome://global/content/bindings/menu.xml#menuitem-iconic
  chrome://global/content/bindings/menu.xml#menuitem-iconic-desc-noaccel
  chrome://global/content/bindings/menu.xml#menuitem-iconic
  chrome://global/content/bindings/menu.xml#menubutton-item
  chrome://global/content/bindings/menu.xml#menuseparator
  chrome://global/content/bindings/popup.xml#popup
  chrome://global/content/bindings/popup.xml#panel
  chrome://global/content/bindings/popup.xml#tooltip
  chrome://global/content/bindings/listbox.xml#listbox
  chrome://global/content/bindings/listbox.xml#listhead
  chrome://global/content/bindings/listbox.xml#listrows
  chrome://global/content/bindings/listbox.xml#listitem
  chrome://global/content/bindings/listbox.xml#listitem-checkbox
  chrome://global/content/bindings/listbox.xml#listheader
  chrome://global/content/bindings/listbox.xml#listcell
  chrome://global/content/bindings/listbox.xml#listcell-checkbox
  chrome://global/content/bindings/listbox.xml#listitem-iconic
  chrome://global/content/bindings/listbox.xml#listitem-checkbox-iconic
  chrome://global/content/bindings/listbox.xml#listcell-iconic
  chrome://global/content/bindings/listbox.xml#listcell-checkbox-iconic
  chrome://global/content/bindings/tree.xml#tree
  chrome://global/content/bindings/tree.xml#treecols
  chrome://global/content/bindings/tree.xml#treecol
  chrome://global/content/bindings/tree.xml#treecol-image
  chrome://global/content/bindings/tree.xml#treebody
  chrome://global/content/bindings/tree.xml#treerows
  chrome://global/content/bindings/tree.xml#columnpicker
  chrome://global/content/bindings/general.xml#deck
  chrome://global/content/bindings/tabbox.xml#tabbox
  chrome://global/content/bindings/tabbox.xml#tabs
  chrome://global/content/bindings/tabbox.xml#tab
  chrome://global/content/bindings/tabbox.xml#tabpanels
  chrome://global/content/bindings/progressmeter.xml#progressmeter
  chrome://global/content/bindings/text.xml#text-base
  chrome://global/content/bindings/text.xml#text-label
  chrome://global/content/bindings/text.xml#text-link
  chrome://global/content/bindings/text.xml#label-control
  chrome://global/content/bindings/textbox.xml#textbox
  chrome://global/content/bindings/textbox.xml#textarea
  chrome://global/content/bindings/textbox.xml#input-box
  chrome://global/content/bindings/textbox.xml#input-box-spell
  chrome://global/content/bindings/textbox.xml#timed-textbox
  chrome://global/content/bindings/textbox.xml#search-textbox
  chrome://global/content/bindings/numberbox.xml#numberbox
  chrome://global/content/bindings/autocomplete.xml#autocomplete
  chrome://global/content/bindings/autocomplete.xml#autocomplete-result-popup
  chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup
  chrome://global/content/bindings/autocomplete.xml#autocomplete-tree
  chrome://global/content/bindings/autocomplete.xml#autocomplete-treebody
  chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistbox
  chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistitem
  chrome://global/content/bindings/autocomplete.xml#autocomplete-treerows
  chrome://global/content/bindings/autocomplete.xml#history-dropmarker
  chrome://global/content/bindings/colorpicker.xml#colorpicker
  chrome://global/content/bindings/colorpicker.xml#colorpicker-button
  chrome://global/content/bindings/colorpicker.xml#colorpickertile
  chrome://global/content/bindings/menulist.xml#menulist
  chrome://global/content/bindings/menulist.xml#menulist-editable
  chrome://global/content/bindings/menulist.xml#menulist-description
  chrome://global/content/bindings/menu.xml#menuitem-iconic-noaccel
  chrome://global/content/bindings/general.xml#dropmarker
  chrome://global/content/bindings/resizer.xml#resizer
  chrome://global/content/bindings/splitter.xml#splitter
  chrome://global/content/bindings/splitter.xml#grippy
  chrome://global/content/bindings/scrollbar.xml#thumb
  chrome://global/content/bindings/scale.xml#scalethumb
  chrome://global/content/bindings/scale.xml#scale
  chrome://global/content/bindings/scale.xml#scaleslider
  chrome://global/content/bindings/scrollbox.xml#scrollbox
  chrome://global/content/bindings/scrollbox.xml#arrowscrollbox
  chrome://global/content/bindings/scrollbox.xml#arrowscrollbox-clicktoscroll
  chrome://global/content/bindings/scrollbox.xml#autorepeatbutton
  chrome://global/content/bindings/general.xml#statusbar
  chrome://global/content/bindings/general.xml#statusbarpanel
  chrome://global/content/bindings/general.xml#statusbarpanel-iconic
  chrome://global/content/bindings/general.xml#statusbarpanel-iconic-text
  chrome://global/content/bindings/general.xml#statusbarpanel-menu-iconic
  chrome://global/content/bindings/spinbuttons.xml#spinbuttons
  chrome://global/content/bindings/stringbundle.xml#stringbundleset
  chrome://global/content/bindings/stringbundle.xml#stringbundle
  chrome://global/content/bindings/dialog.xml#dialog
  chrome://global/content/bindings/dialog.xml#dialogheader
  chrome://global/content/bindings/wizard.xml#wizard
  chrome://global/content/bindings/wizard.xml#wizardpage
  chrome://global/content/bindings/wizard.xml#wizard-header
  chrome://global/content/bindings/wizard.xml#wizard-buttons
  chrome://global/content/bindings/preferences.xml#prefwindow
  chrome://global/content/bindings/preferences.xml#prefpane
  chrome://global/content/bindings/preferences.xml#preferences
  chrome://global/content/bindings/preferences.xml#preference
  chrome://global/content/bindings/preferences.xml#panebutton
  chrome://global/content/bindings/expander.xml#expander
  chrome://global/content/bindings/richlistbox.xml#richlistbox
  chrome://global/content/bindings/richlistbox.xml#richlistitem
  chrome://global/content/bindings/datetimepicker.xml#datepicker
  chrome://global/content/bindings/datetimepicker.xml#datepicker-popup
  chrome://global/content/bindings/datetimepicker.xml#datepicker-grid
  chrome://global/content/bindings/datetimepicker.xml#timepicker
  chrome://global/content/bindings/findbar.xml#findbar
  chrome://global/content/bindings/findbar.xml#findbar-textbox
  chrome://global/content/bindings/filefield.xml#filefield
  resource://gre-resources/quirk.css
  chrome://global/skin/scrollbars.css
  chrome://global/content/bindings/scrollbar.xml#scrollbar
  chrome://global/content/bindings/scrollbar.xml#scrollbar-base
  chrome://global/content/bindings/scrollbar.xml#scrollbar
  resource://gre-resources/forms.css
  chrome://global/content/platformHTMLBindings.xml#inputFields
  chrome://global/content/platformHTMLBindings.xml#textAreas
  resource://gre-resources/arrow.gif
  resource://gre-resources/arrowd.gif
  chrome://mozapps/skin/places/defaultFavicon.png
  chrome://global/content/bindings/general.xml
  x:///content/global/bindings/general.xml
  chrome://global/content/bindings/general.xml
  chrome://global/content/bindings/general.xml#basecontrol
  chrome://global/content/bindings/general.xml
  chrome://global/content/bindings/general.xml#basetext
  chrome://global/content/bindings/general.xml#control-item
  chrome://global/content/bindings/general.xml#root-element
  chrome://global/content/bindings/general.xml#iframe
  chrome://global/content/bindings/general.xml#statusbarpanel
  chrome://global/content/bindings/general.xml#statusbarpanel-menu-iconic
  chrome://global/content/bindings/general.xml#statusbar
  chrome://global/content/bindings/general.xml#statusbar-drag
  chrome://global/content/bindings/general.xml#statusbarpanel-iconic
  chrome://global/content/bindings/general.xml#statusbarpanel-iconic-text
  chrome://global/content/bindings/general.xml#image
  chrome://global/content/bindings/general.xml#deck
  chrome://global/content/bindings/general.xml#dropmarker
  chrome://global/content/bindings/general.xml#windowdragbox
  chrome://global/content/bindings/popup.xml
  x:///content/global/bindings/popup.xml
  chrome://global/content/bindings/popup.xml
  chrome://global/content/bindings/popup.xml#popup-base
  chrome://global/content/bindings/popup.xml
  chrome://global/content/bindings/popup.xml#popup
  chrome://global/content/bindings/popup.xml#panel
  chrome://global/content/bindings/popup.xml#tooltip
  chrome://global/content/bindings/popup.xml#popup-scrollbars
  chrome://global/skin/popup.css
  chrome://global/content/bindings/stringbundle.xml
  x:///content/global/bindings/stringbundle.xml
  chrome://global/content/bindings/stringbundle.xml
  chrome://global/content/bindings/stringbundle.xml#stringbundleset
  chrome://global/content/bindings/stringbundle.xml
  chrome://global/content/bindings/stringbundle.xml#stringbundle
  chrome://global/content/bindings/toolbar.xml
  x:///content/global/bindings/toolbar.xml
  chrome://global/content/bindings/toolbar.xml
  chrome://global/content/bindings/toolbar.xml#toolbar-base
  chrome://global/content/bindings/toolbar.xml
  chrome://global/content/bindings/toolbar.xml#toolbox
  chrome://global/content/bindings/toolbar.xml#toolbar
  chrome://global/content/bindings/toolbar.xml#toolbar-menubar-autohide
  chrome://global/content/bindings/toolbar.xml#toolbar-drag
  chrome://global/content/bindings/toolbar.xml#menubar
  chrome://global/content/bindings/toolbar.xml#toolbardecoration
  chrome://global/content/bindings/toolbar.xml#toolbarpaletteitem
  chrome://global/content/bindings/toolbar.xml#toolbarpaletteitem-palette
  chrome://global/skin/toolbar.css
  chrome://global/skin/toolbar/toolbar-separator.png
  chrome://global/skin/10pct_transparent_grey.png
  chrome://global/skin/toolbar/spring.png
  chrome://global/skin/10pct_transparent_grey.png
  chrome://browser/content/urlbarBindings.xml
  x:///content/browser/urlbarBindings.xml
  chrome://browser/content/urlbarBindings.xml
  chrome://browser/content/urlbarBindings.xml#urlbar
  chrome://browser/content/urlbarBindings.xml
  chrome://browser/content/urlbarBindings.xml#browser-autocomplete-result-popup
  chrome://browser/content/urlbarBindings.xml#urlbar-rich-result-popup
  chrome://global/content/bindings/autocomplete.xml
  x:///content/global/bindings/autocomplete.xml
  chrome://global/content/bindings/autocomplete.xml
  chrome://global/content/bindings/autocomplete.xml#autocomplete
  chrome://global/content/bindings/autocomplete.xml
  chrome://global/content/bindings/autocomplete.xml#autocomplete-result-popup
  chrome://global/content/bindings/autocomplete.xml#autocomplete-base-popup
  chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup
  chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistitem
  chrome://global/content/bindings/autocomplete.xml#autocomplete-tree
  chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistbox
  chrome://global/content/bindings/autocomplete.xml#autocomplete-treebody
  chrome://global/content/bindings/autocomplete.xml#autocomplete-treerows
  chrome://global/content/bindings/autocomplete.xml#history-dropmarker
  chrome://global/skin/tree.css
  chrome://global/skin/tree/sort-asc.gif
  chrome://global/skin/tree/sort-dsc.gif
  chrome://global/skin/tree/columnpicker.gif
  chrome://global/skin/autocomplete.css
  chrome://global/skin/icons/autocomplete-dropmarker.png
  chrome://browser/content/tabbrowser.xml
  x:///content/browser/tabbrowser.xml
  chrome://browser/content/tabbrowser.xml
  chrome://browser/content/tabbrowser.xml#tabbrowser
  chrome://browser/content/tabbrowser.xml
  chrome://browser/content/tabbrowser.xml#tabbrowser-arrowscrollbox
  chrome://browser/content/tabbrowser.xml#tabbrowser-tabs
  chrome://browser/content/tabbrowser.xml#tabbrowser-close-tab-button
  chrome://browser/content/tabbrowser.xml#tabbrowser-tab
  chrome://browser/content/tabbrowser.xml#tabbrowser-alltabs-popup
  chrome://browser/content/tabbrowser.css
  chrome://browser/content/tabbrowser.xml#tabbrowser-tabs
  chrome://browser/content/tabbrowser.xml#tabbrowser-tab
  chrome://browser/content/tabbrowser.xml#tabbrowser-arrowscrollbox
  chrome://browser/content/tabbrowser.xml#tabbrowser-alltabs-popup
  chrome://browser/content/tabbrowser.xml#tabbrowser-close-tab-button
  chrome://global/content/bindings/tabbox.xml
  x:///content/global/bindings/tabbox.xml
  chrome://global/content/bindings/tabbox.xml
  chrome://global/content/bindings/tabbox.xml#tab-base
  chrome://global/content/bindings/tabbox.xml
  chrome://global/content/bindings/tabbox.xml#tabbox
  chrome://global/content/bindings/tabbox.xml#tabs
  chrome://global/content/bindings/tabbox.xml#tabpanels
  chrome://global/content/bindings/tabbox.xml#tab
  chrome://global/skin/tabbox.css
  chrome://global/content/bindings/scrollbox.xml
  x:///content/global/bindings/scrollbox.xml
  chrome://global/content/bindings/scrollbox.xml
  chrome://global/content/bindings/scrollbox.xml#scrollbox-base
  chrome://global/content/bindings/scrollbox.xml
  chrome://global/content/bindings/scrollbox.xml#scrollbox
  chrome://global/content/bindings/scrollbox.xml#arrowscrollbox
  chrome://global/content/bindings/scrollbox.xml#autorepeatbutton
  chrome://global/content/bindings/scrollbox.xml#arrowscrollbox-clicktoscroll
  chrome://global/skin/scrollbox.css
  chrome://global/skin/arrow/arrow-lft-sharp.gif
  chrome://global/skin/arrow/arrow-rit-sharp.gif
  chrome://global/skin/arrow/arrow-lft-dis.gif
  chrome://global/skin/arrow/arrow-rit-dis.gif
  chrome://global/skin/arrow/arrow-up-sharp.gif
  chrome://global/skin/arrow/arrow-dn-sharp.gif
  chrome://global/skin/arrow/arrow-up-dis.gif
  chrome://global/skin/arrow/arrow-dn-dis.gif
  chrome://global/skin/menu/menu-arrow.png
  chrome://global/content/bindings/toolbarbutton.xml
  x:///content/global/bindings/toolbarbutton.xml
  chrome://global/content/bindings/toolbarbutton.xml
  chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton
  chrome://global/content/bindings/toolbarbutton.xml
  chrome://global/content/bindings/toolbarbutton.xml#menu
  chrome://global/content/bindings/toolbarbutton.xml#menu-vertical
  chrome://global/content/bindings/toolbarbutton.xml#menu-button
  chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-image
  chrome://global/skin/toolbarbutton.css
  chrome://global/skin/arrow/arrow-dn.png
  chrome://global/skin/arrow/arrow-dn-dis.png
  chrome://global/skin/arrow/arrow-dn.png
  chrome://global/skin/arrow/arrow-dn-dis.png
  chrome://global/content/bindings/button.xml
  x:///content/global/bindings/button.xml
  chrome://global/content/bindings/button.xml
  chrome://global/content/bindings/button.xml#button-base
  chrome://global/content/bindings/button.xml
  chrome://global/content/bindings/button.xml#button
  chrome://global/content/bindings/button.xml#button-periodic-redraw
  chrome://global/content/bindings/button.xml#menu
  chrome://global/content/bindings/button.xml#menu-button-base
  chrome://global/content/bindings/button.xml#menu-button
  chrome://global/content/bindings/button.xml#button-image
  chrome://global/content/bindings/button.xml#button-repeat
  chrome://global/content/bindings/text.xml
  x:///content/global/bindings/text.xml
  chrome://global/content/bindings/text.xml
  chrome://global/content/bindings/text.xml#text-base
  chrome://global/content/bindings/text.xml
  chrome://global/content/bindings/text.xml#text-label
  chrome://global/content/bindings/text.xml#label-control
  chrome://global/content/bindings/text.xml#text-link
  chrome://global/skin/dropmarker.css
  chrome://global/skin/arrow/arrow-dn.gif
  chrome://global/skin/arrow/arrow-dn-dis.gif
  chrome://global/content/bindings/notification.xml
  x:///content/global/bindings/notification.xml
  chrome://global/content/bindings/notification.xml
  chrome://global/content/bindings/notification.xml#notificationbox
  chrome://global/content/bindings/notification.xml
  chrome://global/content/bindings/notification.xml#notification
  chrome://global/content/bindings/browser.xml
  x:///content/global/bindings/browser.xml
  chrome://global/content/bindings/browser.xml
  chrome://global/content/bindings/browser.xml#browser
  chrome://global/content/bindings/browser.xml
  chrome://global/content/bindings/findbar.xml
  x:///content/global/bindings/findbar.xml
  chrome://global/content/bindings/findbar.xml
  chrome://global/content/bindings/findbar.xml#findbar-textbox
  chrome://global/content/bindings/findbar.xml
  chrome://global/content/bindings/findbar.xml#findbar
  chrome://global/skin/findBar.css
  chrome://global/skin/icons/closetab.png
  chrome://global/skin/icons/closetab-hover.png
  chrome://global/skin/icons/closetab-active.png
  chrome://global/skin/icons/checkbox.png
  chrome://global/skin/icons/search-textbox.png
  chrome://global/content/bindings/progressmeter.xml
  x:///content/global/bindings/progressmeter.xml
  chrome://global/content/bindings/progressmeter.xml
  chrome://global/content/bindings/progressmeter.xml#progressmeter
  chrome://global/content/bindings/progressmeter.xml
  chrome://global/content/bindings/progressmeter.xml#progressmeter-undetermined
  chrome://global/content/bindings/progressmeter.xml#progressmeter-periodic-redraw
  chrome://global/skin/progressmeter.css
  chrome://global/content/bindings/textbox.xml
  x:///content/global/bindings/textbox.xml
  chrome://global/content/bindings/textbox.xml
  chrome://global/content/bindings/textbox.xml#textbox
  chrome://global/content/bindings/textbox.xml
  chrome://global/content/bindings/textbox.xml#timed-textbox
  chrome://global/content/bindings/textbox.xml#search-textbox
  chrome://global/content/bindings/textbox.xml#textarea
  chrome://global/content/bindings/textbox.xml#input-box
  chrome://global/content/bindings/textbox.xml#input-box-spell
  chrome://global/content/textbox.css
  chrome://global/skin/textbox.css
  chrome://global/content/autocomplete.css
  chrome://global/skin/icons/searchfield-regular-cancel.png
  chrome://global/skin/icons/searchfield-small-cancel.png
  chrome://browser/content/places/toolbar.xml
  x:///content/browser/places/toolbar.xml
  chrome://browser/content/places/toolbar.xml
  chrome://browser/content/places/toolbar.xml#places-bar
  chrome://browser/content/places/toolbar.xml
  chrome://browser/skin/places/places.css
  chrome://global/skin/icons/menulist-dropmarker.png
  chrome://mozapps/skin/places/defaultFavicon.png
  chrome://browser/skin/places/livemarkItem.png
  chrome://global/skin/tree/folder.png
  chrome://browser/skin/places/allBookmarks.png
  chrome://browser/skin/page-livemarks.png
  chrome://browser/skin/places/bookmarksToolbar.png
  chrome://browser/skin/places/bookmarksMenu.png
  chrome://browser/skin/places/unfiledBookmarks.png
  chrome://browser/skin/places/query.png
  chrome://mozapps/skin/places/tagContainerIcon.png
  chrome://browser/skin/places/history.png
  chrome://global/skin/tree/folder.png
  chrome://global/skin/tree/folder.png
  chrome://browser/skin/places/history.png
  chrome://global/skin/tree/folder.png
  chrome://browser/content/places/menu.xml
  x:///content/browser/places/menu.xml
  chrome://browser/content/places/menu.xml
  chrome://browser/content/places/menu.xml#places-popup-base
  chrome://browser/content/places/menu.xml
  chrome://browser/content/places/menu.xml#places-menupopup
  chrome://global/skin/button.css
  chrome://global/skin/icons/question-mark.png
  chrome://browser/content/search/search.xml
  x:///content/browser/search/search.xml
  chrome://browser/content/search/search.xml
  chrome://browser/content/search/search.xml#searchbar
  chrome://browser/content/search/search.xml
  chrome://browser/content/search/search.xml#searchbar-textbox
  chrome://browser/content/search/searchbarBindings.css
  chrome://browser/content/search/search.xml#searchbar-textbox
  chrome://browser/skin/searchbar.css
  chrome://mozapps/skin/places/defaultFavicon.png
  chrome://browser/skin/Search-addengines.png
  chrome://global/skin/arrow/arrow-dn.png
  chrome://browser/skin/Search.png
  chrome://global/content/bindings/menu.xml
  x:///content/global/bindings/menu.xml
  chrome://global/content/bindings/menu.xml
  chrome://global/content/bindings/menu.xml#menuitem-base
  chrome://global/content/bindings/menu.xml
  chrome://global/content/bindings/menu.xml#menu-base
  chrome://global/content/bindings/menu.xml#menu
  chrome://global/content/bindings/menu.xml#menuitem
  chrome://global/content/bindings/menu.xml#menu-menubar
  chrome://global/content/bindings/menu.xml#menu-menubar-iconic
  chrome://global/content/bindings/menu.xml#menuitem-iconic
  chrome://global/content/bindings/menu.xml#menuitem-iconic-noaccel
  chrome://global/content/bindings/menu.xml#menuitem-iconic-desc-noaccel
  chrome://global/content/bindings/menu.xml#menu-iconic
  chrome://global/content/bindings/menu.xml#menubutton-item
  chrome://global/content/bindings/menu.xml#menuseparator
  chrome://global/skin/menu.css
  chrome://global/skin/menu/menu-arrow.png
  chrome://global/skin/menu/menu-check.png
  chrome://global/skin/menu/menu-check.png
  chrome://global/skin/columnselect.gif
  resource://gre-resources/loading-image.png
  resource://gre-resources/broken-image.png
  resource://gre-resources/loading-image.png
  resource://gre-resources/broken-image.png

Comment 25

[Andreas Gal :gal]

Here is an example:

The CC has identified a bunch of cycles and is unlinking them. It unlinks the JSObject part of a wrapper first. That goes into nsXPConnect::RootAndUnlinkJSObjects(), where we don't do anything.

unlink:
object 0x11e6acd00
class 0x11f638b38 Window
properties:
proto <XPC_WN_ModsAllowed_NoCall_Proto_JSClass object at 0x11e625bc0>
parent null
private 0x11f638cf0
slots:

We will find the same native wrapper being held here:

unlink:
object 0x11e686cc0
class 0x101d479e0 XPCNativeWrapper
properties:
proto null
parent <Window object at 0x11e6acd00>
private 0x11f638cf0
slots:
   3 (reserved) = 2

Here again we don't do anything.

Eventually we Unlink the wrapper via XPCWrappedNative::RootAndUnlinkJSObject(p), with p being the native wrapper, not a JSObject as the name would suggest:

expirewrapper: 1f638cf0

So for this window object this works as expected.

However, for this particular ChromeWindow object this doesn't work as expected:

unlink:
object 0x11e6f0300
class 0x11e8484f8 ChromeWindow
properties:
proto <XPC_WN_ModsAllowed_NoCall_Proto_JSClass object at 0x11e6f0780>
parent null
private 0x1043c1a50

We never see the wrapper (0x1043c1a50) being expired.

Oh wrapper expiration callback, Where Art Thou?

Comment 26

[Andreas Gal :gal]

Ok, bz thinks that the ChromeWindow goes into AddJSHolder(), which always QIs to an nsXPCOMCycleCollectionParticipant, which again implements an empty RootAndUnlinkJSObjects(). I think that's a bug of some sort. Maybe Blake can sort it out.

In the meantime, it should be perfectly safe (and perfectly ugly) to reach from the JSObject into the wrapper and expire it from there.

Comment 27

[Boris Zbarsky [:bzbarsky]]

What goes into AddJSHolder are elements, not windows, but the former can of course entrain the latter.

Comment 28

[Peter Van der Beken [:peterv]]

(In reply to comment #0)
> A bit of archeology shows that way back we decided to intertwine GC and CC to
> avoid a redundant scan/mark. That's basically exactly what I propose we try
> now, since we can make it concurrent and interruptible. We will need some
> experimenting with this to see whether comment #1 is faster, or the decision
> back in 2007 was right after all. Doesn't look super hard to hack that up
> though (at least to he point where we can benchmark it), since its mostly
> undoing changes.

> https://bugzilla.mozilla.org/show_bug.cgi?id=377884#c9

The current dance between GC and CC mostly came about in bug 379718 comment 19, with followups in bug 401687. At the time it was a significant improvement to CC times, because it avoided duplicating work between the two traversals of the JS heap (one for the GC with a callback to calculate real refcounts and a partial one for the CC to discover cycles). It might block us from doing other improvements, but it did improve the performance of the CC (I know, it's hard to imagine it used to be worse).

(In reply to comment #8)
> Cycles involving JS Objects are not immediately collected.
> Since cycles are long-lived, this mild delay until the cycle gets collected
> should be immaterial.

We'll have to make sure that this doesn't keep traversing collectable cycles in the next CC.

I'm not really sure about how this works with wrappers. If the JS object is kept alive until the next CC but its wrapper is expired, won't we risk handing out an expired wrapper if somebody asks for it?

(In reply to comment #26)
> Ok, bz thinks that the ChromeWindow goes into AddJSHolder(), which always QIs
> to an nsXPCOMCycleCollectionParticipant, which again implements an empty
> RootAndUnlinkJSObjects(). I think that's a bug of some sort.

Not really sure what you mean, RootAndUnlinkJSObjects is virtual. nsXPCOMCycleCollectionParticipant::RootAndUnlinkJSObjects is empty because most objects don't hold JS objects. What code passes an object that holds a JS object but has an empty RootAndUnlinkJSObjects in its CC participant to AddJSHolder?

Comment 29

[Peter Van der Beken [:peterv]]

RootAndUnlinkJSObject might be badly named, it really means root the object being unlinked (which boils down to AddRef'ing C++ objects) and make that object forget about any JS objects it's holding. We needed to combine these two operations. We needed to make sure C++ objects don't have references to JS objects that are swept. And we needed to keep C++ objects alive for unlinking across the sweep phase, since sweeping JS objects used to actually release some C++ objects. So we root C++ objects, unlink their edges to JS objects, sweep JS objects, unlink C++ objects, and unroot C++ objects. AFAICT in the new world we might be able to move all unlinking back into Unlink.

Comment 30

[Andreas Gal :gal]

The principal problem is that we currently ignore most Unlink() operations on XPCOM objects such as wrappers. Most of them end up in nsXPConnect::RootAndUnlinkJSObject, which is empty, instead of XPCWrappedNative::_cycleCollectorGlobal::RootAndUnlinkJSObject as one would expect. This has to do with the way we select CycleCollectorParticipants. As a result we currently rely on JSObjects to go dead to break the cycle. That exactly is what the patch in this bug no longer does since we are separated from the GC cycle. I wanted to rely on Unlink of XPCOM objects, but that seems far fetched at this point (a lot of different things to implement Unlink for). Instead I will make sure objects the CC declares dead are not marked during the next GC run.

Comment 31

[Andreas Gal :gal]

Attachment: patch

Unlink JSObjects by entering them into a "dead objects" hash set that persists until the next GC.

Comment 32

[Andreas Gal :gal]

Attachment: patch

Comment 33

[Andreas Gal :gal]

Attachment: patch

Comment 34

[Andreas Gal :gal]

Attachment: patch

remove bogus asserts, DEBUG_CC should work again

Comment 35

[Andreas Gal :gal]

Attachment: refreshed patch, untested

Comment 36

[Andreas Gal :gal]

Attachment: CC is allowed to mark in JSGC_MARK_END

Comment 37

[Andreas Gal :gal]

Attachment: patch

refresh, clear weak roots when we are about to cc

Comment 38

[Andreas Gal :gal]

This looks good. No more shutdown leaks in my local testing. Send the patch to the try server.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1276654212.1276654987.17830.gz&fulltext=1

Comment 40

[Andreas Gal :gal]

We still leak on tinderbox.

Comment 41

[Peter Van der Beken [:peterv]]

Actually, it looks like we crash sometimes, see for example: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1276657700.1276657962.30057.gz&fulltext=1#err0

Also, it didn't build on Linux and Windows because of missing JS_FRIEND_API on js_IsAboutToBeFinalized, js_SetMarkColor and js_GCThingIsMarked in jsgc.h

Comment 42

[Andreas Gal :gal]

I don't think the crash is us. That looks like a current botch in TM tip (I think). Any ideas on the remaining leak? It doesn't show for startup/shutdown any more for me.

Comment 43

[Peter Van der Beken [:peterv]]

Attachment: clean patch

I've pushed this on an almost tip trunk tree to tryserver, so far only known orange on Linux but no unknown leaks. Still need to wait for the other platforms. It's probably a bad idea to push patches to tryserver that have DEBUG_CC defined.

Comment 44

[Peter Van der Beken [:peterv]]

Other platforms look good too, only a bunch of known orange.

Comment 45

[Andreas Gal :gal]

Maybe it was the DEBUG_CC flag? That would be great.

Comment 46

[Andreas Gal :gal]

Review ping. Patch passes try server. Blocking the rest of my CC patch queue.

Comment 47

[Andreas Gal :gal]

Review ping. I need to land this soon, it blocks a bunch of GC refactoring work.

Comment 48

[Peter Van der Beken [:peterv]]

Comment on attachment 451561 [details] [diff] [review]
clean patch

>diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h

>@@ -1662,23 +1663,20 @@ struct JSContext

>-    /* Flag to indicate that we run inside gcCallback(cx, JSGC_MARK_END). */
>-    JSPackedBool        insideGCMarkCallback;

I don't understand why you can remove this, please get review on this from a JS peer.

>diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp

>+IsMarkedGCThing(void *thing, uint32 color)
> {
>     size_t index;
>     jsbitmap *markBitmap = GetGCThingMarkBit(thing, index);
>-    return !!JS_TEST_BIT(markBitmap, index);
>+    return !!JS_TEST_BIT(markBitmap, index + color);

>+MarkIfUnmarkedGCThing(void *thing, uint32 color)
> {
>     size_t index;
>     jsbitmap *markBitmap = GetGCThingMarkBit(thing, index);
>     if (JS_TEST_BIT(markBitmap, index))
>         return false;
>     JS_SET_BIT(markBitmap, index);
>+    if (color)
>+        JS_SET_BIT(markBitmap, index + color);

Shouldn't these verify (assert) that color is allowed for thing?

>+JS_FRIEND_API(bool)
>+js_GCThingIsMarked(void *thing, uint32 color)
>+{
>+    JS_ASSERT(color != BLACK);

Not sure why we would disallow BLACK?

>diff --git a/js/src/xpconnect/src/nsXPConnect.cpp b/js/src/xpconnect/src/nsXPConnect.cpp

>@@ -389,42 +384,33 @@ nsXPConnect::Collect()

>+    // Phase:

Phases

>     //
>     // 1. marking of the roots in category 1 by having the JS GC do its marking

>+    // 2. marking of the roots in category 2 by XPCJSRuntime::TraceXPConnectRoots
>+    //    using an additional color (gray).
>+    // 3. end of GC, GC can sweep its heap
>+    // 4. walk gray objects and add them to the cycle collector, cycle collect

Maybe clarify that JS objects that become unreachable from the cycle collection will be swept by the next GC?

>@@ -668,25 +650,36 @@ WrapperIsNotMainThreadOnly(XPCWrappedNat

> nsXPConnect::Traverse(void *p, nsCycleCollectionTraversalCallback &cb)
> {
>     if(!mCycleCollectionContext)
>         return NS_ERROR_FAILURE;
> 
>     JSContext *cx = mCycleCollectionContext->GetJSContext();
> 
>     uint32 traceKind = js_GetGCThingTraceKind(p);
>+    if(!ADD_TO_CC(traceKind))
>+        return NS_OK;

I don't think we can do this. What prevents anyone from adding a non-JSObject to the cycle collector as a root? I think we should move this check to nsXPConnect::ToParticipant, ie return null there if !ADD_TO_CC(js_GetGCThingTraceKind(p)) and make this one an assertion. That should prevent JS objects other than JSObjects from being added to the CC. BTW, using GRAY for JSTRACE_XML is ok?

>@@ -924,16 +913,19 @@ nsXPConnect::GetRequestDepth(JSContext* 

> class JSContextParticipant : public nsCycleCollectionParticipant
> {
> public:
>     NS_IMETHOD RootAndUnlinkJSObjects(void *n)
>     {
>+        JSContext *cx = static_cast<JSContext*>(n);
>+        NS_ASSERTION(cx->globalObject, "global object NULL before unlinking");
>+        cx->globalObject = nsnull;

This looks a bit scary. I think we didn't do it before because there's no real order to unlinking, let's hope nothing uses globalObject during unlinking.

>diff --git a/js/src/xpconnect/src/xpcjsruntime.cpp b/js/src/xpconnect/src/xpcjsruntime.cpp

>+static const uint32 GRAY = 1;

Can we at least share this with nsXPConnect? Having the same value defined in multiple files is annoying.

>+void
>+XPCJSRuntime::ClearWeakRoots()
> {
>-    mUnrootedGlobalCount = 0;
>     JSContext *iter = nsnull, *acx;
>+
>     while((acx = JS_ContextIterator(GetJSRuntime(), &iter)))
>     {
>-        NS_ASSERTION(!JS_HAS_OPTION(acx, JSOPTION_UNROOTED_GLOBAL),
>-                     "unrooted global should be set only during CC");
>         if(XPCPerThreadData::IsMainThread(acx) &&
>-           nsXPConnect::GetXPConnect()->GetRequestDepth(acx) == 0)
>-        {
>+           nsXPConnect::GetXPConnect()->GetRequestDepth(acx) == 0) {

{ goes on its own line in XPConnect.

>             JS_ClearNewbornRoots(acx);
>-            if(acx->globalObject)
>-            {
>-                JS_ToggleOptions(acx, JSOPTION_UNROOTED_GLOBAL);
>-                ++mUnrootedGlobalCount;
>-            }

>@@ -500,16 +460,22 @@ JSBool XPCJSRuntime::GCCallback(JSContex

>+                // We always want cx->globalObject to be unrooted.
>+                JSContext *iter = nsnull, *acx;
>+                while ((acx = JS_ContextIterator(self->GetJSRuntime(), &iter))) {

{ goes on its own line in XPConnect.

>+                    if (!JS_HAS_OPTION(acx, JSOPTION_UNROOTED_GLOBAL))
>+                        JS_ToggleOptions(acx, JSOPTION_UNROOTED_GLOBAL);
>+                }

Why not keep the toggling of JSOPTION_UNROOTED_GLOBAL and the clearing of the weak roots combined, and call them from the XPCJSRuntime::GCCallback? Afaik clearing weak roots is a good thing even if we don't cycle collect, so it makes sense to do it in this callback.

>diff --git a/js/src/xpconnect/src/xpcprivate.h b/js/src/xpconnect/src/xpcprivate.h

>@@ -469,16 +469,18 @@ public:

>+    static JSBool IsGray(void *thing);

Add a comment explaining what this means. I know there's not many comments in this file, but still.

>diff --git a/js/src/xpconnect/src/xpcvariant.cpp b/js/src/xpconnect/src/xpcvariant.cpp

>@@ -102,19 +102,20 @@ void XPCTraceableVariant::TraceJS(JSTrac

>-    if(JSVAL_IS_OBJECT(tmp->mJSVal))
>-        cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT,
>-                           JSVAL_TO_OBJECT(tmp->mJSVal));
>+    if(JSVAL_IS_OBJECT(tmp->mJSVal)) {
>+        JSObject *obj = JSVAL_TO_OBJECT(tmp->mJSVal);
>+        cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT, obj);
>+    }

Unnecessary change.

>diff --git a/js/src/xpconnect/src/xpcwrappedjs.cpp b/js/src/xpconnect/src/xpcwrappedjs.cpp

>@@ -69,21 +69,22 @@ NS_CYCLE_COLLECTION_CLASSNAME(nsXPCWrapp

>-    if(refcnt > 1)
>+    if(refcnt > 1) {
>         // nsXPCWrappedJS roots its mJSObj when its refcount is > 1, see
>         // the comment above nsXPCWrappedJS::AddRef.
>         cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT,
>                            tmp->GetJSObject());
>+    }

Unnecessary change.

>diff --git a/js/src/xpconnect/src/xpcwrappednative.cpp b/js/src/xpconnect/src/xpcwrappednative.cpp

>@@ -95,18 +95,19 @@ NS_CYCLE_COLLECTION_CLASSNAME(XPCWrapped

>-        if(NS_SUCCEEDED(rv))
>+        if(NS_SUCCEEDED(rv)) {
>             cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT, obj);
>+        }

Unnecessary change.

Comment 49

[Andreas Gal :gal]

> > 
> >     uint32 traceKind = js_GetGCThingTraceKind(p);
> >+    if(!ADD_TO_CC(traceKind))
> >+        return NS_OK;
> 
> I don't think we can do this. What prevents anyone from adding a non-JSObject
> to the cycle collector as a root? I think we should move this check to

I don't think I understand this. Only XML and OBJ can have interior pointers to anything and thus can participate in cycles. Adding a string as a CC root is a bug and should assert.

> nsXPConnect::ToParticipant, ie return null there if
> !ADD_TO_CC(js_GetGCThingTraceKind(p)) and make this one an assertion. That
> should prevent JS objects other than JSObjects from being added to the CC. BTW,
> using GRAY for JSTRACE_XML is ok?

Yes, gray is safe for anything thats larger than 8 bytes, which is object and xml (and also string as it happens, but those we never mark gray).

Comment 50

[Andreas Gal :gal]

Attachment: patch

The insideGCCallback hack is no longer necessary, because we no longer "revive" (mark) objects from within the callback after GC_MARK_END. Instead, the GC already marked all the objects (we don't clear out roots, we merely mark global objects alter, but we still mark them as part of the regular GC). GC objects that were found to be in circles are disposed at the NEXT GC. Hense we no longer have to worry about incorrectly reporting the result of "IsAboutToBeFinalized" because we haven't processed delayed children yet. It can't happen any more. Makes sense?

Comment 51

[Brendan Eich [:brendan]]

Can we use insideGCCallback ifdef DEBUG to assert that no one can mark after GC_MARK_END? Or something like that, just wondering how we can do without this flag (would need #ifdef'ed setting and clearing) and still assert.

/be

Comment 52

[Peter Van der Beken [:peterv]]

(In reply to comment #49)
> > > 
> > >     uint32 traceKind = js_GetGCThingTraceKind(p);
> > >+    if(!ADD_TO_CC(traceKind))
> > >+        return NS_OK;
> > 
> > I don't think we can do this. What prevents anyone from adding a non-JSObject
> > to the cycle collector as a root? I think we should move this check to
> 
> I don't think I understand this. Only XML and OBJ can have interior pointers to
> anything and thus can participate in cycles. Adding a string as a CC root is a
> bug and should assert.

The point is that you're not asserting it, you're silently ignoring it at a point where it's confusing and potentially a problem (the objects are in the graph already, but you're leaving their description in the CC empty). That's why I propose moving it to ToParticipant, which avoids those objects from getting into the graph in the first place. I personally don't think I want to force the participants to do the check and rather make the CC ignore instead of assert, if you want to just make it assert that's fine by me but you'll need to make sure we don't currently do it.

Comment 53

[Peter Van der Beken [:peterv]]

insideGCCallback predates the CC, if it's the only remaining user then removing it is fine by me. I just don't know if it's the only user.

Comment 54

[Andreas Gal :gal]

Attachment: patch

Comment 55

[Andreas Gal :gal]

Comment on attachment 456399 [details] [diff] [review]
patch

Sent to try server after discussing the remaining issues with peterv. Still need a JS side r+ for the insideGCCallback flag removal.

Comment 56

[Igor Bukanov]

Comment on attachment 456399 [details] [diff] [review]
patch

>diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp
>+/*
>+ * Live objects are marked black. How many other additional colors are available
>+ * depends on the size of the GCThing. String and doubles have no extra colors.
>+ * JSObjects have 3 extra colors available.
>+ */
>+static const uint32 BLACK = 0;
>+
>+static void
>+AssertValidColor(void *thing, uint32 color)
>+{
>+    JS_ASSERT_IF(color, color < JSGCArenaInfo::fromGCThing(thing)->list->thingSize / GC_CELL_SIZE);
>+}
>+

The comment is wrong as sizeof(JSString) == 4 words. Plus it is not
compatible with fatvalues patch which would remove the doubles. Plus with
fatvalues it would make sense to bump GC_CELL_SIZE to 4 words. Plus the
XPConnect does not need colors for strings. So comment that currently only
single extra color for object, function and xml things is supported and
assert that in AssertValidColor.

> inline bool
>-IsMarkedGCThing(void *thing)
>+IsMarkedGCThing(void *thing, uint32 color)
> {

Default the color here to BLACK so IsMarkedGCThing(thing) can be kept in
most of the places as is.

>+    AssertValidColor(thing, color);
>+
>     size_t index;
>     jsbitmap *markBitmap = GetGCThingMarkBit(thing, index);
>-    return !!JS_TEST_BIT(markBitmap, index);
>+    return !!JS_TEST_BIT(markBitmap, index + color);
> }
> 
> inline bool
>-MarkIfUnmarkedGCThing(void *thing)
>+MarkIfUnmarkedGCThing(void *thing, uint32 color)
> {

Write comments before the function regarding the exact semantics
in presence of colors and default color to black to minimize the patch.

>+    AssertValidColor(thing, color);
>+
>     size_t index;
>     jsbitmap *markBitmap = GetGCThingMarkBit(thing, index);
>     if (JS_TEST_BIT(markBitmap, index))
>         return false;
>     JS_SET_BIT(markBitmap, index);
>+    if (color)
>+        JS_SET_BIT(markBitmap, index + color);
>     return true;
> }
> 
> inline size_t
> ThingsPerArena(size_t thingSize)
> {
>     JS_ASSERT(!(thingSize & GC_CELL_MASK));
>     JS_ASSERT(thingSize <= GC_ARENA_SIZE);
>@@ -868,23 +887,46 @@ js_GetGCThingTraceKind(void *thing)
> 
> JSRuntime *
> js_GetGCThingRuntime(void *thing)
> {
>     jsuword chunk = JSGCArena::fromGCThing(thing)->getChunk();
>     return JSGCChunkInfo::fromChunk(chunk)->runtime;
> }
> 
>-bool
>+JS_FRIEND_API(bool)
> js_IsAboutToBeFinalized(void *thing)
> {
>     if (JSString::isStatic(thing))
>         return false;
> 
>-    return !IsMarkedGCThing(thing);
>+    return !IsMarkedGCThing(thing, BLACK);
>+}
>+
>+static void
>+MarkDelayedChildren(JSTracer *trc);
>+
>+JS_FRIEND_API(uint32)
>+js_SetMarkColor(JSTracer *trc, uint32 color)
>+{

Comment at the function declaration that the color will only be applied to
object, function and xml things.

Nit: assert here that trc == rt->gcMarkingTracer.

>+    JS_ASSERT(color < sizeof(JSObject)/GC_CELL_SIZE);
>+
>+    /* Must process any delayed tracing here, otherwise we confuse colors. */
>+    MarkDelayedChildren(trc);
>+
>+    JSRuntime *rt = trc->context->runtime;
>+    uint32 oldColor = rt->gcMarkColor;

Move gcMarkColor into a sublass of JSTracer that the GC uses to avoid adding
to the runtime yet another flag that is relevant only during GC-marking. Also
the code should assert that no color-to-color transitions should happen,
i.e. either old or new color should be black. 

>+JS_FRIEND_API(bool)
>+js_GCThingIsMarked(void *thing, uint32 color)
>+{

Given that for now the only one color is supported rename this into
js_IsColoredGCThing(thing).

>diff --git a/js/src/xpconnect/src/xpcjsruntime.cpp b/js/src/xpconnect/src/xpcjsruntime.cpp
>@@ -304,23 +304,21 @@ void XPCJSRuntime::TraceJS(JSTracer* trc
...
>+
>+    // Mark these roots as gray so the CC can walk them later.
>+    uint32 oldColor = js_SetMarkColor(trc, XPC_GC_COLOR_GRAY);
>+    self->TraceXPConnectRoots(trc);

TraceJS currently is called before the conservative stack scanner scans the
stack, see js_TraceRuntime. So the latter should be changed so 
rt->gcExtraRootsTraceOp is called as the last thing is js_TraceRuntime with
corresponding commnets.

>@@ -502,16 +461,23 @@ JSBool XPCJSRuntime::GCCallback(JSContex
...
>+                // We always want cx->globalObject to be unrooted.
>+                JSContext *iter = nsnull, *acx;

The comment is misleading. The intention is to do a custom marking in
TraceXPConnectRoots so rephrase appropriately. Also move this flag 
setting into XPCJSRuntime::OnJSContextNew since it will be kept
permanently AFAICS.

Comment 57

[Andreas Gal :gal]

> The comment is wrong as sizeof(JSString) == 4 words. Plus it is not
> compatible with fatvalues patch which would remove the doubles. Plus with
> fatvalues it would make sense to bump GC_CELL_SIZE to 4 words. Plus the
> XPConnect does not need colors for strings. So comment that currently only
> single extra color for object, function and xml things is supported and
> assert that in AssertValidColor.

I will remove the comment. If we decide to bump cell size, we can change the code accordingly. For now the assert is valid and meaningful.

Comment 58

[Andreas Gal :gal]

> Comment at the function declaration that the color will only be applied to
> object, function and xml things.

Thats incorrect, especially functions need to be marked this way as well.

> Move gcMarkColor into a sublass of JSTracer that the GC uses to avoid adding
> to the runtime yet another flag that is relevant only during GC-marking. Also

The GC stuff will move into its own struct from runtime. Right now it makes sense to keep everything in one place, which is runtime (which is wrong, but will change soon en-block).

> the code should assert that no color-to-color transitions should happen,
> i.e. either old or new color should be black. 

Why not? This seems like a bogus constraint.

> Given that for now the only one color is supported rename this into
> js_IsColoredGCThing(thing).

We support 3 colors. Its just a name. Neither naming seems massively superior.

> TraceJS currently is called before the conservative stack scanner scans the
> stack, see js_TraceRuntime. So the latter should be changed so 
> rt->gcExtraRootsTraceOp is called as the last thing is js_TraceRuntime with
> corresponding commnets.

I don't think this makes sense. We want to mark strong roots first, and color them accordingly. I switch back to black before the conservative scanner runs and those are all strong (BLACK only roots). If we follow your approach and some XPConnect root is found by the stack scanner, we don't disclose parts of the GC heap to the cycle collector, and we might miss leaks. Please explain why this change is a good idea.

Comment 59

[Andreas Gal :gal]

Attachment: patch

Comment 60

[Igor Bukanov]

(In reply to comment #58)
> > Comment at the function declaration that the color will only be applied to
> > object, function and xml things.
> 
> Thats incorrect, especially functions need to be marked this way as well.

I have wrote that "the color will only be applied to "object, *function* and xml things".


> The GC stuff will move into its own struct from runtime. Right now it makes
> sense to keep everything in one place, which is runtime (which is wrong, but
> will change soon en-block).

The suggestion is to move the flag into per GC-marking structure. This is not about moving GC-stuff from one per-runtime place to another.

> > the code should assert that no color-to-color transitions should happen,
> > i.e. either old or new color should be black. 
> 
> Why not? This seems like a bogus constraint.

Right, that is not necessary, but see below.

> 
> > Given that for now the only one color is supported rename this into
> > js_IsColoredGCThing(thing).
> 
> We support 3 colors. Its just a name. Neither naming seems massively superior.

The observation is that the cycle collector needs only one extra state on top of marked/unmarked. IMO it suggests to have API supporting just that case. The API would query/set the extra state hiding the fact that this is implemented via spare bits in the GC bitmaps.

> We want to mark strong roots first, and color
> them accordingly. I switch back to black before the conservative scanner runs
> and those are all strong (BLACK only roots).

The conservative scan roots must have the same color as the rest of strong roots. This may not happen with the patch as it does:

1. marking as black autoroots and other runtime roots.
2. marking as gray things found during TraceXPConnectRoots(trc) unless 
   they are already marked as black.
3. marking as black that the conservative scanner finds unless they
   already have black or gray color.

So if a thing A is present on the native stack and among things that TraceXPConnectRoots finds, then A would have wrongfully gray color,
not black.

Comment 61

[Andreas Gal :gal]

> > The GC stuff will move into its own struct from runtime. Right now it makes
> > sense to keep everything in one place, which is runtime (which is wrong, but
> > will change soon en-block).
> 
> The suggestion is to move the flag into per GC-marking structure. This is not
> about moving GC-stuff from one per-runtime place to another.

For the record, you are making me change an API (compatibly) instead of hiding this in the engine. Whatever, I really want to land this patch and have no time to discuss window dressings.

> The observation is that the cycle collector needs only one extra state on top
> of marked/unmarked. IMO it suggests to have API supporting just that case. The
> API would query/set the extra state hiding the fact that this is implemented
> via spare bits in the GC bitmaps.

For cross compartment GCs I may or may not need another of the bits, depending how on how I implement that. This is not an API. Its merely a friend function.

Comment 62

[Andreas Gal :gal]

Attachment: patch

Comment 63

[Andreas Gal :gal]

Attachment: patch

The color in JSTracer has to be initialized. I don't have BLACK defined in jsapi.h so I have to initialize to 0 here.

Comment 64

[Igor Bukanov]

Comment on attachment 456576 [details] [diff] [review]
patch

>diff --git a/js/src/jsapi.h b/js/src/jsapi.h
>--- a/js/src/jsapi.h
>+++ b/js/src/jsapi.h
>@@ -1123,16 +1123,17 @@ JS_MarkGCThing(JSContext *cx, void *thin
>  */
> #define JSVAL_IS_TRACEABLE(v)   (JSVAL_IS_GCTHING(v) && !JSVAL_IS_NULL(v))
> #define JSVAL_TO_TRACEABLE(v)   (JSVAL_TO_GCTHING(v))
> #define JSVAL_TRACE_KIND(v)     (JSVAL_TAG(v) >> 1)
> 
> struct JSTracer {
>     JSContext           *context;
>     JSTraceCallback     callback;
>+    uint32              color;

I should have been more clear here. My suggestion was top make a subclass of JSTracer that the GC would use and store the color there, it was not about expanding the tracer. In details, jsgc.h should have something like:

struct GCMarker : JSTracer {
    GCMarker() : callback(NULL), color(0) {}
    uint32 color;
};

This implies that the type for JSRuntime::gcMarkingTracer would be GCMarker *.

>diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp
>+++ b/js/src/jsgc.cpp
>@@ -492,32 +492,54 @@ GetGCThingMarkBit(void *thing, size_t &b
> {
>     CheckValidGCThingPtr(thing);
>     jsuword addr = reinterpret_cast<jsuword>(thing);
>     jsuword chunk = addr & ~GC_CHUNK_MASK;
>     bitIndex = (addr & GC_CHUNK_MASK) >> GC_CELL_SHIFT;
>     return reinterpret_cast<jsbitmap *>(chunk | GC_MARK_BITMAP_ARRAY_OFFSET);
> }
> 

>+static void
>+AssertValidColor(void *thing, uint32 color)
>+{
>+    JS_ASSERT_IF(color, color < JSGCArenaInfo::fromGCThing(thing)->list->thingSize / GC_CELL_SIZE);


Check here also that the thing is object, function or xml here.

> inline bool
>-MarkIfUnmarkedGCThing(void *thing)
>+MarkIfUnmarkedGCThing(void *thing, uint32 color)
> {
...
@@ -2311,74 +2356,54 @@ js_CallGCMarker(JSTracer *trc, void *thi
>       case JSTRACE_DOUBLE: {
>-        MarkIfUnmarkedGCThing(thing);
>+        MarkIfUnmarkedGCThing(thing, BLACK);
...
>       case JSTRACE_STRING:
...
>-            if (!MarkIfUnmarkedGCThing(thing))
>+            if (!MarkIfUnmarkedGCThing(thing, BLACK))
>                 goto out;
>             if (!((JSString *) thing)->isDependent())
>                 goto out;
>

Repeated nit: default the color to BLACK to avoid changing the code marking strings and doubles. It will minimized the conflicts with the fatvalues patch.

>     JS_ASSERT(kind == GetFinalizableThingTraceKind(thing));
>-    if (!MarkIfUnmarkedGCThing(thing))
>+    if (!MarkIfUnmarkedGCThing(thing, trc->color))

Repeated nit: remind in comments that trc->color does not applies to doubles and strings above so we default to BLACK color for them.

>+JSBool
>+nsXPConnect::IsGray(void *thing)
>+{
>+    return js_GCThingIsMarked(thing, XPC_GC_COLOR_GRAY);
>+}
>+
> NS_IMETHODIMP
> nsXPConnect::Traverse(void *p, nsCycleCollectionTraversalCallback &cb)
> {
>     if(!mCycleCollectionContext)
>         return NS_ERROR_FAILURE;
> 
>     JSContext *cx = mCycleCollectionContext->GetJSContext();
> 
>     uint32 traceKind = js_GetGCThingTraceKind(p);
>+

Nit: this is unnecessary change.

r+ with this addressed.

Comment 65

[Andreas Gal :gal]

(In reply to comment #64)
> (From update of attachment 456576 [details] [diff] [review])
> >diff --git a/js/src/jsapi.h b/js/src/jsapi.h
> >--- a/js/src/jsapi.h
> >+++ b/js/src/jsapi.h
> >@@ -1123,16 +1123,17 @@ JS_MarkGCThing(JSContext *cx, void *thin
> >  */
> > #define JSVAL_IS_TRACEABLE(v)   (JSVAL_IS_GCTHING(v) && !JSVAL_IS_NULL(v))
> > #define JSVAL_TO_TRACEABLE(v)   (JSVAL_TO_GCTHING(v))
> > #define JSVAL_TRACE_KIND(v)     (JSVAL_TAG(v) >> 1)
> > 
> > struct JSTracer {
> >     JSContext           *context;
> >     JSTraceCallback     callback;
> >+    uint32              color;
> 
> I should have been more clear here. My suggestion was top make a subclass of
> JSTracer that the GC would use and store the color there, it was not about
> expanding the tracer. In details, jsgc.h should have something like:
> 
> struct GCMarker : JSTracer {
>     GCMarker() : callback(NULL), color(0) {}
>     uint32 color;
> };
> 
> This implies that the type for JSRuntime::gcMarkingTracer would be GCMarker *.

SetColor takes a JSTracer, because thats what the API exposes to XPConnect, so in there I am going to upcast unsafely to a GCMarker? This is completely pointless and twice as ugly as before.

Comment 66

[Igor Bukanov]

(In reply to comment #65)
> > I should have been more clear here. My suggestion was top make a subclass of
> > JSTracer that the GC would use and store the color there, it was not about
> > expanding the tracer. In details, jsgc.h should have something like:
> > 
> > struct GCMarker : JSTracer {
> >     GCMarker() : callback(NULL), color(0) {}
> >     uint32 color;
> > };
> > 
> > This implies that the type for JSRuntime::gcMarkingTracer would be GCMarker *.
> 
> SetColor takes a JSTracer, because thats what the API exposes to XPConnect, so
> in there I am going to upcast unsafely to a GCMarker?

You can safely upcast since at the moment of the call 

trc == trc->context->runtime->gcMarkingTracer.

Asserting that and doing upcast is completely safe.

In one of the patches in my queue I already have GCMarker to move some of the marking only state there.

Comment 67

[Igor Bukanov]

(In reply to comment #66)
> You can safely upcast since at the moment of the call 
> 
> trc == trc->context->runtime->gcMarkingTracer.
> 
> Asserting that and doing upcast is completely safe.
> 
> In one of the patches in my queue I already have GCMarker to move some of the
> marking only state there.

I was wrong here - the code in question that traverses xpconnect gray
roots can be called with a non-gc tracer. But this points that
SetColor should only be called when IS_GC_MARKING_TRACER is true. This
also suggests to make SetColor a method of GCMarker

Comment 68

[Andreas Gal :gal]

Igor, I really have no time for another 10 iterations over this. I want to land this patch. I will move color back into the runtime, and we can cutify this in a separate patch.

Comment 69

[Igor Bukanov]

(In reply to comment #68)
> Igor, I really have no time for another 10 iterations over this. I want to land
> this patch. 

OK, but you still needs IS_GC_MARKING_TRACER before SetColor calls no matter where the color will end up.

Comment 70

[Andreas Gal :gal]

Non-ADD_TO_CC values are being added to the CC graph after all. Debugging that at the moment and waiting for fatvals to land so I can land this patch.

Comment 71

[Andreas Gal :gal]

I went back to if (!ADD_TO_CC()) return NULL in ToParticipant as originally suggested by peterv. About to land this.

Comment 72

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/47300d15cd1c

Comment 73

[Andreas Gal :gal]

peterv, you suggested to UNROOT global objects in one central place instead of every time is causing a ton of problems all over the place. context flags are not properly maintained and overwritten. I have fixed nsJSEnvironment but other places seem to exist. I will remove that part of the patch as well and go back to the original version that passed try server.

For a patch that is this complex and hard to test, we should really reduce review nits to stuff that can't be done in follow-up patches.

Comment 74

[Brendan Eich [:brendan]]

That (comment 48, about JSOPTION_UNROOTED_GLOBAL) wasn't a review nit :-P.

/be

Comment 75

[Andreas Gal :gal]

Attachment: patch

set mDefaultOptions instead of doing ::JS_GetOptions every time. This hopefully explains the test failures (looked like the "strict" flag got stuck)

Comment 76

[Andreas Gal :gal]

Whatever it was, it wasn't strictly necessary for this patch I think. It was ugly before, we should have left it ugly here, and file a follow up to do it nicer.

Comment 77

[Andreas Gal :gal]

Attachment: make context first, then ask it for its options

Comment 78

[Peter Van der Beken [:peterv]]

(In reply to comment #73)
> peterv, you suggested to UNROOT global objects in one central place instead of
> every time

I did no such thing, I suppose this was directed at igor.

Comment 79

[Andreas Gal :gal]

The patch seems to stick, except that we are leaking now. Sigh! I have seen the tree leak before the patch, too, but only on macosx. Now we leak everywhere. This patch is really fighting me all the way :) I will see if I have to back out again or diagnose in place.

Comment 80

[Andreas Gal :gal]

re-landed: http://hg.mozilla.org/tracemonkey/rev/ef7f3e72ac08

this is leaking due to a pre-existing bug, mrbkap will fix

Comment 81

[Igor Bukanov]

During normal browsing when zoomimg on google maps I got that assert:

Assertion failure: JS_HAS_OPTION(acx, JSOPTION_UNROOTED_GLOBAL), at /home/igor/m/tm/js/src/xpconnect/src/xpcjsruntime.cpp:377

The backtrace is:

#8  0x0027bce1 in JS_Assert (s=0x287fe7c "JS_HAS_OPTION(acx, JSOPTION_UNROOTED_GLOBAL)", file=0x287fdc0 "/home/igor/m/tm/js/src/xpconnect/src/xpcjsruntime.cpp", ln=377) at /home/igor/m/tm/js/src/jsutil.cpp:84
#9  0x01c2fdab in XPCJSRuntime::TraceXPConnectRoots (this=0xf5b3b0f0, trc=0xfff33cbc) at /home/igor/m/tm/js/src/xpconnect/src/xpcjsruntime.cpp:377
#10 0x01c2fc8f in XPCJSRuntime::TraceJS (trc=0xfff33cbc, data=0xf5b3b0f0) at /home/igor/m/tm/js/src/xpconnect/src/xpcjsruntime.cpp:341
#11 0x0019cf49 in js_TraceRuntime (trc=0xfff33cbc) at /home/igor/m/tm/js/src/jsgc.cpp:2577
#12 0x0019d7b0 in GC (cx=0xf3231800) at /home/igor/m/tm/js/src/jsgc.cpp:3116
#13 0x0019e0a1 in GCUntilDone (cx=0xf3231800, gckind=GC_NORMAL) at /home/igor/m/tm/js/src/jsgc.cpp:3491
#14 0x0019e1f5 in js_GC (cx=0xf3231800, gckind=GC_NORMAL) at /home/igor/m/tm/js/src/jsgc.cpp:3545
#15 0x001397b5 in JS_GC (cx=0xf3231800) at /home/igor/m/tm/js/src/jsapi.cpp:2433
#16 0x01bfe17a in nsXPConnect::Collect (this=0xf670cd30) at /home/igor/m/tm/js/src/xpconnect/src/nsXPConnect.cpp:402
#17 0x01bfe1ca in nsXPConnect::GarbageCollect (this=0xf670cd30) at /home/igor/m/tm/js/src/xpconnect/src/nsXPConnect.cpp:410
#18 0x01752750 in nsJSContext::CC () at /home/igor/m/tm/dom/base/nsJSEnvironment.cpp:3613
#19 0x01752949 in nsJSContext::IntervalCC () at /home/igor/m/tm/dom/base/nsJSEnvironment.cpp:3702
#20 0x017528bb in nsJSContext::MaybeCC (aHigherProbability=1) at /home/igor/m/tm/dom/base/nsJSEnvironment.cpp:3680
#21 0x017528f0 in nsJSContext::CCIfUserInactive () at /home/igor/m/tm/dom/base/nsJSEnvironment.cpp:3690
#22 0x017529be in GCTimerFired (aTimer=0xe19deac0, aClosure=0x0) at /home/igor/m/tm/dom/base/nsJSEnvironment.cpp:3728
#23 0x0231cd60 in nsTimerImpl::Fire (this=0xe19deac0) at /home/igor/m/tm/xpcom/threads/nsTimerImpl.cpp:427
#24 0x0231cf97 in nsTimerEvent::Run (this=0xd2086560) at /home/igor/m/tm/xpcom/threads/nsTimerImpl.cpp:519
#25 0x023162c2 in nsThread::ProcessNextEvent (this=0xf754a470, mayWait=0, result=0xfff3404c) at /home/igor/m/tm/xpcom/threads/nsThread.cpp:547
#26 0x022a9989 in NS_ProcessNextEvent_P (thread=0xf754a470, mayWait=0) at nsThreadUtils.cpp:250
#27 0x021859ee in mozilla::ipc::MessagePump::Run (this=0xf755e160, aDelegate=0xf7521520) at /home/igor/m/tm/ipc/glue/MessagePump.cpp:118
#28 0x0237c17d in MessageLoop::RunInternal (this=0xf7521520) at /home/igor/m/tm/ipc/chromium/src/base/message_loop.cc:219
#29 0x0237c0fd in MessageLoop::RunHandler (this=0xf7521520) at /home/igor/m/tm/ipc/chromium/src/base/message_loop.cc:202
#30 0x0237c0a1 in MessageLoop::Run (this=0xf7521520) at /home/igor/m/tm/ipc/chromium/src/base/message_loop.cc:176
#31 0x0206e066 in nsBaseAppShell::Run (this=0xf41d20b0) at /home/igor/m/tm/widget/src/xpwidgets/nsBaseAppShell.cpp:175
#32 0x01df1d2d in nsAppStartup::Run (this=0xf3f4d0d0) at /home/igor/m/tm/toolkit/components/startup/src/nsAppStartup.cpp:192
#33 0x00f38683 in XRE_main (argc=2, argv=0xfff34744, aAppData=0xf750e380) at /home/igor/m/tm/toolkit/xre/nsAppRunner.cpp:3619
#34 0x08049847 in main (argc=2, argv=0xfff34744) at /home/igor/m/tm/browser/app/nsBrowserApp.cpp:158
(gdb) fr 8
#8  0x0027bce1 in JS_Assert (s=0x287fe7c "JS_HAS_OPTION(acx, JSOPTION_UNROOTED_GLOBAL)", file=0x287fdc0 "/home/igor/m/tm/js/src/xpconnect/src/xpcjsruntime.cpp", ln=377) at /home/igor/m/tm/js/src/jsutil.cpp:84

Comment 82

[Igor Bukanov]

The above could come from http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpccomponents.cpp#3449.

Comment 83

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/47300d15cd1c