Closed Bug 1420975 Opened 2 years ago Closed 2 years ago

Add a way to record JS stack for leaks

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file)

Setting XPCOM_MEM_LOG_CLASSES can be used to get a C++ stack for the allocation of a leaked object, but for some leaks, like bug 1392130, we want to know what the JS stack looks like for the allocation. This could be implemented by adding a new environment variable.
Depends on: 1421104
With my patch, if you set the environment variable XPCOM_MEM_LOG_JS_STACK, on top of all of the other ones you might set to track leaks for a particular class, then it'll record a JS stack in addition to a C++ stack for allocated objects of the given class. This is probably much slower than the C++ stack recording (which I think does not symbolicate as it goes) but still better than nothing.
No longer depends on: 1421104
:froydn do you have any updates on this bug? Because https://bugzilla.mozilla.org/show_bug.cgi?id=1392130 is depending on it.
Flags: needinfo?(nfroyd)
Andrew probably has more context here.
Flags: needinfo?(nfroyd) → needinfo?(continuation)
I'll try to figure out a way to move forward on bug 1392130.
Flags: needinfo?(continuation)
Assignee: nobody → continuation
Blake, please review the XPConnect changes. I had to add a new call to dom::danger::GetJSContext(), so it feels a dodgy. Hopefully it is okay. As I mention in the commit, I couldn't simply use nsContentUtils::GetCurrentJSContextForThread() to get a context, because that only works from either the main thread or a worker thread. I was using this to investigate an nsTimer leaked from JS, and of course nsTimers are created on all sorts of threads, so the method has to be able to deal with it. I'm hoping guarding things with IsJSAPIActive() is reasonably.

Nathan, please review the XPCOM changes. There's nothing too odd going on. It piggybacks on top of your previous C++ stack recording work.
Comment on attachment 8941670 [details]
Bug 1420975 - Add a environment variable to record JS stack for leaks.

https://reviewboard.mozilla.org/r/211912/#review218288

Hooray.  Question below.

::: xpcom/base/nsTraceRefcnt.cpp:140
(Diff revision 1)
>    // leak-checking, you're gonna have a bad time.
>    std::vector<void*> allocationStack;
> +  JS::UniqueChars jsStack;
> +
> +  void SaveJSStack() {
> +    jsStack = xpc::PrintJSStackFromAnyThread();

Do we want to save this to our own `UniquePtr` type, just in case the memory in the `JS::UniqueChars` is tied to the current thread's `JSContext` somehow, so we don't UAF ourselves?
Attachment #8941670 - Flags: review?(nfroyd) → review+
(In reply to Andrew McCreight [:mccr8] from comment #6)
> this to investigate an nsTimer leaked from JS, and of course nsTimers are
> created on all sorts of threads, so the method has to be able to deal with
> it. I'm hoping guarding things with IsJSAPIActive() is reasonably.

I don't understand the reasoning behind this (and this patch seems super scary to me). We aren't running JS code on these random threads, are we? So we shouldn't need to return a stack on them? What am I missing?
Flags: needinfo?(continuation)
(In reply to Blake Kaplan (:mrbkap) from comment #8)
> I don't understand the reasoning behind this (and this patch seems super
> scary to me). We aren't running JS code on these random threads, are we? So
> we shouldn't need to return a stack on them? What am I missing?

You are right: with my patch, we don't return a stack. Somehow we need to detect if we're on a thread that is running JS. If we're not, the code I added does nothing. Is there a better way to do this? For instance, is there a better way to detect if JS is running on the current thread from any thread? If there is, I could call nsContentUtils::GetCurrentJSContextForThread() instead.
Flags: needinfo?(continuation)
Comment on attachment 8941670 [details]
Bug 1420975 - Add a environment variable to record JS stack for leaks.

https://reviewboard.mozilla.org/r/211912/#review219516

After speaking to mccr8 on IRC, he's going to work on a version of this that can use public APIs.
Attachment #8941670 - Flags: review?(mrbkap) → review-
Attachment #8941670 - Flags: review-
Attachment #8941670 - Flags: review+
Comment on attachment 8941670 [details]
Bug 1420975 - Add a environment variable to record JS stack for leaks.

Blake, this version uses a standard method to get the JS context, but I did have to move xpc_PrintJSStack into the public API. That seems like a benign function. I also moved xpc_DumpJSStack into the public API at the same time because it seemed weird to not have them in the same place.

Nathan, please look at SaveJSStack(). I want to make sure I didn't do anything bad there with my string functions. I implemented your suggestion to not use JS::UniqueChars.
Attachment #8941670 - Flags: review?(nfroyd)
Comment on attachment 8941670 [details]
Bug 1420975 - Add a environment variable to record JS stack for leaks.

https://reviewboard.mozilla.org/r/211912/#review220580

Just two comments, which you are welcome to implement or ignore?

::: xpcom/base/nsTraceRefcnt.cpp:151
(Diff revision 2)
> +    JSContext* cx = nsContentUtils::GetCurrentJSContextForThread();
> +    if (!cx) {
> +      return;
> +    }
> +
> +    JS::UniqueChars chars = xpc_PrintJSStack(cx, false, false, false);

Do we want to add some `/*argument=*/` comments here for these `false` arguments?

::: xpcom/base/nsTraceRefcnt.cpp:154
(Diff revision 2)
> +    }
> +
> +    JS::UniqueChars chars = xpc_PrintJSStack(cx, false, false, false);
> +    size_t len = strlen(chars.get());
> +    jsStack = MakeUnique<char[]>(len + 1);
> +    strncpy(jsStack.get(), chars.get(), len + 1);

Maybe use plain `memcpy` here, so people don't have to think about whether there's some secret behavior of `strncpy` that they have to remember?
Attachment #8941670 - Flags: review?(nfroyd) → review+
Comment on attachment 8941670 [details]
Bug 1420975 - Add a environment variable to record JS stack for leaks.

https://reviewboard.mozilla.org/r/211912/#review220700
Attachment #8941670 - Flags: review?(mrbkap) → review+
(In reply to Nathan Froyd [:froydnj] from comment #13)
> Do we want to add some `/*argument=*/` comments here for these `false`
> arguments?
Sure, I can do that.

> Maybe use plain `memcpy` here, so people don't have to think about whether
> there's some secret behavior of `strncpy` that they have to remember?
Alright.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c7b21fa7315
Add a environment variable to record JS stack for leaks. r=froydnj,mrbkap
https://hg.mozilla.org/mozilla-central/rev/5c7b21fa7315
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.