Last Comment Bug 720686 - Add bunch of methods to xpcpublic so that xpc stuff can be marked black/in-cc-generation
: Add bunch of methods to xpcpublic so that xpc stuff can be marked black/in-cc...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla12
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
: 714642 (view as bug list)
Depends on:
Blocks: 705582 720536
  Show dependency treegraph
 
Reported: 2012-01-24 06:39 PST by Olli Pettay [:smaug] (vacation Aug 25-28)
Modified: 2012-01-26 13:20 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (7.10 KB, patch)
2012-01-24 07:25 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
better method name (7.10 KB, patch)
2012-01-24 07:43 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
mrbkap: feedback+
Details | Diff | Splinter Review
patch (7.24 KB, patch)
2012-01-25 06:46 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
continuation: review-
Details | Diff | Splinter Review
patch (7.21 KB, patch)
2012-01-25 08:42 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
continuation: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (vacation Aug 25-28) 2012-01-24 06:39:11 PST
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] (vacation Aug 25-28) 2012-01-24 07:25:39 PST
Created attachment 591084 [details] [diff] [review]
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] (vacation Aug 25-28) 2012-01-24 07:26:36 PST
(It is possible I need some more methods in xpcpublic)
Comment 3 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-01-24 07:43:47 PST
Created attachment 591089 [details] [diff] [review]
better method name
Comment 4 Andrew McCreight [:mccr8] 2012-01-24 10:02:28 PST
*** Bug 714642 has been marked as a duplicate of this bug. ***
Comment 5 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-01-25 06:46:22 PST
Created attachment 591449 [details] [diff] [review]
patch

QI aVariant, not variant. And don't add stuff to the purple buffer.
Comment 6 Andrew McCreight [:mccr8] 2012-01-25 08:27:50 PST
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] (vacation Aug 25-28) 2012-01-25 08:34:22 PST
(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] (vacation Aug 25-28) 2012-01-25 08:42:47 PST
Created attachment 591481 [details] [diff] [review]
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] 2012-01-25 08:54:42 PST
(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] 2012-01-25 08:55:45 PST
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] (vacation Aug 25-28) 2012-01-25 16:07:43 PST
It is basic XPCOM rule that caller must keep the object alive. We rely on that everywhere.
Comment 12 Andrew McCreight [:mccr8] 2012-01-25 16:08:38 PST
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] (vacation Aug 25-28) 2012-01-26 12:28:35 PST
https://hg.mozilla.org/mozilla-central/rev/ee2d438cdd77

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