877584 - Route JS holding through the cycle collection runtime

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This provides a more generic method for NS_HOLD/DROP_JS_OBJECTS to use than calling XPConnect directly.

Comment 1

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

Is the new stuff available to binary addons?

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 755837 [details] [diff] [review]
Patch

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

r=me with the comments addressed

::: xpcom/base/nsCycleCollector.cpp
@@ +2976,5 @@
>          collector->ForgetJSRuntime();
>  }
>  
> +void
> +cyclecollector::AddJSHolder(void* aHolder, nsScriptObjectTracer* aTracer)

Does this need to be mozilla::cyclecollector::AddJSHolder?

@@ +2980,5 @@
> +cyclecollector::AddJSHolder(void* aHolder, nsScriptObjectTracer* aTracer)
> +{
> +    nsCycleCollector *collector = sCollector.get();
> +
> +    MOZ_ASSERT(collector && collector != (nsCycleCollector*)1 &&

Please define some new constant like kShutdownSentinel or something and use that instead of |(nsCycleCollector*)1| here and elsewhere in the file.  You can delete the explanatory comments for |(nsCycleCollector*)1| at the same time if you like.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Comment on attachment 755837 [details] [diff] [review]
> Patch
> 
> Review of attachment 755837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the comments addressed
> 
> ::: xpcom/base/nsCycleCollector.cpp
> @@ +2976,5 @@
> >          collector->ForgetJSRuntime();
> >  }
> >  
> > +void
> > +cyclecollector::AddJSHolder(void* aHolder, nsScriptObjectTracer* aTracer)
> 
> Does this need to be mozilla::cyclecollector::AddJSHolder?

No, because of http://hg.mozilla.org/mozilla-central/annotate/a167dfaa105b/xpcom/base/nsCycleCollector.cpp#l154

> @@ +2980,5 @@
> > +cyclecollector::AddJSHolder(void* aHolder, nsScriptObjectTracer* aTracer)
> > +{
> > +    nsCycleCollector *collector = sCollector.get();
> > +
> > +    MOZ_ASSERT(collector && collector != (nsCycleCollector*)1 &&
> 
> Please define some new constant like kShutdownSentinel or something and use
> that instead of |(nsCycleCollector*)1| here and elsewhere in the file.  You
> can delete the explanatory comments for |(nsCycleCollector*)1| at the same
> time if you like.

Done.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b53ab3047ce5

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Was backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/36b914bf56f5

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2864e2610800

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94fb66d82988

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2864e2610800
https://hg.mozilla.org/mozilla-central/rev/94fb66d82988

Comment 9

[Ed Morley [:emorley]]

Backed out at khuey's request for causing bug 881266:
remote:   https://hg.mozilla.org/mozilla-central/rev/200344975d6f
remote:   https://hg.mozilla.org/mozilla-central/rev/86413e921d5d

Nightlies respun.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 2

This should fix the crashes

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 764291 [details] [diff] [review]
Part 2

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

::: xpcom/base/nsCycleCollector.cpp
@@ +2985,2 @@
>      }
> +    else {

please cuddle the else |} else {|

@@ +3082,4 @@
>          MOZ_CRASH();
>      }
>  
> +    nsAutoPtr<CollectorData> data(new CollectorData);

It would be better to just move all this data stuff down into the NS_SUCCEEDED body below, as you don't use it otherwise.

@@ +3089,5 @@
>      nsAutoPtr<nsCycleCollector> collector(new nsCycleCollector(aThreadingModel));
>  
>      nsresult rv = collector->Init();
>      if (NS_SUCCEEDED(rv)) {
> +        data->mCollector = collector.forget();

Is this really going to compile on all platforms?  I think there's some check that you don't go from a ref counted pointer to a raw pointer.  I don't remember how you are supposed to work around it though.  Maybe throw a .get() in there?

@@ +3102,5 @@
>  {
> +    CollectorData *data = sCollectorData.get();
> +
> +    // We should have started the cycle collector by now.
> +    MOZ_ASSERT(data);

It would be better if you had some kind of debug-only StartedCollector(CollectorData*) that checks these two things, and use that, instead of having these 3 lines of boilerplate all over the place.

@@ +3129,5 @@
> +    // We should have started the cycle collector by now.
> +    MOZ_ASSERT(data);
> +    MOZ_ASSERT(data->mCollector);
> +
> +    

trailing whitespace. you also don't really need two blank lines in a row.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/projects/cypress/rev/bb6c79366a3a
https://hg.mozilla.org/projects/cypress/rev/089b861c6688

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> Comment on attachment 764291 [details] [diff] [review]
> Part 2
> 
> Review of attachment 764291 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/base/nsCycleCollector.cpp
> @@ +2985,2 @@
> >      }
> > +    else {
> 
> please cuddle the else |} else {|

Done.

> @@ +3082,4 @@
> >          MOZ_CRASH();
> >      }
> >  
> > +    nsAutoPtr<CollectorData> data(new CollectorData);
> 
> It would be better to just move all this data stuff down into the
> NS_SUCCEEDED body below, as you don't use it otherwise.

Done.

> @@ +3089,5 @@
> >      nsAutoPtr<nsCycleCollector> collector(new nsCycleCollector(aThreadingModel));
> >  
> >      nsresult rv = collector->Init();
> >      if (NS_SUCCEEDED(rv)) {
> > +        data->mCollector = collector.forget();
> 
> Is this really going to compile on all platforms?  I think there's some
> check that you don't go from a ref counted pointer to a raw pointer.  I
> don't remember how you are supposed to work around it though.  Maybe throw a
> .get() in there?

We're forgetting an nsAutoPtr, not an nsRefPtr.  It just returns a raw T*.

> @@ +3102,5 @@
> >  {
> > +    CollectorData *data = sCollectorData.get();
> > +
> > +    // We should have started the cycle collector by now.
> > +    MOZ_ASSERT(data);
> 
> It would be better if you had some kind of debug-only
> StartedCollector(CollectorData*) that checks these two things, and use that,
> instead of having these 3 lines of boilerplate all over the place.

They're different in different functions, so I didn't consolidate.

> @@ +3129,5 @@
> > +    // We should have started the cycle collector by now.
> > +    MOZ_ASSERT(data);
> > +    MOZ_ASSERT(data->mCollector);
> > +
> > +    
> 
> trailing whitespace. you also don't really need two blank lines in a row.

Fixed.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb6c79366a3a
https://hg.mozilla.org/mozilla-central/rev/089b861c6688