1299770 - Support stack capturing in content processes

Status: RESOLVED WONTFIX

(Toolkit :: Telemetry, defect, P4)

Description

[Georg Fritzsche [:gfritzsche]]

Following up to bug 1225851, we'll want to support stack capturing in the content process too.

The plan would be to:
* stop storing this data in the child processes, instead sending it up to
the parent process like bug 1218576
* change the stack snapshot function to return the snapshots organized by
process (`parent`, `content`)
* put the snapshot data here into `payloadObj.processes.parent`, `payloadObj.processes.content`, etc.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

All of the interesting graphics things happen in non-browser processes (content or gpu/compositor), so it will be very exciting when this bug gets resolved.

Comment 2

[Iaroslav Sheptykin]

Attachment: Bug 1299770: Implement stack capturing in content processes.

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

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

https://reviewboard.mozilla.org/r/190162/#review195404


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/telemetry/KeyedStackCapturer.cpp:131
(Diff revision 1)
> -  ProcessedStack stack = GetStackAndModules(rawStack);
>  
>    // Store the new stack info.
>    size_t stackIndex = mStacks.AddStack(stack);
> -  mStackInfos.Put(aKey, new StackFrequencyInfo(1, stackIndex));
> +  mStackInfos.Put(aKey, new StackFrequencyInfo(1, stackIndex, aProcessId));
> +  return;

Warning: Redundant return statement at the end of a function with a void return type [clang-tidy: readability-redundant-control-flow]

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

https://reviewboard.mozilla.org/r/190162/#review198234

Nice, this is coming together and the functional pieces are mostly in place!
For the C++ we can move over to clean-up and adjusting structure now.

I left some comments below based on our conversations.
And i understand the TelemetrySession JS changes are coming up after.

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:124
(Diff revision 1)
> -  std::vector<uintptr_t> rawStack;
> -  auto callback = [](uint32_t, void* aPC, void*, void* aClosure) {
> -    std::vector<uintptr_t>* stack =
> +  ProcessedStack stack = aStack
> +    ? *aStack
> +    : this->CaptureStackAndModules();

It seems a bit surprising to have the capturing in this function.
Can we move this and the check to the caller?

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:135
(Diff revision 1)
> -  mStackInfos.Put(aKey, new StackFrequencyInfo(1, stackIndex));
> +  mStackInfos.Put(aKey, new StackFrequencyInfo(1, stackIndex, aProcessId));
> +  return;
>  }
>  
>  NS_IMETHODIMP
>  KeyedStackCapturer::ReflectCapturedStacks(JSContext *cx, JS::MutableHandle<JS::Value> ret)

Currently this is in `<main ping>/payload/capturedStacks/`.
Let's move this to `<main ping>/payload/processes/<process>/capturedStacks`.
This means snapshot should probably be of the form: `{"parent": ..., "content": ...}`, with the process parts having the full data for that process.

Then TelemetrySession.jsm takes care of putting it into the right place in `payload/processes/`.

::: toolkit/components/telemetry/ipc/TelemetryComms.h:388
(Diff revision 1)
> +    size_t i,
> +      numModules = aParam.GetNumModules(),
> +      numFrames = aParam.GetStackSize();
> +
> +    // Serializing the list of modules.
> +    WriteParam(aMsg, numModules);

You should be able to use `WriteParam(aMsg, theModules)`.
For that you'll need a serializer for `Module`.
Then you should be able to just serialize an array of modules.
If `std::vector` is not supported you might need to use a `nsTArray`.

Then `ReadParam()` also becomes much simpler.

I wonder: Does `ProcessedStack` actually need to use `std::vector`? Just using `nsTArray` there might simplify things here.

::: toolkit/components/telemetry/ipc/TelemetryComms.h:396
(Diff revision 1)
> +      WriteParam(aMsg, module.mName);
> +      WriteParam(aMsg, module.mBreakpadId);
> +    }
> +
> +    // Serializing the stack (list of frames).
> +    WriteParam(aMsg, numFrames);

