720686 - Add bunch of methods to xpcpublic so that xpc stuff can be marked black/in-cc-generation

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

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

XPCVariant will need to be able to be marked in CC generation, and its
jsobj non-gray
nsXPCWrapppedJS needs clearing non-gray
Hopefully this all with as few virtual calls as possible.

Comment 1

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

Attachment: patch

This might be enough.
Does this look ok?
I can ask peterv or something to look at the XPCJSRuntime.cpp changes.

Comment 2

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

(It is possible I need some more methods in xpcpublic)

Comment 3

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

Attachment: better method name

Comment 5

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

Attachment: patch

QI aVariant, not variant. And don't add stuff to the purple buffer.

Comment 6

[Andrew McCreight [:mccr8]]

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

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

Looks good.  These are mostly minor comments.

r- only because I'm concerned about the RemovePurple in nsXPConnect that I comment on below.  Maybe I'm wrong there?

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +576,5 @@
> +    for (XPCRootSetElem *e = mVariantRoots; e ; e = e->GetNextRoot()) {
> +        XPCTraceableVariant* v = static_cast<XPCTraceableVariant*>(e);
> +        if (!cb.WantAllTraces() &&
> +            nsCCUncollectableMarker::InGeneration(cb,
> +                                                  v->CCGeneration())) {

This version of InGeneration checks WantAllTraces, so you don't need to manually check it here.

@@ +578,5 @@
> +        if (!cb.WantAllTraces() &&
> +            nsCCUncollectableMarker::InGeneration(cb,
> +                                                  v->CCGeneration())) {
> +           jsval val = v->GetJSValPreserveColor();
> +           if (val.isObject() && !xpc_IsGrayGCThing(&val.toObject()))

Are there any interesting cases where this might be something that isn't an object?  Like, a JS Script, or a JS XML (whatever it is called)?  You could change val.isObject() to val.isMarkable() and val.toObject() to val.toGCThing().

You do this in another patch here I've partially reviewed, I think.  You should probably add a version of xpc_IsGrayGCThing that takes a jsval or a jsval&, and is something like val.isMarkable() && xpc_IsGrayGCThing(&val.toGCThing())).  I'll double check with a JS person that this is the right thing to do.

Then this could become !xpc_IsGrayGCThing(val) or some such.

@@ +591,5 @@
> +        // If traversing wrappedJS wouldn't release it, nor
> +        // cause any other objects to be added to the graph, no
> +        // need to add it to the graph at all.
> +        if (nsCCUncollectableMarker::sGeneration &&
> +            !cb.WantAllTraces() && obj && !xpc_IsGrayGCThing(obj) &&

Could "obj && !xpc_IsGrayGCThing(obj)" be "(!obj || !xpc_IsGrayGCThing(obj))"?  If the thing being held is null, that's as good as if there's something and it is marked?

@@ +593,5 @@
> +        // need to add it to the graph at all.
> +        if (nsCCUncollectableMarker::sGeneration &&
> +            !cb.WantAllTraces() && obj && !xpc_IsGrayGCThing(obj) &&
> +            !wrappedJS->IsSubjectToFinalization() &&
> +            wrappedJS == wrappedJS->GetRootWrapper() &&

Maybe put wrappedJS on the right side of the == so it is less like an assignment.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +831,5 @@
> +        variant->GetJSVal(); // Unmarks gray JSObject.
> +        XPCVariant* weak = variant.get();
> +        variant = nsnull;
> +        if (weak->IsPurple()) {
> +          weak->RemovePurple();

What is the idea with the RemovePurple()?  Is to make it so the refcount traffic in here doesn't cause it to become purple?  This concerns me a little.  If |variant| is the last reference to the underlying object, this could cause a leak.  Are we guaranteed that the caller will still hold a reference to aVariant?

Comment 7

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

(In reply to Andrew McCreight [:mccr8] from comment #6)

> r- only because I'm concerned about the RemovePurple in nsXPConnect that I
> comment on below.  Maybe I'm wrong there?
> 


> This version of InGeneration checks WantAllTraces, so you don't need to
> manually check it here.
Indeed. I'm moving and changing this code too much


> 
> @@ +578,5 @@
> > +        if (!cb.WantAllTraces() &&
> > +            nsCCUncollectableMarker::InGeneration(cb,
> > +                                                  v->CCGeneration())) {
> > +           jsval val = v->GetJSValPreserveColor();
> > +           if (val.isObject() && !xpc_IsGrayGCThing(&val.toObject()))
> 
> Are there any interesting cases where this might be something that isn't an
> object?  Like, a JS Script, or a JS XML (whatever it is called)?  You could
> change val.isObject() to val.isMarkable() and val.toObject() to
> val.toGCThing().
JS XML and JS Script are very much edge cases


> 
> You do this in another patch here I've partially reviewed, I think.  You
> should probably add a version of xpc_IsGrayGCThing that takes a jsval or a
> jsval&, and is something like val.isMarkable() &&
> xpc_IsGrayGCThing(&val.toGCThing())).  I'll double check with a JS person
> that this is the right thing to do.
I'd rather make such change in a followup. I have enough patches to get landed ;)


> @@ +591,5 @@
> > +        // If traversing wrappedJS wouldn't release it, nor
> > +        // cause any other objects to be added to the graph, no
> > +        // need to add it to the graph at all.
> > +        if (nsCCUncollectableMarker::sGeneration &&
> > +            !cb.WantAllTraces() && obj && !xpc_IsGrayGCThing(obj) &&
> 
> Could "obj && !xpc_IsGrayGCThing(obj)" be "(!obj ||
> !xpc_IsGrayGCThing(obj))"?  If the thing being held is null, that's as good
> as if there's something and it is marked?
OK.


> 
> Maybe put wrappedJS on the right side of the == so it is less like an
> assignment.
Ok


> > @@ +831,5 @@
> > +        variant->GetJSVal(); // Unmarks gray JSObject.
> > +        XPCVariant* weak = variant.get();
> > +        variant = nsnull;
> > +        if (weak->IsPurple()) {
> > +          weak->RemovePurple();
> 
> What is the idea with the RemovePurple()?  Is to make it so the refcount
> traffic in here doesn't cause it to become purple? 
Yes.

> This concerns me a
> little.  If |variant| is the last reference to the underlying object, this
> could cause a leak. 
No it would not. This would cause a crash.

> Are we guaranteed that the caller will still hold a
> reference to aVariant?
Yes, because something is marking aVariant black!

Comment 8

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

Attachment: patch

didn't change anything for the JS XML _edge_case_
and unmarkPurple should be just ok. If someone is 
trying mark non-certainly-black variant black, better to crash soon :)

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #7)
> JS XML and JS Script are very much edge cases

There are a ton of JS Scripts.  But I guess you mean for variants they are an edge case. :)  I'm okay with leaving it for a followup.  I'll file a bug about it.

> > This concerns me a
> > little.  If |variant| is the last reference to the underlying object, this
> > could cause a leak. 
> No it would not. This would cause a crash.

Well, it could cause a leak or a crash.  It would cause a leak if the object was part of a garbage cycle, but a crash otherwise as you say (because the object would be freed).

> > Are we guaranteed that the caller will still hold a
> > reference to aVariant?
> Yes, because something is marking aVariant black!

Right, so I can imagine that in any place you are actually calling this it isn't a problem, but in general it is a little unnerving, as kind of a landmine waiting for future people.  I guess I'm okay with it if you put a comment saying that it should only be called with an aVariant that is known to be held alive by something or it will causes leaks or crashes.  Or something to that effect.

Comment 10

[Andrew McCreight [:mccr8]]

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

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

r=me with scary comment added.

::: js/xpconnect/src/xpcpublic.h
@@ +190,5 @@
>          xpc_UnmarkGrayObjectRecursive(obj);
>  }
>  
> +// If aVariant is an XPCVariant, this marks the object to be in aGeneration.
> +// This also unmarks the gray JSObject.

Add some kind of scary comment here as I referred as I describe in comment 9.

Comment 11

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

It is basic XPCOM rule that caller must keep the object alive. We rely on that everywhere.

Comment 12

[Andrew McCreight [:mccr8]]

Ah, okay!  Sounds like a good rule to have.  I'm okay with skipping the comment then.  Sorry for my confusion.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/ee2d438cdd77