1367406 - Collect interleaved stacks in the Background Hang Reporter

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

In bug 1357829 I changed the background hang reporter to use the profiler's stackwalking code to collect native hang stacks.

It would be even nicer if we could re-use the profiler's stack interleaving logic to generate fully interleaved stacks from background hang reports. This bug tracks doing that in a privacy preserving manner.

Comment 1

[Nicholas Nethercote [inactive]]

I've started working on this.

Comment 2

[Nicholas Nethercote [inactive]]

I'm making progress on blockers for this, untangling some messes.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Introduce ProfilerStackCollector

mystor, please take a look at ProfilerStackCollector and see if it fits your
needs.

There are two things I haven't addressed yet.

- Privacy concerns about the dynamic strings. Is the dynamic string always
  privacy-sensitive? If so, that would make things easy, because
  BHRStackCollector could just always ignore the aStr argument.

- I'm not sure what additional processing of JITReturnAddr frames is required.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Created attachment 8885659 [details] [diff] [review]
> Introduce ProfilerStackCollector

It's possible this patch should go in a bug report that blocks this one, given that this patch introduces machinery to support the collection of interleaved stacks, but doesn't actually collect them.

Comment 5

[Nicholas Nethercote [inactive]]

> It's possible this patch should go in a bug report that blocks this one,
> given that this patch introduces machinery to support the collection of
> interleaved stacks, but doesn't actually collect them.

I filed bug 1380286.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Modify HangStack to allow it to also contain native frame entries

This one is a bit large, as changing one piece had large impacts on the rest of BHR. 

If it's too much for one patch I can try to break it up into smaller patches.

MozReview-Commit-ID: AfrtWfLUi2l

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Add a method for reading module information for native stack frames

MozReview-Commit-ID: 7gzFhygwXx4

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Expose computed modules through .modules getter

MozReview-Commit-ID: F6aDnlwr9jt

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8895461 [details] [diff] [review]
Part 3: Expose computed modules through .modules getter

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

::: toolkit/components/backgroundhangmonitor/HangDetails.cpp
@@ +145,4 @@
>  nsHangDetails::GetModules(JSContext* aCx, JS::MutableHandleValue aVal)
>  {
>    size_t length = 0;
>    JS::RootedObject retObj(aCx, JS_NewArrayObject(aCx, length));

Now that we're putting information in here, maybe we should do instead:

  auto& modules = mDetails.mStack.GetModules();
  size_t length = modules.Length();
  ...JS_NewArrayObject called with the correct length...

  for (size_t i = 0; i < length; ++i) {
    ...
  }

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8895460 [details] [diff] [review]
Part 2: Add a method for reading module information for native stack frames

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

I think this is all reasonable?

::: toolkit/components/backgroundhangmonitor/HangStack.cpp
@@ +92,5 @@
> +
> +} // anonymous namespace
> +
> +void
> +HangStack::ReadModuleInformation()

Documentation says mModules should be empty when this function is called, maybe MOZ_ASSERT this?