Same for `Frame`s here.

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:26
(Diff revision 1)
> +const KEYED_BOOL_SCALAR = "telemetry.test.keyed_boolean_kind";
> +const CONTENT_ONLY_UINT_SCALAR = "telemetry.test.content_only_uint";
> +const ALL_PROCESSES_UINT_SCALAR = "telemetry.test.all_processes_uint";
> +const ALL_CHILD_PROCESSES_STRING_SCALAR = "telemetry.test.all_child_processes_string";
> +
> +function run_child_test() {

There seem to be some scalar test left-overs in this file.

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:76
(Diff revision 1)
> +  // Run test in child, don't wait for it to finish: just wait for the
> +  // MESSAGE_CHILD_TEST_DONE.
> +  run_test_in_child("test_ChildStacks.js");
> +  await do_await_remote_message(MESSAGE_CHILD_TEST_DONE);
> +
> +  await waitForContentScalars();

This probably wants to wait instead for content stacks to show up in the snapshot.

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:85
(Diff revision 1)
> +  const payload = TelemetrySession.getPayload("test-ping");
> +
> +  // Validate the scalar data.
> +  Assert.ok("processes" in payload, "Should have processes section");
> +  Assert.ok("content" in payload.processes, "Should have child process section");
> +  Assert.ok("capturedStacks" in payload.processes.parent, "Child process section should have stacks.");

You mean to use `payload.processes.content`?

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:87
(Diff revision 1)
> +  // Validate the scalar data.
> +  Assert.ok("processes" in payload, "Should have processes section");
> +  Assert.ok("content" in payload.processes, "Should have child process section");
> +  Assert.ok("capturedStacks" in payload.processes.parent, "Child process section should have stacks.");
> +
> +  let stacks = payload.processes.parent.capturedStacks;

`.content.`?

Comment 5

[Iaroslav Sheptykin]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

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

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

https://reviewboard.mozilla.org/r/190162/#review207918

I only took a high-level look over TelemetrySession, the child stack test and the snapshot function.
The structure for adding it to the ping looks good to me now.
Also happy to see the test coverage is working!

A small thing for later when you get to cleaning up: I noticed some indentation inconsistencies and some "tabs vs. spaces" issues.

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:189
(Diff revisions 1 - 2)
> +
> +  return fullReportObj;
> +}
> +
> +NS_IMETHODIMP
> +KeyedStackCapturer::ReflectCapturedStacks(JSContext *cx, JS::MutableHandle<JS::Value> ret)

I like this clear separation into one function for each serialization level.

::: toolkit/components/telemetry/TelemetrySession.jsm:1373
(Diff revision 2)
> -        payloadObj.processes.parent.capturedStacks = stacks;
> +        for (let process in stacks) {
> +    	  let capturesInfo = stacks[process];

We can now do neat things like:
`for (const [process, captureInfo] of Object.entries(stacks)) {`

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

Comment 7

[Iaroslav Sheptykin]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

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

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

https://reviewboard.mozilla.org/r/190162/#review216774

Hey Iaroslav,

it looks like we're basically there, i can't see any fundamental issues here.
I commented below on some smaller things from a high-level review, i'm still missing a detailed pass (i can do that before or after an update).

Something we shouldn't forget: IPC serializers need IPC peer review; we can do that after my passes.

::: toolkit/components/telemetry/KeyedStackCapturer.h:36
(Diff revision 3)
> +	  // Pre-populating the array of process captures with empty capture data.
> +	  for (size_t i = 0; i < this->mCapturesByProcess.Capacity(); ++i) {
> +		  this->mCapturesByProcess.AppendElement(Captures());
> +	  }

Let's move the constructor into the .cpp file.

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:161
(Diff revision 3)
>    if (!keysArray) {
> -    return NS_ERROR_FAILURE;
> +	  return nullptr;
>    }
>  
>    bool ok = JS_DefineProperty(cx, fullReportObj, "captures",
> -                              keysArray, JSPROP_ENUMERATE);
> +				                              keysArray, JSPROP_ENUMERATE);

Nit:
There are a few indentation issues in this file, specifically using tabs instead of spaces.
I also see places where you use 4 instead of 2 spaces per indentation level.

