1420975 - Add a way to record JS stack for leaks

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Arthur Iakab [arthur_iakab]]

:froydn do you have any updates on this bug? Because https://bugzilla.mozilla.org/show_bug.cgi?id=1392130 is depending on it.

Comment 3

[Nathan Froyd [:froydnj]]

Andrew probably has more context here.

Comment 4

[Andrew McCreight [:mccr8]]

I'll try to figure out a way to move forward on bug 1392130.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1420975 - Add a environment variable to record JS stack for leaks.

I had to define a new function for grabbing the stack as a string
because nsContentUtils::GetCurrentJSContextForThread() can only be
called from either the main thread or a worker thread. I was
interested in investigating a leak of an nsTimer created by JS, so
this function ends up getting called on other threads.

Review commit: https://reviewboard.mozilla.org/r/211912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211912/

Comment 6

[Andrew McCreight [:mccr8]]

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 7

[Nathan Froyd [:froydnj]]

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?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(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?

Comment 9

[Andrew McCreight [:mccr8]]

(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.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8941670 [details]
Bug 1420975 - Add a environment variable to record JS stack for leaks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211912/diff/1-2/

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 13

[Nathan Froyd [:froydnj]]

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?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8941670 [details]
Bug 1420975 - Add a environment variable to record JS stack for leaks.

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

Comment 15

[Andrew McCreight [:mccr8]]

(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.

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 8941670 [details]
Bug 1420975 - Add a environment variable to record JS stack for leaks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211912/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c7b21fa7315
Add a environment variable to record JS stack for leaks. r=froydnj,mrbkap

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5c7b21fa7315