@@ +109,5 @@
> +  rawModules.SortByAddress();
> +
> +  size_t frameIdx = 0;
> +  for (size_t i = 0; i < rawModules.GetSize(); ++i) {
> +    const SharedLibrary& info= rawModules.GetEntry(i);

Nit: spaces around `=`.

@@ +127,5 @@
> +        // ModOffset one. mModules.Length() will be the index of the module when
> +        // we append it below, and we set moduleReferenced to true to ensure
> +        // that we do.
> +        moduleReferenced = true;
> +        uint32_t module = mModules.Length();

Maybe these computations should go after checking the offset for validity, so we don't wind up declaring the module used and then not inserting it into the list because the offset was > UINT32_MAX?  (A bit of a weird situation, to be sure, but worth handling, no?)

@@ +129,5 @@
> +        // that we do.
> +        moduleReferenced = true;
> +        uint32_t module = mModules.Length();
> +        size_t offset = frame->AsPC() - moduleStart;
> +        if (NS_WARN_IF(offset > UINT32_MAX)) {

Should probably explicitly compute offset as a uint64_t so you don't get warnings here for the tautological comparison on 32-bit platforms?

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8895459 [details] [diff] [review]
Part 1: Modify HangStack to allow it to also contain native frame entries

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

I think I would prefer a smaller set of patches if possible.

::: toolkit/components/backgroundhangmonitor/HangStack.h
@@ +88,5 @@
> +      MOZ_ASSERT(mKind == Kind::STRING);
> +      return mString;
> +    }
> +
> +    const char* const& AsString() const {

Your dedication to const-correctness is to be applauded.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nathan Froyd [:froydnj] from comment #10)
> Documentation says mModules should be empty when this function is called,
> maybe MOZ_ASSERT this?

Fair.

I don't MOZ_ASSERT much in BHR because we don't run BHR when DEBUG is set (bug 979069), so it isn't _exactly_ tested. Perhaps I'll MOZ_RELEASE_ASSERT it.

Comment 13

[Nathan Froyd [:froydnj]]

MOZ_RELEASE_ASSERT has a bunch of potential for badness, though it would definitely catch the error.  Maybe just force-clearing the array instead?

Comment 14

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nathan Froyd [:froydnj] from comment #13)
> MOZ_RELEASE_ASSERT has a bunch of potential for badness, though it would
> definitely catch the error.  Maybe just force-clearing the array instead?

Sure, I can do that.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1A: Allow HangStack to contain raw PCs and Module offsets

The HangStack previously consisted of an array of const char* pointers into its
backing string buffer, which represented pseudostack entries. With interleaved
stacks, it is now possible for the stack to contain raw unresolved program
counters (Kind::PC), and module/offset pairs (Kind::MODOFFSET). To do this, we
use a discriminated union, and make the backing array use the discriminated
union instead of const char*s.

The code cannot use mozilla::Variant<const char*, uintptr_t, Module>
unfortuantely, as we cannot use the implementation of ParamTraits for Variant in
HangStack's ParamTraits implementation.

When deserializing a HangStack over IPC, we need to read the string frame
entries into the backing string buffer, and generate const char* entries for
each of the strings which we read in over IPC. The default implementation of
ParamTraits wouldn't give us access to the enclusing HangStack object while
deserializing each individual entry, so we couldn't use it. In fact, Entries
don't have ParamTraits implemented for them at all, and can only be sent over
IPC as part of a HangStack due to this dependency.

MozReview-Commit-ID: KrLn9e9OHAR

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1B: Remove nsIHangDetails.pseudoStack, replace ProcessedStack w/ new HangStack type

Previously there were two stack objects on each HangDetails object: mStack and
mPseudoStack. mStack was a Telemetry::ProcessedStack, while mPseudoStack was a
HangStack. After the changes in part 1A, HangStack can now contain all of the
information of both the old HangStack and ProcessedStack, so the mPseudoStack
field is renamed to mStack, and the old mStack field is removed.

This patch also implements the new GetStack getter, which generates the JS data
format for the new HangStack type.

MozReview-Commit-ID: C27ooyw70oD

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1C: Collect interleaved stacks w/ ProfilerStackCollector API in ThreadStackHelper

This new API was added by njn in bug 1380286, and provides both pseudostack and
native stack entries to the consumer of the API.

This patch changes ThreadStackHelper to use this new API instead of the previous
one, and use it to collect the frames directly into HangStack objects.

MozReview-Commit-ID: JihfcBL5Cdl

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8896002 [details] [diff] [review]
Part 1C: Collect interleaved stacks w/ ProfilerStackCollector API in ThreadStackHelper

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

Thanks for splitting these patches up, things made a lot more sense this way.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5f22010d45
Part 1: Add interleaved stack functionality to HangStack, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a6959119152
Part 2: Add a method for reading module information for native stack frames, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f65afbc48a
Part 3: Expose computed modules through .modules getter, r=froydnj

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5a5f22010d45
https://hg.mozilla.org/mozilla-central/rev/5a6959119152
https://hg.mozilla.org/mozilla-central/rev/70f65afbc48a