::: toolkit/components/telemetry/docs/collection/stack-capture.rst:24
(Diff revision 3)
>  
>  The API
>  =======
>  Capturing stacks is available either via the `nsITelemetry interface <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/nsITelemetry.idl>`_
>  or the `C++ API <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.h>`_.
> -Note that current implementation of the API is not thread safe. Also, capturing
> +Note that current implementation of the API is not thread safe. Capturing

Isn't it actually thread-safe now?

::: toolkit/components/telemetry/ipc/TelemetryIPC.cpp:66
(Diff revision 3)
> +#if defined(MOZ_GECKO_PROFILER)
> +
> +void
> +TelemetryIPC::RecordChildStacks(Telemetry::ProcessID aProcessType, const nsTArray<Telemetry::ChildStackData>& aStacks)
> +{
> +  Telemetry::RecordChildStacks(aProcessType, aStacks);

I don't think we need to add any public interfaces to `Telemetry.h`.
Can you just call `KeyedStackCapturer::RecordChildStacks()` here?

::: toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp:56
(Diff revision 3)
> +// TODO: Figure out a good limit for stacks.
> +const size_t kStacksArrayHighWaterMark = 100;

We can do a rough estimate S of how much the maximum size of a single stack capture is.
From there we can pick a low-enough N here, so that `S*N*kWaterMarkDiscardFactor` never goes over the 10MB buffer size.

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:19
(Diff revision 3)
> +const PLATFORM_VERSION = "1.9.2";
> +const APP_VERSION = "1";
> +const APP_ID = "[e-mail]";
> +const APP_NAME = "XPCShell";
> +
> +// We need both in order to capture stacks.

Both? Looks like a left-over?
Maybe just move the check for the profiler into `add_task()` below.

::: toolkit/components/telemetry/tests/unit/test_TelemetryCaptureStack.js:65
(Diff revision 3)
>    Telemetry.captureStack(key);
>    let stacks = Telemetry.snapshotCapturedStacks(clear);
> -  Assert.ok(checkObjectStructure(stacks));
> -  return stacks;
> +  ["parent", "content", "gpu"].forEach(process => {
> +    Assert.ok(checkObjectStructure(stacks[process]));
> +  });
> +  return stacks.parent;

Asserting first that the "parent" key exists makes test errors more clear.

Comment 9

[Iaroslav Sheptykin]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

https://reviewboard.mozilla.org/r/190162/#review216774

> I don't think we need to add any public interfaces to `Telemetry.h`.
> Can you just call `KeyedStackCapturer::RecordChildStacks()` here?

The problem here is that `KeyedStackCapturer::RecordChildStacks()` isn't a static method. TelemetryImpl is holding a single instance of `KeyedStackCapturer`. Therefore all methond calls on that instance must go through `TelemetryImpl` (`Telemetry.cpp`). Or am I missing something?

We might add some code to share the capturer instance via a getter for ex. Do you think it's good idea?

Comment 10

[Iaroslav Sheptykin]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

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

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Iaroslav Sheptykin from comment #9)
> Comment on attachment 8919258 [details]
> Bug 1299770: Implement stack capturing in content processes.
> 
> https://reviewboard.mozilla.org/r/190162/#review216774
> 
> > I don't think we need to add any public interfaces to `Telemetry.h`.
> > Can you just call `KeyedStackCapturer::RecordChildStacks()` here?
> 
> The problem here is that `KeyedStackCapturer::RecordChildStacks()` isn't a
> static method. TelemetryImpl is holding a single instance of
> `KeyedStackCapturer`. Therefore all methond calls on that instance must go
> through `TelemetryImpl` (`Telemetry.cpp`). Or am I missing something?
> 
> We might add some code to share the capturer instance via a getter for ex.
> Do you think it's good idea?

Ah, i see. That is a different architecture from the rest of the data collection.
We probably only ever need one KeyedStacjCapturer though, like TelemetryHistogram & TelemetryScalar?
Then let's defer this part to a follow-up bug and mark the method as internal-usage-only with a comment.

Comment 12

[Iaroslav Sheptykin]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190162/diff/4-5/

Comment 13

[Iaroslav Sheptykin]

Hi Georg!

I addressed your feedback regarding the comment. Updated the patch and rebased it on the latest central. It should be ready for a detailed review.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Alessio is taking a look here to get things moving.

Comment 15

[Alessio Placitelli [:Dexter]]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

https://reviewboard.mozilla.org/r/190162/#review240056

Hey Iraoslav, good job on this one. This is almost there, I found a few style nits and have a couple of requests for the test coverage. After that, we should be good to go!

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:82
(Diff revision 5)
> +  for (size_t i = 0; i < this->mCapturesByProcess.Capacity(); ++i) {
> +    this->mCapturesByProcess.AppendElement(Captures());
> +  }
> +}
>  
> +void KeyedStackCapturer::Capture(const nsACString& aKey) {

nit: since we're here, can we make this conform to our [style guide](https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style), *Methods and functions* section?

tl;dr - there should be a line break after `void` (just for the function implementations);

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:109
(Diff revision 5)
>    }
>  
> -  // We haven't captured a stack for this key before, do it now.
> -  // Note that this does a stackwalk and is an expensive operation.
> +  this->AddStack(aKey, &stack, Telemetry::ProcessID::Parent);
> +}
> +
> +bool KeyedStackCapturer::StackKeyExists(const nsACString& aKey,

nit: let's add a line break after `bool`

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:112
(Diff revision 5)
> -  // Note that this does a stackwalk and is an expensive operation.
> +}
> +
> +bool KeyedStackCapturer::StackKeyExists(const nsACString& aKey,
> +                                        Telemetry::ProcessID aProcessId) {
> +  // A shortcut for the process captures in the current process.
> +  Captures & captures = mCapturesByProcess[static_cast<size_t>(aProcessId)];

nit: since we're not doing anything with `captures`, we can merge these two lines as:

`return mCapturesByProcess[static_cast<size_t>(aProcessId)].mStackInfos.Get(aKey) != nullptr;`

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:116
(Diff revision 5)
> +  // A shortcut for the process captures in the current process.
> +  Captures & captures = mCapturesByProcess[static_cast<size_t>(aProcessId)];
> +  return nullptr != captures.mStackInfos.Get(aKey);
> +}
> +
> +ProcessedStack KeyedStackCapturer::CaptureStackAndModules()

