Make the cycle collector invoke methods directly on nsCycleCollectorLogger

RESOLVED FIXED in Firefox 41

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

3 years ago
nsICycleCollectorListener is odd.  It only has a single implementation class, but there's no way to take advantage of this.  We also expose a bunch of methods to JS, but really we only use a few boolean attributes.

The simplest thing to do would be to add some kind of annotation for most of the methods that they aren't exposed to script.  A more complete fix would convert this to a WebIDL interface that had only a few boolean attributes, for use from JS, and then just define nsCycleCollectorLogger as a final class for people to use from C++.
(Assignee)

Comment 1

3 years ago
nsCycleCollectorLogger is causing problems for the GC rooting analysis. The CC calls methods on mListener, which has type nsICycleCollectorListener, which the rooting analysis can't tell must be nsCycleCollectorLogger.

We can solve this by splitting nsCycleCollectorLogger into two classes. The first outer class, which I'll call nsCycleCollectorListener, represents the part of the nsICycleCollectorListener that we actually manipulate from JS: the various setters, plus processNext. The residual inner class, nsCycleCollectorLogger, will be what we call methods on directly while the CC is running. The main difficulty is dealing with processNext().
Assignee: nobody → continuation
Summary: Expose less of nsICycleCollectorListener to JS → Spit nsCycleCollectorLogger into two classes
(Assignee)

Comment 2

3 years ago
We'll still be calling an abstract nsI method in mListener->Begin() and mListener->End() but hopefully that's okay.
(Assignee)

Comment 3

3 years ago
The splitting approach was fairly complicated, and wasn't working for some reason. A simpler approach is to add an AsLogger() method on nsICycleCollectorListener that returns an instance of the concrete type.
Summary: Spit nsCycleCollectorLogger into two classes → Make the cycle collector invoke methods directly on nsCycleCollectorLogger
(Assignee)

Updated

3 years ago
Blocks: 749370
(Assignee)

Updated

3 years ago
status-firefox40: affected → ---
(Assignee)

Comment 4

3 years ago
Created attachment 8614743 [details] [diff] [review]
part 1 - Make the cycle collector use the concrete logger class.

This patch makes it so that while the cycle collector is running methods are called
on the concrete implementation nsCycleCollectorLogger, rather than the interface
nsICycleCollectorListener. This makes explicit the requirement that we have to be
very careful about what we call during the cycle collector, and should make it
possible for the GC rooting static analysis to understand what is happening.
Attachment #8614743 - Flags: review?(bugs)
(Assignee)

Comment 5

3 years ago
Created attachment 8614744 [details] [diff] [review]
part 2 - Rename various cycle collector listener variables to logger.
Attachment #8614744 - Flags: review?(bugs)
(Assignee)

Comment 6

3 years ago
Created attachment 8614745 [details] [diff] [review]
part 3 - Clean up some cycle collector logger set up code.
Attachment #8614745 - Flags: review?(bugs)
(Assignee)

Comment 7

3 years ago
Created attachment 8614746 [details] [diff] [review]
part 4 - De-COM the nsICycleCollectorListener methods we only call from C++.
Attachment #8614746 - Flags: review?(bugs)
(Assignee)

Comment 8

3 years ago
Created attachment 8614747 [details] [diff] [review]
part 5 - Add a less COM-y getter for mWantAllTraces.
Attachment #8614747 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8614743 - Flags: review?(bugs) → review+

Updated

3 years ago
Attachment #8614744 - Flags: review?(bugs) → review+

Updated

3 years ago
Attachment #8614745 - Flags: review?(bugs) → review+

Comment 9

3 years ago
Comment on attachment 8614746 [details] [diff] [review]
part 4 - De-COM the nsICycleCollectorListener methods we only call from C++.

+ * A set of interfaces for recording the cycle collector's work. An instance
+ * of @mozilla.org/cycle-collector-logger;1 configured to enable various
+ * options, then passed to the cycle collector when it runs.
+ * Note that additional logging options are available by setting environment
+ * variables, as described at the top of nsCycleCollector.cpp.
something odd with the second sentence. Missing some verb?
Attachment #8614746 - Flags: review?(bugs) → review+

Updated

3 years ago
Attachment #8614747 - Flags: review?(bugs) → review+
(Assignee)

Comment 12

3 years ago
(In reply to Olli Pettay [:smaug] from comment #9)
> something odd with the second sentence. Missing some verb?

Good catch. I added "can be".

I had to rev the UUID of nsICycleCollectorHandler in part 1 because the UUID commit hook was complaining. I'll file a bug about that.
You need to log in before you can comment on or make changes to this bug.