Closed Bug 1483650 Opened 3 years ago Closed 3 years ago

Remove the component registration for nsCycleCollectorLogger

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → ehsan
Blocks: 1477576
Attachment #9001352 - Flags: review?(continuation)
Does this break the ability to create a new logger in JS?
Flags: needinfo?(ehsan)
nsICycleCollectorListener is builtinclass.  Is that possible today?
Flags: needinfo?(ehsan) → needinfo?(continuation)
We used to have a CC logger in JS that could be run during mochitests...I guess that code died at some point.
I had old style extension which implemented the interface and analyzed CC graphs in browser.

Doesn't the change make all nsICycleCollectorHandler handling rather useless.
(In reply to :Ehsan Akhgari from comment #3)
> nsICycleCollectorListener is builtinclass.  Is that possible today?

I think so. I think you can use @mozilla.org/cycle-collector-logger;1 to create a new instance of nsICycleCollectorListener in JS, then pass that into the CC. You can use this to capture a log for a specific CC, which is useful sometimes. That said, I'm not wedded to createInstance being the way to create these logger things.

(In reply to Nathan Froyd [:froydnj] from comment #4)
> We used to have a CC logger in JS that could be run during mochitests...I
> guess that code died at some point.

Mochitest browser chrome tests originally used the ++DOMWINDOW/--DOMWINDOW stuff to find windows and docshells that were created during a test, but not cleaned up during that test. The testing was flakey, so somebody wrote a similar thing that would instead look at the CC log, in JS, to figure out if a window is still around or not. Later, somebody figured out how to make the ++DOMWINDOW stuff not flakey (by using unbuffered printf, I believe), but we were still running both styles of test. In the e10s era, I got the ++DOMWINDOW stuff working for child processes, but the CC logger based leak checking seemed redundant (and more prone to false negatives) so I just removed it instead of updating it.
Comment on attachment 9001352 [details] [diff] [review]
Remove the component registration for nsCycleCollectorLogger

CC logging from JS is sometimes useful for debugging.
Flags: needinfo?(continuation)
Attachment #9001352 - Flags: review?(continuation) → review-
Note to self: add a method to Components.utils to return a new instance of the logger for JS consumers to use...
Attachment #9001352 - Attachment is obsolete: true
Comment on attachment 9001608 [details] [diff] [review]
Remove the component registration for nsCycleCollectorLogger

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

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +686,5 @@
>     * Will throw a DOM security error if called without chrome privileges in
>     * non-debug builds. Available to all callers in debug builds.
>     *
>     * @param aListener listener that receives information about the CC graph
> +   *                  (see nsCycleCollectorLogger for a logger component)

I think you can just drop the "(see nsCycleCollectorLogger for a logger component)" from both places. I don't think it is that useful. Anybody who wants to figure out how to create a nsICycleCollectorListener can just search for that.

::: js/xpconnect/idl/xpccomponents.idl
@@ +288,5 @@
>       */
>      void forceCC([optional] in nsICycleCollectorListener aListener);
>  
> +    /*
> +     * To be called from JS only.

Please specify how to get a listener in C++.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +1112,5 @@
>  MOZ_EXPORT void
>  DumpCompleteHeap()
>  {
>      nsCOMPtr<nsICycleCollectorListener> listener =
> +      nsCycleCollector_createLogger();

This could probably all be on one line.

::: xpcom/base/nsICycleCollectorListener.idl
@@ +15,5 @@
>  interface nsIFile;
>  
>  /**
>   * A set of interfaces for recording the cycle collector's work. An instance
> + * of nsCycleCollectorLogger can be configured to enable various

I think you should change the "nsCycleCollectorLogger" you added to "nsICycleCollectorListener", for all of the comments in this file.

@@ +97,5 @@
>   * is called while the cycle collector is running.
>   *
>   * To analyze cycle collection data in JS:
>   *
> + * - Create an instance of nsCycleCollectorLogger, which implements this

Please specify how to create an instance of nsICycleCollectorListener. Something like, "In C++, this can be done by calling nsCycleCollector_createLogger(). In JS, this can be done by calling Components.utils.createCCLogger()."
Attachment #9001608 - Flags: review?(continuation) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58650e51b8b7
Remove the component registration for nsCycleCollectorLogger; r=mccr8
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd4fe6172c8
follow-up: Remove two extra declarations which aren't needed any more
https://hg.mozilla.org/mozilla-central/rev/58650e51b8b7
https://hg.mozilla.org/mozilla-central/rev/9fd4fe6172c8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.