nit: add a line break after `ProcessedStack`

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:129
(Diff revision 5)
> +
>    MozStackWalk(callback, /* skipFrames */ 0, /* maxFrames */ 0, &rawStack);
> -  ProcessedStack stack = GetStackAndModules(rawStack);
> +  return GetStackAndModules(rawStack);
> +}
> +
> +void KeyedStackCapturer::AddStack(const nsACString& aKey,

nit: linebreak :)

::: toolkit/components/telemetry/ipc/TelemetryComms.h:433
(Diff revision 5)
>    }
>  };
>  
>  template<>
>  struct
> +ParamTraits<mozilla::Telemetry::ProcessedStack::Frame>

This (and the IPC parts) require a review from an IPC peer: see [here](https://wiki.mozilla.org/Modules/Core) for a list of IPC peers (e.g. froydnj).

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:5
(Diff revision 5)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/
> +*/
> +
> +Cu.import("resource://gre/modules/Services.jsm", this);

All the `Cu.` need to be replaced with `ChromeUtils.`.

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:31
(Diff revision 5)
> +    return stacks.content.captures.length > 0;
> +
> +  });
> +}
> +
> +add_task({

Let's add two more tests:

- let's test multiple accumulations for the same stack in different processes: ensure that the counts match up (in this file);
- let's test the limit for the number of stacks kept (kMaxCaptureStacksKept, in `test_TelemetryCaptureStack.js`)

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:53
(Diff revision 5)
> +  finishAddonManagerStartup();
> +  await TelemetryController.testSetup();
> +  // Make sure we don't generate unexpected pings due to pref changes.
> +  await setEmptyPrefWatchlist();
> +
> +  // Run test in child, don't wait for it to finish: just wait for the

Let's also capture a stack for the parent process. This way we make sure that things are stored properly.

Let's also verify that both stacks are present below.

::: toolkit/components/telemetry/tests/unit/test_TelemetryCaptureStack.js:63
(Diff revision 5)
>   */
>  function captureStacks(key, clear = true) {
>    Telemetry.captureStack(key);
>    let stacks = Telemetry.snapshotCapturedStacks(clear);
> -  Assert.ok(checkObjectStructure(stacks));
> -  return stacks;
> +  ["parent", "content", "gpu"].forEach(process => {
> +    // Ensure that the informaiton for the process type exists in the stacks

nit: 'information'

Comment 16

[Iaroslav Sheptykin]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190162/diff/5-6/

Comment 17

[Iaroslav Sheptykin]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

https://reviewboard.mozilla.org/r/190162/#review240056

Hi Alessio! Thanks for going throught the patch back then. It took me longer than expected to work through the changes. I addressed you feedback and rebased the patch onto the latest central. Build and tests pass on my machine. Woould be great if you could take a look at the rewritten tests for child stacks. Also, could you give me a hint on how to proceed with IPC-review? Should I simply flag someone from the list you mentionend?

Comment 18

[Iaroslav Sheptykin]

adding a needinfo for the comment abouve.

Comment 19

[Alessio Placitelli [:Dexter]]

(In reply to Iaroslav Sheptykin from comment #18)
> adding a needinfo for the comment abouve.

Hey! Next week is All-Hands, so I'm afraid I won't be able to get to this (but I'll try!). As for the IPC-review, yes, please flag froydnj or another IPC peer to review the code in TelemetryComms.h (and probably the IPDL file changes too).

Comment 20

[Alessio Placitelli [:Dexter]]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

https://reviewboard.mozilla.org/r/190162/#review257862

Thanks Iaroslav and sorry for the delay. I'm basically nitpicking, as the foundation looks solid enough. Please address the comments below and flag froydnj (by adding him into the reviewers' list in the commit message) for the IPC parts. Also leave him a ni? so that he knows he has to take a focused look at the IPC-relevant changes :)

::: toolkit/components/telemetry/KeyedStackCapturer.h:139
(Diff revision 6)
> +   * @param aCaptures - object storing information on captures.
> +   *
> +   * @returns constructed rooted JSObject or a null pointer if construction
> +   *          failed.
> +   */
> +  JSObject *

nit: make it `JSObject*` instead of `JSObject *` (the pointer stays with the type), and move `CreateJSCapturesObject` on the same line. See the [style guide](https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code) for both.

::: toolkit/components/telemetry/KeyedStackCapturer.h:140
(Diff revision 6)
> +   *
> +   * @returns constructed rooted JSObject or a null pointer if construction
> +   *          failed.
> +   */
> +  JSObject *
> +  CreateJSCapturesObject(JSContext *cx,

nit `aCx` and the pointer goes with the type.

::: toolkit/components/telemetry/KeyedStackCapturer.h:141
(Diff revision 6)
> +   * @returns constructed rooted JSObject or a null pointer if construction
> +   *          failed.
> +   */
> +  JSObject *
> +  CreateJSCapturesObject(JSContext *cx,
> +                         const Captures & aCaptures);

nit: `const Captures&`, reference with the type

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:133
(Diff revision 6)
> -  ProcessedStack stack = GetStackAndModules(rawStack);
> +  return GetStackAndModules(rawStack);
> +}
> +
> +void
> +KeyedStackCapturer::AddStack(const nsACString& aKey,
> +                                  const ProcessedStack* aStack,

nit: indentation should match the line above

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:134
(Diff revision 6)
> +}
> +
> +void
> +KeyedStackCapturer::AddStack(const nsACString& aKey,
> +                                  const ProcessedStack* aStack,
> +                                  const Telemetry::ProcessID aProcessId) {

nit: indentation should match the line above

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:138
(Diff revision 6)
> +                                  const ProcessedStack* aStack,
> +                                  const Telemetry::ProcessID aProcessId) {
> +  MutexAutoLock captureStackMutex(mStackCapturerMutex);
> +
> +  // A shortcut for the process captures in the current process.
> +  Captures & captures = mCapturesByProcess[static_cast<size_t>(aProcessId)];

nit: ref goes with the type

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:159
(Diff revision 6)
>    // Store the new stack info.
> -  size_t stackIndex = mStacks.AddStack(stack);
> -  mStackInfos.Put(aKey, new StackFrequencyInfo(1, stackIndex));
> +  size_t stackIndex = captures.mStacks.AddStack((*aStack));
> +  captures.mStackInfos.Put(aKey, new StackFrequencyInfo(1, stackIndex));
>  }
>  
> -NS_IMETHODIMP
> +JSObject *

nit: the asterisk goes ith the type

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:160
(Diff revision 6)
> -  size_t stackIndex = mStacks.AddStack(stack);
> -  mStackInfos.Put(aKey, new StackFrequencyInfo(1, stackIndex));
> +  size_t stackIndex = captures.mStacks.AddStack((*aStack));
> +  captures.mStackInfos.Put(aKey, new StackFrequencyInfo(1, stackIndex));
>  }
>  
> -NS_IMETHODIMP
> -KeyedStackCapturer::ReflectCapturedStacks(JSContext *cx, JS::MutableHandle<JS::Value> ret)
> +JSObject *
> +KeyedStackCapturer::CreateJSCapturesObject(JSContext *cx, const Captures & captures)

nit: adjust as done in the header file

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:203
(Diff revision 6)
> +
> +  return fullReportObj;
> +}
> +
> +NS_IMETHODIMP
> +KeyedStackCapturer::ReflectCapturedStacks(JSContext *cx, JS::MutableHandle<JS::Value> ret)

nit: `* aCx`, `aRet`, the asterisk goes with the type

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:229
(Diff revision 6)
>  
>  void
>  KeyedStackCapturer::Clear()
>  {
>    MutexAutoLock captureStackMutex(mStackCapturerMutex);
> -  mStackInfos.Clear();
> +  for (auto & captures : mCapturesByProcess) {

nit: `auto&`

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:238
(Diff revision 6)
> +}
> +
> +void
> +KeyedStackCapturer::RecordChildStacks(Telemetry::ProcessID aProcessType,
> +                                      const nsTArray<Telemetry::ChildStackData>& aStacks) {
> +  for (auto & stackData : aStacks) {

nit: `auto&`

::: toolkit/components/telemetry/ProcessedStack.h:48
(Diff revision 6)
>    };
>  
>    const Frame &GetFrame(unsigned aIndex) const;
>    void AddFrame(const Frame& aFrame);
> +
> +  const nsTArray<Frame> & GetFrames() const;

nit: the reference goes with the type.

::: toolkit/components/telemetry/ProcessedStack.h:51
(Diff revision 6)
>    void AddFrame(const Frame& aFrame);
> +
> +  const nsTArray<Frame> & GetFrames() const;
> +  void SetFrames(const nsTArray<Frame>& aFrames);
> +
>    const Module &GetModule(unsigned aIndex) const;

nit: while we're here, would you kindly move the `&` near `const Module`?

::: toolkit/components/telemetry/ProcessedStack.h:54
(Diff revision 6)
> +  void SetFrames(const nsTArray<Frame>& aFrames);
> +
>    const Module &GetModule(unsigned aIndex) const;
>    void AddModule(const Module& aFrame);
>  
> +  const nsTArray<Module> & GetModules() const;

nit: same :)

::: toolkit/components/telemetry/ProcessedStack.cpp:86
(Diff revision 6)
> +{
> +  mModules.Clear();
> +  mStack.Clear();
> +}
> +
> +const nsTArray<ProcessedStack::Module> & ProcessedStack::GetModules() const

nit: the reference goes next to the type

::: toolkit/components/telemetry/ProcessedStack.cpp:91
(Diff revision 6)
> +const nsTArray<ProcessedStack::Module> & ProcessedStack::GetModules() const
> +{
> +  return this->mModules;
>  }
>  
> -void ProcessedStack::Clear() {
> +const nsTArray<ProcessedStack::Frame> & ProcessedStack::GetFrames() const

nit: same

::: toolkit/components/telemetry/Telemetry.cpp:1614
(Diff revision 6)
>                                     std::move(aAnnotations));
>  }
>  
>  void
>  TelemetryImpl::DoStackCapture(const nsACString& aKey) {
> -  if (Telemetry::CanRecordExtended() && XRE_IsParentProcess()) {
> +  if (Telemetry::CanRecordExtended()) {

Since we're here, can we change it to `Telemetry::CanRecordPrereleaseData()`?

::: toolkit/components/telemetry/Telemetry.cpp:1622
(Diff revision 6)
>  }
> +
> +void
> +TelemetryImpl::RecordChildStacks(ProcessID aProcessType,
> +                                 const nsTArray<ChildStackData>& aStacks) {
> +  if (Telemetry::CanRecordExtended()) {

Can we change it to `Telemetry::CanRecordPrereleaseData()`? The other function will be removed in the future.

::: toolkit/components/telemetry/docs/data/main-ping.rst:316
(Diff revision 6)
>  --------------
>  Contains information about stacks captured on demand via Telemetry API. For more
>  information see :doc:`stack capture <../collection/stack-capture>`.
>  
>  This is similar to :ref:`chromeHangs`, but only Precise C++ stacks on the main thread of
> -the parent process are reported. This data is only available on Windows, either
> +the calling process are reported. This data is available either in Firefox Nightly

This is not exactly true: "or in builds using ``--enable-profiling`` switch on pre Release channels"

::: toolkit/components/telemetry/ipc/TelemetryComms.h:440
(Diff revision 6)
>    }
>  };
>  
>  template<>
>  struct
> +ParamTraits<mozilla::Telemetry::ProcessedStack::Frame>

Please let's have :froydnj review this

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:29
(Diff revision 6)
> + * Wait until stacks captured in child appear in the parent process.
> + */
> +async function waitForContentStacks() {
> +  await ContentTaskUtils.waitForCondition(() => {
> +    const stacks =
> +      Telemetry.snapshotCapturedStacks(false);

nit: add `/* aClear */` after the `false` so that it's clear what that is about.

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:34
(Diff revision 6)
> +      Telemetry.snapshotCapturedStacks(false);
> +    return stacks.content.captures.length > 0;
> +  });
> +}
> +
> +function captureStack(key, times) {

nit: add a documentation header for this

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:105
(Diff revision 6)
> +  ]
> +]);
> +
> +add_task({
> +  // Only execute tests in this file if the build contains profiler.
> +  // Without profiler stack capturing is disabled.

Do we have a test for this? We could add a simple test that only runs when MOZ_GECKO_PROFILER is false and checks that stack capturing is really disabled. (if it's not already around)

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:119
(Diff revision 6)
> +  }
> +
> +  // Setup parent telemetry.
> +  do_get_profile(true);
> +  loadAddonManager(APP_ID, APP_NAME, APP_VERSION, PLATFORM_VERSION);
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.TelemetryEnabled, true);

No need to set this pref, it's already done in [head.js](https://searchfox.org/mozilla-central/rev/f822a0b61631cbb38901569e69b4967176314aa8/toolkit/components/telemetry/tests/unit/head.js#370)

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:130
(Diff revision 6)
> +  // Run test in child, don't wait for it to finish: just wait for the
> +  // MESSAGE_CHILD_TEST_DONE.
> +  run_test_in_child("test_ChildStacks.js");
> +  await do_await_remote_message(MESSAGE_CHILD_TEST_DONE);
> +
> +  // Make sure captured stacks has arrived into parent.

nit: "have arrived"

::: toolkit/components/telemetry/tests/unit/test_ChildStacks.js:143
(Diff revision 6)
> +
> +  TestSuite.forEach(handler => {
> +    handler.checkPayload(payload);
> +  });
> +
> +  do_test_finished();

Please mention that this is needed due to `run_test_in_child`

Comment 21

[Alessio Placitelli [:Dexter]]

Hey Iaroslav, do you need any support or clarification related to comment 20?

Comment 22

[Iaroslav Sheptykin]

Hi Alessio!

Thanks for checking on me! I am moving in a snail pace towards addressing your feedback. Some private issue like buying a house are keeping me busy but I am still on this issue. I'll try to update the patch till Fri.

Comment 23

[Alessio Placitelli [:Dexter]]

(In reply to Iaroslav Sheptykin from comment #22)
> Hi Alessio!
> 
> Thanks for checking on me! I am moving in a snail pace towards addressing
> your feedback. Some private issue like buying a house are keeping me busy
> but I am still on this issue. I'll try to update the patch till Fri.

No problem Iaroslav, I just wanted to make sure you were not stuck because my comments were unclear :) Please do take care ;-)

Comment 24

[Iaroslav Sheptykin]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

https://reviewboard.mozilla.org/r/190162/#review257862

> nit: `* aCx`, `aRet`, the asterisk goes with the type

I adjusted method signature in the `.h` file as well.

> nit: add `/* aClear */` after the `false` so that it's clear what that is about.

I added a const with a speaking name here instead. Hope it's ok? To me it reads better what the code does.

Comment 25

[Iaroslav Sheptykin]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

https://reviewboard.mozilla.org/r/190162/#review257862

> Do we have a test for this? We could add a simple test that only runs when MOZ_GECKO_PROFILER is false and checks that stack capturing is really disabled. (if it's not already around)

I could not find a test for ensuring that we stack capturing is disabled on builds without profiler. I could easily add one.
There were some issues with MOZ_GECKO_PROFILER though as far as I can recall. If my understanding is correct then we don't do test builds without profiler, so our test pipeline will never execute the test. Some platforms provide firefox builds with profiler disabled (free BSD? not sure). If MOZ_GECKO_PROFILER are misplaced builds on those platform start to fail. I don't know however if those build run tests. In the worst case, we write a test that never gets executed. Should I still go ahead an add the test?

Comment 26

[Iaroslav Sheptykin]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190162/diff/6-7/

Comment 27

[Code Review Bot [:reviewbot]]

Comment on attachment 8919258 [details]
Bug 1299770: Implement stack capturing in content processes.

https://reviewboard.mozilla.org/r/190162/#review269596


Code analysis found 2 defects in this patch:
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/telemetry/KeyedStackCapturer.cpp:16
(Diff revision 7)
>  #include "jsapi.h"
> +#include "ipc/TelemetryIPCAccumulator.h"
> +#include "ipc/TelemetryComms.h"
> +#include "TelemetryCommon.h"
> +
> +namespace TelemetryIPCAccumulator = mozilla::TelemetryIPCAccumulator;

Warning: Namespace alias decl 'TelemetryIPCAccumulator' is unused [clang-tidy: misc-unused-alias-decls]

namespace TelemetryIPCAccumulator = mozilla::TelemetryIPCAccumulator;
~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

::: toolkit/components/telemetry/KeyedStackCapturer.cpp:157
(Diff revision 7)
> +  }
>  
>    // Store the new stack info.
> -  size_t stackIndex = mStacks.AddStack(stack);
> -  mStackInfos.Put(aKey, new StackFrequencyInfo(1, stackIndex));
> +  size_t stackIndex = captures.mStacks.AddStack((*aStack));
> +  captures.mStackInfos.Put(aKey, new StackFrequencyInfo(1, stackIndex));
>  }

Warning: Potential memory leak [clang-tidy: clang-analyzer-cplusplus.NewDeleteLeaks]

}
^

Comment 28

[Iaroslav Sheptykin]

Alessio,

I updated the patch and rebased it onto the last central. Could you take a look a the comments regarding test? Also I don't quite understand how to deal with what the review bot has found. Do you have an idea how to address its feedback?

Comment 29

[Iaroslav Sheptykin]

Attachment: Bug 1299770: Implement stack capturing in content processes. r?gfritzsche

MozReview-Commit-ID: GNPFB2NinIK

Comment 30

[Iaroslav Sheptykin]

Hi Georg! I moved the review to phabricator! Hopefully we land this patch until the next review tool change :)
See: https://phabricator.services.mozilla.com/D4662

Comment 31

[Georg Fritzsche [:gfritzsche]]

I think this could be closed?

Comment 32

[Chris H-C :chutten]

(In reply to Georg Fritzsche [:gfritzsche] from comment #31)

I think this could be closed?

I think that's fair, seeing as the demand for this feature has lessened over time. We can always reopen and pick up from where we left off.