Factor the nsCycleCollectionJSRuntime implementation out of XPConnect so it can be shared between workers and the main thread.

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
3 months ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments)

16.74 KB, patch
mccr8
: review+
bholley
: superreview+
Details | Diff | Splinter Review
10.17 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
24.93 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
5.04 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
30.71 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
38.44 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
8.94 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
11.11 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
20.79 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
11.55 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
4.84 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
124.30 KB, patch
Details | Diff | Splinter Review
Big patch queue coming up, I suggest you read all the descriptions before beginning to review.
We have to leave mJSHolders protected for the moment so that XPCJSRuntime can poke at it, but that will be fixed later.
Attachment #761454 - Flags: review?(continuation)
PConnect has unrooted globals but workers do not, so add a flag so we can check that we get what we expect.
Attachment #761460 - Flags: review?(continuation)
This factors out much of nsCycleCollectionJSRuntime, including the notification functions for the various threads, NeedCollect, FixWeakMappingGrayBits, BeginCycleCollection, and traversal of JS.
Attachment #761462 - Flags: review?(continuation)
This is pretty straightforward.  We need hooks on the runtime to correctly handle a couple XPConnect specific cases that won't apply to workers.
Attachment #761465 - Flags: review?(continuation)
Get rid of nsCycleCollectionJSRuntime, teach the cycle collector to use mozilla::CycleCollectedJSRuntime, and move the registered runtime object from nsXPConnect to XPCJSRuntime.
Attachment #761474 - Flags: review?(continuation)
There are some other loose ends to tie up such as some of the callbacks and deferred finalization, but I'm going to do that in other bugs.
Duplicate of this bug: 768099
Comment on attachment 761452 [details] [diff] [review]
Part 1 - Create a mozilla::CycleCollectedJSRuntime class and move XPCJSRuntime's JSRuntime into it

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2906,5 @@
>  #ifdef DEBUG
>      depth--;
>      XPC_LOG_ALWAYS(("XPCJSRuntime @ %x", this));
>          XPC_LOG_INDENT();
> +        XPC_LOG_ALWAYS(("mJSRuntime @ %x", Runtime()));

Seems like this makes it sound like mJSRuntime is an XPCJSRuntime member still.

::: js/xpconnect/src/xpcprivate.h
@@ +109,5 @@
>  #include "nsAutoPtr.h"
>  #include "nsCycleCollectionParticipant.h"
>  #include "nsCycleCollectionJSRuntime.h"
>  #include "nsCycleCollectorUtils.h"
> +#include "mozilla/JSRuntime.h"

(I thought this list looked sorted at first, but if it was, not terribly well.)

::: xpcom/base/JSRuntime.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_CycleCollectedJSRuntime_h__
> +#define mozilla_CycleCollectedJSRuntime_h__

No trailing underscores, please

