Route JS holding through the cycle collection runtime

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Posted patch PatchSplinter Review
This provides a more generic method for NS_HOLD/DROP_JS_OBJECTS to use than calling XPConnect directly.
Attachment #755837 - Flags: review?(continuation)
Is the new stuff available to binary addons?
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.
Attachment #755837 - Flags: review?(continuation) → review+
(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.
Assignee: nobody → khuey
Blocks: 845545
https://hg.mozilla.org/mozilla-central/rev/2864e2610800
https://hg.mozilla.org/mozilla-central/rev/94fb66d82988
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 881266
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch Part 2Splinter Review
This should fix the crashes
Attachment #764291 - Flags: review?(continuation)
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.
Attachment #764291 - Flags: review?(continuation) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/bb6c79366a3a
https://hg.mozilla.org/mozilla-central/rev/089b861c6688
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.