Closed
Bug 877584
Opened 11 years ago
Closed 11 years ago
Route JS holding through the cycle collection runtime
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: khuey, Assigned: khuey)
References
Details
Attachments
(2 files)
10.39 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
10.51 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
This provides a more generic method for NS_HOLD/DROP_JS_OBJECTS to use than calling XPConnect directly.
Attachment #755837 -
Flags: review?(continuation)
Comment 1•11 years ago
|
||
Is the new stuff available to binary addons?
Comment 2•11 years ago
|
||
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+
Assignee | ||
Comment 3•11 years ago
|
||
(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 | ||
Comment 4•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b53ab3047ce5
Status: NEW → ASSIGNED
Comment 5•11 years ago
|
||
Was backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/36b914bf56f5
Assignee | ||
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2864e2610800
Assignee | ||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/94fb66d82988
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2864e2610800 https://hg.mozilla.org/mozilla-central/rev/94fb66d82988
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 9•11 years ago
|
||
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 → ---
Assignee | ||
Comment 10•11 years ago
|
||
This should fix the crashes
Attachment #764291 -
Flags: review?(continuation)
Comment 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/projects/cypress/rev/bb6c79366a3a https://hg.mozilla.org/projects/cypress/rev/089b861c6688
Assignee | ||
Comment 13•11 years ago
|
||
(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•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bb6c79366a3a https://hg.mozilla.org/mozilla-central/rev/089b861c6688
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•