@@ +8,5 @@
> +#include "jsapi.h"
> +
> +namespace mozilla {
> +
> +class CycleCollectedJSRuntime {

{ on the next line

@@ +14,5 @@
> +  CycleCollectedJSRuntime(uint32_t aMaxbytes,
> +                          JSUseHelperThreads aUseHelperThreads);
> +  virtual ~CycleCollectedJSRuntime();
> +
> +  JSRuntime* Runtime() const

Why make this protected?
Comment on attachment 761457 [details] [diff] [review]
Part 3 - Replace GetJSRuntime() with just Runtime().

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2747,5 @@
>  
>      XPCJSRuntime* self = new XPCJSRuntime(aXPConnect);
>  
>      if (self                                    &&
> +        self->Runtime()                    &&

Indentation, but this isn't necessary anymore as you already crash in the constructor.
Comment on attachment 761460 [details] [diff] [review]
Part 4 - Push tracing of global objects down into moz illa::CycleCollectedJSRuntime.

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

::: xpcom/base/JSRuntime.cpp
@@ +34,5 @@
>  
> +void
> +CycleCollectedJSRuntime::MaybeTraceGlobals(JSTracer* aTracer) const
> +{
> +  JSContext *iter = nullptr;

* to the left, please (three times)

@@ +42,5 @@
> +      continue;
> +    }
> +
> +    if (JSObject *global = js::GetDefaultGlobalForContext(acx)) {
> +      JS_CallObjectTracer(aTracer, &global, "Global Object");

I wonder if we need to assert that the global object wasn't moved?
Comment on attachment 761462 [details] [diff] [review]
Part 5 - Move most of nsCycleCollectionJSRuntime's im plementation into mozilla::CycleCollectedJSRuntime.

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

::: xpcom/base/JSRuntime.cpp
@@ +18,5 @@
> +}
> +
> +struct NoteWeakMapChildrenTracer : public JSTracer
> +{
> +  NoteWeakMapChildrenTracer(nsCycleCollectionNoteRootCallback &cb)

& to the left / aCb? Also below

@@ +19,5 @@
> +
> +struct NoteWeakMapChildrenTracer : public JSTracer
> +{
> +  NoteWeakMapChildrenTracer(nsCycleCollectionNoteRootCallback &cb)
> +      : mCb(cb)

Indentation

@@ +170,5 @@
> +    if (v && xpc_IsGrayGCThing(v) &&
> +        (!k || !xpc_IsGrayGCThing(k)) &&
> +        (!m || !xpc_IsGrayGCThing(m)) &&
> +        vkind != JSTRACE_SHAPE)
> +    {

{-on-next-line is a jsism that isn't really used outside js/

@@ +329,5 @@
> +
> +  // NB: This is here just to preserve the existing XPConnect order. I doubt it
> +  // would hurt to do this after the JS holders.
> +  TraverseAdditionalNativeRoots(aCb);
> +  

Whitespace

@@ +391,5 @@
> +// static
> +nsCycleCollectionParticipant*
> +CycleCollectedJSRuntime::JSContextParticipant()
> +{
> +    return JSContext_cycleCollectorGlobal.GetParticipant();

Indentation

@@ +398,5 @@
> +bool
> +CycleCollectedJSRuntime::NotifyLeaveMainThread() const
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (JS_IsInRequest(mJSRuntime))

{}

@@ +399,5 @@
> +CycleCollectedJSRuntime::NotifyLeaveMainThread() const
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (JS_IsInRequest(mJSRuntime))
> +      return false;

Indentation

@@ +421,5 @@
> +
> +void
> +CycleCollectedJSRuntime::NotifyEnterMainThread() const
> +{
> +  MOZ_ASSERT(NS_IsMainThread(),);

Why the comma?

@@ +432,5 @@
> +  static bool gcHasRun = false;
> +  if (!gcHasRun) {
> +    uint32_t gcNumber = JS_GetGCParameter(mJSRuntime, JSGC_NUMBER);
> +    if (!gcNumber)
> +        NS_RUNTIMEABORT("Cannot cycle collect if GC has not run first!");

Indentation, {}, and MOZ_CRASH?

@@ +441,5 @@
> +
> +  NoteWeakMapsTracer trc(mJSRuntime, TraceWeakMapping, aCb);
> +  js::TraceWeakMaps(&trc);
> +
> +  return NS_OK;

(Does this need to return nsresult?)
Comment on attachment 761465 [details] [diff] [review]
Part 6 - Move the runtime and zone CC participants in to mozilla::CycleCollectedJSRuntime.

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2747,5 @@
>      return true;
>  }
>  
>  bool
> +XPCJSRuntime::DescribeCustomObjects(JSObject* obj, js::Class* clasp,

aFoo

@@ +2750,5 @@
>  bool
> +XPCJSRuntime::DescribeCustomObjects(JSObject* obj, js::Class* clasp,
> +                                    char (&name)[72]) const
> +{
> +    XPCNativeScriptableInfo *si = nullptr;

* to the left

@@ +2758,5 @@
> +    }
> +
> +    XPCWrappedNativeProto *p =
> +        static_cast<XPCWrappedNativeProto*>(xpc_GetJSPrivate(obj));
> +    si = p->GetScriptableInfo();

You can declare si here

@@ +2759,5 @@
> +
> +    XPCWrappedNativeProto *p =
> +        static_cast<XPCWrappedNativeProto*>(xpc_GetJSPrivate(obj));
> +    si = p->GetScriptableInfo();
> +    

Whitespace

::: xpcom/base/JSRuntime.cpp
@@ +304,5 @@
> +{
> +  TraversalTracer(nsCycleCollectionTraversalCallback &aCb) : cb(aCb)
> +  {
> +  }
> +  nsCycleCollectionTraversalCallback &cb;

mCb

@@ +308,5 @@
> +  nsCycleCollectionTraversalCallback &cb;
> +};
> +
> +static void
> +NoteJSChild(JSTracer* aTrc, void *aThing, JSGCTraceKind aTraceKind)

* to the left, throughout

@@ +327,5 @@
> +   * parent pointers iteratively, rather than recursively, to avoid overflow.
> +   */
> +if (AddToCCKind(aTraceKind)) {
> +    if (MOZ_UNLIKELY(tracer->cb.WantDebugInfo())) {
> +      // based on DumpNotify in jsapi.c

jsapi.c, eh

@@ +401,5 @@
>                                                   JSUseHelperThreads aUseHelperThreads,
>                                                   bool aExpectUnrootedGlobals)
> +  : mRuntimeCycleCollectorGlobal(sRuntimeCycleCollectorGlobal),
> +    mJSZoneCycleCollectorGlobal(sJSZoneCycleCollectorGlobal),
> +    mJSRuntime(nullptr)

I think you know I prefer putting the comma in front (and the ifdef below shows why)

@@ +451,5 @@
> +  char name[72];
> +  if (aTraceKind == JSTRACE_OBJECT) {
> +    JSObject *obj = static_cast<JSObject*>(aThing);
> +    js::Class *clasp = js::GetObjectClass(obj);
> +    

Whitespace

@@ +459,5 @@
> +    } else if (clasp == &js::FunctionClass) {
> +      JSFunction *fun = JS_GetObjectFunction(obj);
> +      JSString *str = JS_GetFunctionDisplayId(fun);
> +      if (str) {
> +          NS_ConvertUTF16toUTF8 fname(JS_GetInternedStringChars(str));

Indentation

@@ +480,5 @@
> +      "Shape",
> +      "BaseShape",
> +      "TypeObject",
> +    };
> +    JS_STATIC_ASSERT(NS_ARRAY_LENGTH(trace_types) == JSTRACE_LAST + 1);

MOZ_STATIC_ASSERT, please

@@ +560,5 @@
> +    NoteGCThingXPCOMChildren(js::GetObjectClass(obj), obj, aCb);
> +  }
> +}
> +
> +struct TraverseObjectShimClosure {

{ to the next line

::: xpcom/base/JSRuntime.h
@@ +111,5 @@
> +
> +
> +  enum TraverseSelect {
> +      TRAVERSE_CPP,
> +      TRAVERSE_FULL

Indentation
Comment on attachment 761467 [details] [diff] [review]
Part 7 - Move tracing of gray roots into mozilla::Cyc leCollectedJSRuntime.

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

::: xpcom/base/JSRuntime.cpp
@@ +652,5 @@
>  
> +/* static */ void
> +CycleCollectedJSRuntime::TraceGrayJS(JSTracer* aTracer, void* aData)
> +{
> +  CycleCollectedJSRuntime* self = (CycleCollectedJSRuntime*)aData;

static_cast please

@@ +660,5 @@
> +}
> +
> +struct JsGcTracer : public TraceCallbacks
> +{
> +  virtual void Trace(JS::Value *p, const char *name, void *closure) const MOZ_OVERRIDE {

* / aFoo / { on the next line
Comment on attachment 761468 [details] [diff] [review]
Part 8 - Move the rest of nsCycleCollectionJSRuntime' s implementation into mozilla::CycleCollectedJSRuntime.

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

::: xpcom/base/JSRuntime.cpp
@@ +845,5 @@
> +    MOZ_ASSERT(!js::GetObjectParent(global));
> +    if (JS::GCThingIsMarkedGray(global) &&
> +        !js::IsSystemCompartment(js::GetObjectCompartment(global))) {
> +      return true;
> +      return true;

Double?

@@ +911,5 @@
> +  // To improve debugging, if WantAllTraces() is true all JS objects are
> +  // traversed.
> +
> +  MOZ_ASSERT(aReason < JS::gcreason::NUM_REASONS);
> +  JS::gcreason::Reason gcreason = (JS::gcreason::Reason)aReason;

static_cast
Comment on attachment 761476 [details] [diff] [review]
Part 10 - Add a hook for tracing black JS and update the big comment to more accurately describe how this works.

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

::: js/xpconnect/src/xpcprivate.h
@@ +771,2 @@
>      void TraverseAdditionalNativeRoots(nsCycleCollectionNoteRootCallback& cb);
> +    void TraceAdditionalNativeGrayRoots(JSTracer* aTracer);

* to the right here

::: xpcom/base/JSRuntime.cpp
@@ +652,5 @@
>  
>  /* static */ void
> +CycleCollectedJSRuntime::TraceBlackJS(JSTracer* aTracer, void* aData)
> +{
> +  CycleCollectedJSRuntime* self = (CycleCollectedJSRuntime*)aData;

static_cast
Comment on attachment 761479 [details] [diff] [review]
Part 11 - Push the final uses of mJSHolders down into  mozilla::CycleCollectedJSRuntime and make it private.

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

::: xpcom/base/JSRuntime.cpp
@@ +428,5 @@
> +CycleCollectedJSRuntime::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const
> +{
> +  size_t n = 0;
> +
> +  // NULL for the second arg;  we're not measuring anything hanging off the

second arg?
Could you attach a combined patch, too?  That might help me get oriented a bit more.  Thanks.
Comment on attachment 761452 [details] [diff] [review]
Part 1 - Create a mozilla::CycleCollectedJSRuntime class and move XPCJSRuntime's JSRuntime into it

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

bholley sr+ or whatever moving mJSRuntime out of XPCJSRuntime into this other new class, as it affects XPConnect, and the general idea of having this new superclass.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1409,5 @@
>  #endif
>          delete mDetachedWrappedNativeProtoMap;
>      }
>  
> +    JS_ShutDown();

You are changing the behavior here.  The destructor for the parent class won't run until after the destructor for XPCJSRuntime, so we will run JS_ShutDown() before the runtime is destroyed.  Maybe JS_ShutDown() should be moved to the parent classes destructor?  I have no idea what it actually does...

@@ -1412,5 @@
>  
> -    if (mJSRuntime) {
> -        JS_DestroyRuntime(mJSRuntime);
> -        JS_ShutDown();
> -#ifdef DEBUG_shaver_off

Do we need shaver to r+ this removal? ;)

@@ +2906,5 @@
>  #ifdef DEBUG
>      depth--;
>      XPC_LOG_ALWAYS(("XPCJSRuntime @ %x", this));
>          XPC_LOG_INDENT();
> +        XPC_LOG_ALWAYS(("mJSRuntime @ %x", Runtime()));

Ms2ger commented on this, but I think it is okay to just leave it as mJSRuntime.  Or you could change it to Runtime().

::: xpcom/base/JSRuntime.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Add the mode line to the top of this file please.
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style?redirectlocale=en-US&redirectslug=Mozilla_Coding_Style_Guide#Mode_Line

@@ +6,5 @@
> +
> +using namespace mozilla;
> +
> +CycleCollectedJSRuntime::CycleCollectedJSRuntime(uint32_t aMaxbytes,
> +                                                 JSUseHelperThreads aUseHelperThreads)

It looks like it would make more sense to just pass in a JSRuntime* here, but either way is okay I guess.

::: xpcom/base/JSRuntime.h
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Add the mode line at the top of the file, please.

@@ +6,5 @@
> +#define mozilla_CycleCollectedJSRuntime_h__
> +
> +#include "jsapi.h"
> +
> +namespace mozilla {

This file should really be called something like CycleCollectedJSRuntime.{h, cpp}, as it only has to do with the CC.  You should be able to do that rename by running sed on the patches or something...
Attachment #761452 - Flags: review?(continuation) → review+
Attachment #761454 - Flags: review?(continuation) → review+
Comment on attachment 761457 [details] [diff] [review]
Part 3 - Replace GetJSRuntime() with just Runtime().

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2747,5 @@
>  
>      XPCJSRuntime* self = new XPCJSRuntime(aXPConnect);
>  
>      if (self                                    &&
> +        self->Runtime()                    &&

As Ms2ger said, you don't need this any more.  And your indentation is wrong.  And the portions are too small.

::: js/xpconnect/src/xpcprivate.h
@@ +656,5 @@
>  public:
>      static XPCJSRuntime* newXPCJSRuntime(nsXPConnect* aXPConnect);
>      static XPCJSRuntime* Get() { return nsXPConnect::XPConnect()->GetRuntime(); }
>  
> +    // Make this public for now.  Ideally we'd hide the JSRuntime inside.

Eh, I don't think this comment is really true.  Doing that would just require moving all the things that call this inside XPCJSRuntime. :)

@@ +657,5 @@
>      static XPCJSRuntime* newXPCJSRuntime(nsXPConnect* aXPConnect);
>      static XPCJSRuntime* Get() { return nsXPConnect::XPConnect()->GetRuntime(); }
>  
> +    // Make this public for now.  Ideally we'd hide the JSRuntime inside.
> +    JSRuntime* Runtime() const

This naming will come back to haunt us when we implement a Ruby runtime!
Attachment #761457 - Flags: review?(continuation) → review+
Comment on attachment 761460 [details] [diff] [review]
Part 4 - Push tracing of global objects down into moz illa::CycleCollectedJSRuntime.

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

r=me with comments addressed

::: xpcom/base/JSRuntime.cpp
@@ +34,5 @@
>  
> +void
> +CycleCollectedJSRuntime::MaybeTraceGlobals(JSTracer* aTracer) const
> +{
> +  JSContext *iter = nullptr;

* to the left nits, as Ms2ger said.

@@ +37,5 @@
> +{
> +  JSContext *iter = nullptr;
> +  while (JSContext *acx = JS_ContextIterator(Runtime(), &iter)) {
> +    MOZ_ASSERT(js::HasUnrootedGlobal(acx) == mExpectUnrootedGlobals);
> +    if (!js::HasUnrootedGlobal(acx)) {

I guess in workers globals are just rooted through some other mechanism?  Do workers going to have a mix of rooted and unrooted globals?
Attachment #761460 - Flags: review?(continuation) → review+
Comment on attachment 761462 [details] [diff] [review]
Part 5 - Move most of nsCycleCollectionJSRuntime's im plementation into mozilla::CycleCollectedJSRuntime.

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

I'm going to assume you copied the WeakMap code over properly, as every time I read that code it etches away my sanity.

Please also address Ms2ger's comments on this patch except where noted.

r=me with that

::: xpcom/base/JSRuntime.cpp
@@ +170,5 @@
> +    if (v && xpc_IsGrayGCThing(v) &&
> +        (!k || !xpc_IsGrayGCThing(k)) &&
> +        (!m || !xpc_IsGrayGCThing(m)) &&
> +        vkind != JSTRACE_SHAPE)
> +    {

Ms2ger complained about the bracing here, but I think it is fine to leave as is.

@@ +304,5 @@
>  void
> +CycleCollectedJSRuntime::MaybeTraverseGlobals(nsCycleCollectionNoteRootCallback& aCb) const
> +{
> +  JSContext *iter = nullptr, *acx;
> +  while ((acx = JS_ContextIterator(Runtime(), &iter))) {

Kind of weird this doesn't need that check for expecting whether things are rooted or not, but okay.

@@ +317,5 @@
> +
> +void
> +CycleCollectedJSRuntime::TraverseNativeRoots(nsCycleCollectionNoteRootCallback& aCb)
> +{
> +  // For all JS objects that are held by native objects but aren't held

Please move this comment up to in front of MaybeTraverseGlobals.

@@ +432,5 @@
> +  static bool gcHasRun = false;
> +  if (!gcHasRun) {
> +    uint32_t gcNumber = JS_GetGCParameter(mJSRuntime, JSGC_NUMBER);
> +    if (!gcNumber)
> +        NS_RUNTIMEABORT("Cannot cycle collect if GC has not run first!");

NS_RUNTIMEABORT is fine...

@@ +441,5 @@
> +
> +  NoteWeakMapsTracer trc(mJSRuntime, TraceWeakMapping, aCb);
> +  js::TraceWeakMaps(&trc);
> +
> +  return NS_OK;

Ms2ger remarked that this doesn't really need to return a result, but this is existing code so it is okay to leave here.  He can file a followup bug if he cares. ;)
Attachment #761462 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #25)
> Comment on attachment 761452 [details] [diff] [review]
> ::: js/xpconnect/src/XPCJSRuntime.cpp
> @@ +1409,5 @@
> >  #endif
> >          delete mDetachedWrappedNativeProtoMap;
> >      }
> >  
> > +    JS_ShutDown();
> 
> You are changing the behavior here.  The destructor for the parent class
> won't run until after the destructor for XPCJSRuntime, so we will run
> JS_ShutDown() before the runtime is destroyed.  Maybe JS_ShutDown() should
> be moved to the parent classes destructor?  I have no idea what it actually
> does...

JS_ShutDown just shuts down some PRMJ time stuff.  I don't think the order makes a difference.
Attachment #761452 - Flags: superreview?(bobbyholley+bmo)
Comment on attachment 761452 [details] [diff] [review]
Part 1 - Create a mozilla::CycleCollectedJSRuntime class and move XPCJSRuntime's JSRuntime into it

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

sr=bholley on the superclass.
Attachment #761452 - Flags: superreview?(bobbyholley+bmo) → superreview+
Comment on attachment 761465 [details] [diff] [review]
Part 6 - Move the runtime and zone CC participants in to mozilla::CycleCollectedJSRuntime.

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

Very nice to have this out of XPConnect!

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2747,5 @@
>      return true;
>  }
>  
>  bool
> +XPCJSRuntime::DescribeCustomObjects(JSObject* obj, js::Class* clasp,

JS doesn't use aFoo style, so ignore Ms2ger here. ;)

@@ +2748,5 @@
>  }
>  
>  bool
> +XPCJSRuntime::DescribeCustomObjects(JSObject* obj, js::Class* clasp,
> +                                    char (&name)[72]) const

Is passing a reference to a fixed size buffer really a generally supported thing?  I guess you are using MSVC, so if it works there it probably works everywhere. ;)

@@ +2750,5 @@
>  bool
> +XPCJSRuntime::DescribeCustomObjects(JSObject* obj, js::Class* clasp,
> +                                    char (&name)[72]) const
> +{
> +    XPCNativeScriptableInfo *si = nullptr;

Ms2ger had various suggestions for this method.

::: xpcom/base/JSRuntime.cpp
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/JSRuntime.h"
> +#include "mozilla/dom/DOMJSClass.h"
> +#include "mozilla/dom/BindingUtils.h"

b is before d in the alphabet.  I'm sad Ms2ger didn't notice this.

@@ +265,5 @@
>  
>    return PL_DHASH_NEXT;
>  }
>  
> +NS_METHOD

// static

@@ +266,5 @@
>    return PL_DHASH_NEXT;
>  }
>  
> +NS_METHOD
> +CCRuntimeParticipant::TraverseImpl(CCRuntimeParticipant *that, void *p,

* to the left

@@ +271,5 @@
> +                                   nsCycleCollectionTraversalCallback &cb)
> +{
> +  CycleCollectedJSRuntime* runtime = reinterpret_cast<CycleCollectedJSRuntime*>
> +    (reinterpret_cast<char*>(that) -
> +     offsetof(CycleCollectedJSRuntime, mRuntimeCycleCollectorGlobal));

This is gross, but other people seem less horrified by this than me, so I guess it is okay.  I may file a bug about allowing multiple instances of a participant so we could just add a field to the participant.

@@ +279,5 @@
> +  return NS_OK;
> +}
> +
> +static const CCParticipantVTable<CCRuntimeParticipant>::Type
> +sRuntimeCycleCollectorGlobal =

Please add a comment that this should only be used to initialize the global field in CycleCollectedJSRuntime.

@@ +285,5 @@
> +  NS_IMPL_CYCLE_COLLECTION_NATIVE_VTABLE(CCRuntimeParticipant)
> +};
> +
> +NS_METHOD
> +JSZoneParticipant::TraverseImpl(JSZoneParticipant *that, void *p,

* to the left

@@ +301,5 @@
> +}
> +
> +struct TraversalTracer : public JSTracer
> +{
> +  TraversalTracer(nsCycleCollectionTraversalCallback &aCb) : cb(aCb)

& to the left

@@ +304,5 @@
> +{
> +  TraversalTracer(nsCycleCollectionTraversalCallback &aCb) : cb(aCb)
> +  {
> +  }
> +  nsCycleCollectionTraversalCallback &cb;

& to the left, and mCb, as Ms2ger said.

@@ +385,5 @@
> + * cycle.)
> + */
> +
> +static const CCParticipantVTable<JSZoneParticipant>::Type
> +sJSZoneCycleCollectorGlobal = {

Please add a comment that this should only be used to initialize the global field in CycleCollectedJSRuntime.

@@ +438,5 @@
>    }
>  }
>  
>  void
> +CycleCollectedJSRuntime::DescribeGCThing(bool aIsMarked, void *aThing,

* to the left throughout this function

@@ +451,5 @@
> +  char name[72];
> +  if (aTraceKind == JSTRACE_OBJECT) {
> +    JSObject *obj = static_cast<JSObject*>(aThing);
> +    js::Class *clasp = js::GetObjectClass(obj);
> +    

whitespace, as Ms2ger said

@@ +459,5 @@
> +    } else if (clasp == &js::FunctionClass) {
> +      JSFunction *fun = JS_GetObjectFunction(obj);
> +      JSString *str = JS_GetFunctionDisplayId(fun);
> +      if (str) {
> +          NS_ConvertUTF16toUTF8 fname(JS_GetInternedStringChars(str));

Indentation, as Ms2ger said

@@ +507,5 @@
> +{
> +  MOZ_ASSERT(aClasp);
> +  MOZ_ASSERT(aClasp == js::GetObjectClass(aObj));
> +
> +  if (NoteCustomGCThingXPCOMChildren(aClasp, aObj, aCb)) {

Early return please.

::: xpcom/base/JSRuntime.h
@@ +16,5 @@
>  class nsScriptObjectTracer;
>  
>  namespace mozilla {
>  
> +class CCRuntimeParticipant: public nsCycleCollectionParticipant

Please rename this to something more useful, like JSGCThingParticipant.  I know the old name was terrible, but we should fix that. :)

@@ +19,5 @@
>  
> +class CCRuntimeParticipant: public nsCycleCollectionParticipant
> +{
> +public:
> +  static NS_METHOD RootImpl(void *n)

nit: I think this should be * to the left here?  Many places in the two participant classes.  They should also be aFoo-style names, but I don't really care about that...

@@ +141,5 @@
>  
>    // This returns the singleton nsCycleCollectionParticipant for JSContexts.
> +  static nsCycleCollectionParticipant* JSContextParticipant();
> +
> +  nsCycleCollectionParticipant* Participant() const;

Please name this GCThingParticipant or something matching whatever you change the CCRuntimeParticipant to above.

@@ +157,5 @@
>  protected:
>    nsDataHashtable<nsPtrHashKey<void>, nsScriptObjectTracer*> mJSHolders;
>  
>  private:
> +  typedef const CCParticipantVTable<CCRuntimeParticipant>::Type RuntimeParticipantVTable;

rename

@@ +158,5 @@
>    nsDataHashtable<nsPtrHashKey<void>, nsScriptObjectTracer*> mJSHolders;
>  
>  private:
> +  typedef const CCParticipantVTable<CCRuntimeParticipant>::Type RuntimeParticipantVTable;
> +  const RuntimeParticipantVTable mRuntimeCycleCollectorGlobal;

rename
Attachment #761465 - Flags: review?(continuation) → review+
Comment on attachment 761467 [details] [diff] [review]
Part 7 - Move tracing of gray roots into mozilla::Cyc leCollectedJSRuntime.

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

::: xpcom/base/JSRuntime.cpp
@@ +645,4 @@
>    // NB: This is here just to preserve the existing XPConnect order. I doubt it
>    // would hurt to do this after the JS holders.
>    TraverseAdditionalNativeRoots(aCb);
>    

whitespace, though I suppose I probably already complained about this in a prior patch.

@@ +652,5 @@
>  
> +/* static */ void
> +CycleCollectedJSRuntime::TraceGrayJS(JSTracer* aTracer, void* aData)
> +{
> +  CycleCollectedJSRuntime* self = (CycleCollectedJSRuntime*)aData;

static_cast, as Ms2ger said.
Attachment #761467 - Flags: review?(continuation) → review+
Comment on attachment 761468 [details] [diff] [review]
Part 8 - Move the rest of nsCycleCollectionJSRuntime' s implementation into mozilla::CycleCollectedJSRuntime.

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

::: xpcom/base/JSRuntime.cpp
@@ +844,5 @@
> +    global = JS_ObjectToInnerObject(cx, global);
> +    MOZ_ASSERT(!js::GetObjectParent(global));
> +    if (JS::GCThingIsMarkedGray(global) &&
> +        !js::IsSystemCompartment(js::GetObjectCompartment(global))) {
> +      return true;

As Ms2ger pointed out, you are returning twice in a row...

@@ +867,5 @@
> +
> +void
> +CycleCollectedJSRuntime::Collect(uint32_t aReason) const
> +{
> +  // We're dividing JS objects into 2 categories:

Please just move this comment to the top of the file. This is still a useful general comment, but I think it was just sitting in Collect() for historical reasons, and really doesn't have anything to do with Collect() any more.

@@ +911,5 @@
> +  // To improve debugging, if WantAllTraces() is true all JS objects are
> +  // traversed.
> +
> +  MOZ_ASSERT(aReason < JS::gcreason::NUM_REASONS);
> +  JS::gcreason::Reason gcreason = (JS::gcreason::Reason)aReason;

static_cast, as Ms2ger said.
Attachment #761468 - Flags: review?(continuation) → review+
Comment on attachment 761474 [details] [diff] [review]
Part 9 - Kill nsCycleCollectionJSRuntime.

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

Huzzah!
Attachment #761474 - Flags: review?(continuation) → review+
Comment on attachment 761476 [details] [diff] [review]
Part 10 - Add a hook for tracing black JS and update the big comment to more accurately describe how this works.

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

::: js/xpconnect/src/xpcprivate.h
@@ +766,5 @@
>          NS_ASSERTION(index < IDX_TOTAL_COUNT, "index out of range");
>          return mStrings[index];
>      }
>  
> +    void TraceNativeBlackRoots(JSTracer* trc);

Please put the two Trace functions next to each other here, black first.

::: xpcom/base/JSRuntime.cpp
@@ +408,5 @@
>      MOZ_CRASH();
>    }
>  
>    JS_SetGrayGCRootsTracer(mJSRuntime, TraceGrayJS, this);
> +  JS_SetExtraGCRootsTracer(mJSRuntime, TraceBlackJS, this);

Please put this right before JS_SetGrayGCRootsTracer to keep things in conceptual harmony. ;)

@@ +652,5 @@
>  
>  /* static */ void
> +CycleCollectedJSRuntime::TraceBlackJS(JSTracer* aTracer, void* aData)
> +{
> +  CycleCollectedJSRuntime* self = (CycleCollectedJSRuntime*)aData;

per Ms2ger, static_cast please

@@ +663,5 @@
>  CycleCollectedJSRuntime::TraceGrayJS(JSTracer* aTracer, void* aData)
>  {
>    CycleCollectedJSRuntime* self = (CycleCollectedJSRuntime*)aData;
>  
>    // Mark these roots as gray so the CC can walk them later.

This comment is pretty pointless, you can just delete it.

@@ +875,5 @@
>  
>  void
>  CycleCollectedJSRuntime::Collect(uint32_t aReason) const
>  {
> +  // We're dividing JS objects into 3 categories:

Ah, I see you fixed up the comment here.  Awesome!  Well, please move it all out to the top at the end of the stack after you make changes to it.

@@ +889,3 @@
>    //
> +  // 3. all other roots held by C++ objects that participate in cycle
> +  //    collection, held by the us (see TraceNativeGrayRoots). Roots from this

"by the us"?

@@ +889,4 @@
>    //
> +  // 3. all other roots held by C++ objects that participate in cycle
> +  //    collection, held by the us (see TraceNativeGrayRoots). Roots from this
> +  //    category are considered grey in the cycle collector, their final color

"collector, their" should be something like "collector: their". Also, I think it would be better to say something more like "whether or not they are collected depends on the objects that hold them".  Better not to mention CC colors here, which are totally different, and yet named the same... ;)

@@ +891,5 @@
> +  //    collection, held by the us (see TraceNativeGrayRoots). Roots from this
> +  //    category are considered grey in the cycle collector, their final color
> +  //    depends on the objects that hold them.
> +  //
> +  // Note that if a root is in multiple categories it is the fact that it is in

"it is the fact" should be something like "the fact"

::: xpcom/base/JSRuntime.h
@@ +138,3 @@
>    static void TraceGrayJS(JSTracer* aTracer, void* aData);
>  
> +  virtual void TraceNativeBlackRoots(JSTracer* aTracer) { };

Should this be |=0| rather than empty?
Attachment #761476 - Flags: review?(continuation) → review+
Comment on attachment 761479 [details] [diff] [review]
Part 11 - Push the final uses of mJSHolders down into  mozilla::CycleCollectedJSRuntime and make it private.

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

::: xpcom/base/JSRuntime.cpp
@@ +428,5 @@
> +CycleCollectedJSRuntime::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const
> +{
> +  size_t n = 0;
> +
> +  // NULL for the second arg;  we're not measuring anything hanging off the

As Ms2ger, this comment should be "first arg" not second.

@@ +436,5 @@
> +  return n;
> +}
> +
> +static PLDHashOperator
> +UnmarkJSHolder(void* holder, nsScriptObjectTracer*& tracer, void* arg)

nit: aHolder, aTracer, aArg.
Attachment #761479 - Flags: review?(continuation) → review+
Thanks!
Is XPC_DUMP_AT_SHUTDOWN still useful?
You need to log in before you can comment on or make changes to this bug.