Closed Bug 1194424 Opened 9 years ago Closed 9 years ago

Serialize and deserialize JS::ubi::StackFrames into and out of core dumps

Categories

(DevTools :: Memory, defect)

Product:
DevTools
Component:
Memory
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox43 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 43
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(6 files, 7 obsolete files)

Part 0: Extend the core dump protobuf message format to accomodate allocation stacks
73.75 KB, patch
sfink
: review+
Details | Diff | Splinter Review
Part 4: Add an xpcshell test for heap snapshots and allocation stacks
2.55 KB, patch
sfink
: review+
Details | Diff | Splinter Review
Part 1: Serialize JS::ubi::StackFrame allocation stacks into core dumps
6.53 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
Part 2: Deserialize JS::ubi::StackFrame allocation stacks from core dumps
12.79 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
Part 5: gtest for DeserializedStackFrame ubi::StackFrames
5.98 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
Part 3: Implement a concrete JS::ubi::StackFrame specialization backed by a frame deserialized from a core dump
19.37 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review 
We need to

* Serialize JS::ubi::StackFrame allocation stacks into core dumps

* Deserialize them back out again into DeserializedStackFrames (similar to the existing DeserializedNode type)

* Make a concrete JS::ubi::StackFrame specialization backed by a DeserializedStackFrame
Depends on: 1194426
Attachment #8648186 - Flags: review?(sphink)
Attachment #8648189 - Flags: review?(sphink)
Attachment #8648193 - Flags: review?(sphink)
Attachment #8648221 - Flags: review?(sphink)
Attachment #8648222 - Flags: review?(sphink)
Note that most of part 0 is largely code generated by the protobuf compiler. You can focus only on the .proto message definition file.

As far as tests go, I had a hard time writing tests for this series. The result is pretty meager (part 4). It is pretty hard to capture allocation stacks without using the Debugger API in JS, and so writing gtest unit tests (like the rest of the serialization/deserialization unit tests) is really difficult.

My hope is that we can have more in depth test coverage at a slightly higher level in bug 1139476, when we are actually running analyses on top of offline heap snapshots and can compare them to the results of live analyses.

That said, if you have ideas for how to make this serialization/deserialization more testable, please give me suggestions!
I think I can write up some basic gtests for the JS::ubi::StackFrame concrete specialization for DeserializedStackFrame, however. Will do that and attach a patch.
Attachment #8648186 - Flags: review?(sphink) → review+
Comment on attachment 8648189 [details] [diff] [review]
Part 1: Serialize JS::ubi::StackFrame allocation stacks into core dumps

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

::: toolkit/devtools/server/HeapSnapshot.cpp
@@ +483,5 @@
>      return writeMessage(metadata);
>    }
>  
>    virtual bool writeNode(const JS::ubi::Node& ubiNode,
> +                         EdgePolicy includeEdges) override final {

I think they want one of {virtual,override,final}. So this should just be 'final'.

@@ +672,5 @@
>    StreamWriter writer(cx, gzipStream, wantNames);
> +  if (NS_WARN_IF(!writer.init())) {
> +    rv.Throw(NS_ERROR_OUT_OF_MEMORY);
> +    return;
> +  }

Whoa, this wasn't needed before? Weird.
Attachment #8648189 - Flags: review?(sphink) → review+
Comment on attachment 8648193 [details] [diff] [review]
Part 2: Deserialize JS::ubi::StackFrame allocation stacks from core dumps

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

Looks ok to my naive eye.

::: toolkit/devtools/server/DeserializedNode.h
@@ +101,5 @@
>    DeserializedNode& operator=(const DeserializedNode&) = delete;
>  };
>  
> +static inline js::HashNumber
> +hashIdDerivedFromPtr(uint64_t id)

Can you use DefaultHasher<void*>(reinterpret_cast<void*>(id)) here or in the callers? It would remove some code duplication and remove the magic constant 3.

::: toolkit/devtools/server/HeapSnapshot.cpp
@@ +165,5 @@
> +HeapSnapshot::saveStackFrame(const protobuf::StackFrame& frame,
> +                             StackFrameId& outFrameId)
> +{
> +  if (frame.has_ref()) {
> +    // We should only get a reference to previous frame if we have already seen

s/to/to the/

::: toolkit/devtools/server/HeapSnapshot.h
@@ +82,5 @@
>    // Save the given `protobuf::Node` message in this `HeapSnapshot` as a
>    // `DeserializedNode`.
>    bool saveNode(const protobuf::Node& node);
>  
> +  // Save the give `protobuf::StackFrame` message in this `HeapSnapshot` as a

*given

@@ +86,5 @@
> +  // Save the give `protobuf::StackFrame` message in this `HeapSnapshot` as a
> +  // `DeserializedStackFrame`. The saved stack frame's id is returned via the
> +  // out parameter.
> +  bool saveStackFrame(const protobuf::StackFrame& frame,
> +                      StackFrameId& outFrameId);

Hm. I thought the coding style recommended pointers for outparams, but I don't see it.
Attachment #8648193 - Flags: review?(sphink) → review+
Comment on attachment 8648221 [details] [diff] [review]
Part 3: Implement a concrete JS::ubi::StackFrame specialization backed by a frame deserialized from a core dump

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

I'm probably missing something obvious, but I'm no compiler, and I didn't understand why the Principals needed to be de-constified? I mean, I know you're deserializing principals (or rather, deserializing an isSystem bit to decide what principals to use), but I don't see why that means it can no longer be const?

::: js/src/vm/SavedStacks.cpp
@@ +1395,5 @@
>      }
>      return true;
>  }
>  
> +struct MOZ_STACK_CLASS Atomizer

This *really* needs a comment. IIUC, you're abusing something::match() as a foreach()/map() by passing this in as the filter predicate? It would be nicer to export a foreach() or something instead, but I guess if it's limited to this one use, it's not a big deal. Still needs the comment.

::: toolkit/devtools/server/DeserializedNode.cpp
@@ +198,5 @@
> +}
> +
> +bool
> +ConcreteStackFrame<DeserializedStackFrame>::constructSavedFrameStack(
> +  JSContext*cx,

space before cx
Attachment #8648221 - Flags: review?(sphink) → review+
Comment on attachment 8648222 [details] [diff] [review]
Part 4: Add an xpcshell test for heap snapshots and allocation stacks

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

::: toolkit/devtools/server/tests/unit/test_ReadHeapSnapshot_with_allocations.js
@@ +32,5 @@
> +  ok(true, "Should be able to save a snapshot.");
> +
> +  const snapshot = ChromeUtils.readHeapSnapshot(filePath);
> +  ok(snapshot, "Should be able to read a heap snapshot");
> +  ok(snapshot instanceof HeapSnapshot, "Should be an instanceof HeapSnapshot");

Hm, there should be a way to do better testing. I suppose you could grab a saveStack() near the allocation site and then compare the relevant bits with the snapshot. Is there a way to produce a snapshot in memory instead of writing it to a file, and then compare the two? (Throwing out anything with a line number greater than the first one's line, maybe.)

I'm not sure what's possible.
Attachment #8648222 - Flags: review?(sphink) → review+
Waldo: two questions for you. First, one of these patches is de-constipating some JSPrincipals pointers because SavedFrames hold onto either trusted or untrusted principals, and the untrusted ones need to be refcounted with JS_HoldPrincipals.

One option would be to use a Variant<>, but personally I'd rather make the refcount field mutable. What do you think?

Second, when deserializing stack frames with principals attached, we only know whether the principals were the trusted ones or not (JS_SetTrustedPrincipals). We need to map all others to some untrusted principals pointer. It feels a little slippery to be putting "trusted" vs "untrusted" into JSAPI, and I was initially thinking we should have JS_SetUnknownPrincipals or something for this. But now I'm feeling like if we already have something called JS_SetTrustedPrincipals, and we already do things differently depending on whether principals are "system" principals or not, that maybe JS_SetUntrustedPrincipals really is the right thing.
Flags: needinfo?(jwalden+bmo)
Depends on: 1195577
Attachment #8649029 - Flags: review?(sphink)
(In reply to Steve Fink [:sfink, :s:] from comment #12)
> Second, when deserializing stack frames with principals attached, we only
> know whether the principals were the trusted ones or not
> (JS_SetTrustedPrincipals). We need to map all others to some untrusted
> principals pointer. It feels a little slippery to be putting "trusted" vs
> "untrusted" into JSAPI, and I was initially thinking we should have
> JS_SetUnknownPrincipals or something for this. But now I'm feeling like if
> we already have something called JS_SetTrustedPrincipals, and we already do
> things differently depending on whether principals are "system" principals
> or not, that maybe JS_SetUntrustedPrincipals really is the right thing.

Note that I split JS_SetUntrustedPrincipals out into bug 1195577.
Comment on attachment 8649386 [details] [diff] [review]
Part 2: Deserialize JS::ubi::StackFrame allocation stacks from core dumps

Note that I didn't convert the id to void* and use the default pointer hasher, because the snapshot could have been serialized on a 64bit platform while the deserializing is on a 32 bit platform, and in this scenario we would lose some of the entropy in the upper 32 bits of the id.
Attachment #8649386 - Flags: review+
Comment on attachment 8649387 [details] [diff] [review]
Part 3: Implement a concrete JS::ubi::StackFrame specialization backed by a frame deserialized from a core dump

I added some comments about AtomizingMatcher (née Atomizer). It isn't really a hacky thing like you seem to imply in the review comments, it is just a mozilla::Variant matcher[0] to exhuastively match every variant case and have that checked by the compiler. We have a Variant<const char16_t*, JSAtom*> variant and want to ensure that we get a JSAtom* out on the other side. Hopefully my comments have cleared this up a bit.

[0] https://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h?from=mozilla%3A%3AVariant&offset=0#258-286
Attachment #8649387 - Flags: review+
(In reply to Steve Fink [:sfink, :s:] from comment #11)
> Comment on attachment 8648222 [details] [diff] [review]
> Part 4: Add an xpcshell test for heap snapshots and allocation stacks
> 
> Review of attachment 8648222 [details] [diff] [review]:
> -----------------------------------------------------------------
> Hm, there should be a way to do better testing. I suppose you could grab a
> saveStack() near the allocation site and then compare the relevant bits with
> the snapshot. Is there a way to produce a snapshot in memory instead of
> writing it to a file, and then compare the two? (Throwing out anything with
> a line number greater than the first one's line, maybe.)
> 
> I'm not sure what's possible.

I haven't expanded this test, but I did write a gtest for JS::ubi::StackFrame instances that are backed by DeserializedStackFrame (part 5).

At the moment, you can't do anything with a heap snapshot; census will be the first useful thing you can do with it.

We could write some test-only APIs (not sure how to do that with webidl) that iterate each node in the graph and give it to the test-driving JS caller as an object and with a SavedFrame stack if it has an allocation stack. This seems like quite a bit of work, and I'm not against that, but I'd like your feedback on whether you think it is still necessary after the gtest in part 5 and with the future census-on-live-heap vs census-on-offline-snapshot tests that I am planning on writing in bug 1139476.

My vision for the latter tests with regards to allocation stacks in particular is:

* Start tracking allocations
* Allocate objects with "interesting" [[Class]] names (some typed array or something) that are not present unless the JS explicitly allocates them (unlike say Function objects)
* Take a census of the live heap graph
* Save and read back an offline heap snapshot
* Take a census of the offline heap snapshot
* Compare the ByAllocationSite breakdowns of the two census (they should be the same)

We could write a CoreDumpWriter that writes to memory, but again we would still need to write test-only APIs for working with that and it doesn't seem to me to have a huge benefit other than being faster for tests.

So what do you think? Are the sanity integration test (part 4), the gtest for JS::ubi::StackFrame backed by a DeserializedStackFrame (part 5), and the future census tests good enough test coverage?
Flags: needinfo?(sphink)
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #18)
> Comment on attachment 8649386 [details] [diff] [review]
> Part 2: Deserialize JS::ubi::StackFrame allocation stacks from core dumps
> 
> Note that I didn't convert the id to void* and use the default pointer
> hasher, because the snapshot could have been serialized on a 64bit platform
> while the deserializing is on a 32 bit platform, and in this scenario we
> would lose some of the entropy in the upper 32 bits of the id.

Oh wow. I would not have considered that. Comment, please?

I wonder if it would be useful to have a uint64_ptr_t with a DefaultHasher specialization.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #19)
> Comment on attachment 8649387 [details] [diff] [review]
> Part 3: Implement a concrete JS::ubi::StackFrame specialization backed by a
> frame deserialized from a core dump
> 
> I added some comments about AtomizingMatcher (née Atomizer). It isn't really
> a hacky thing like you seem to imply in the review comments, it is just a
> mozilla::Variant matcher[0] to exhuastively match every variant case and
> have that checked by the compiler. We have a Variant<const char16_t*,
> JSAtom*> variant and want to ensure that we get a JSAtom* out on the other
> side. Hopefully my comments have cleared this up a bit.

Oh! Thanks, I never looked at Variant before, I just sort of assumed I knew enough based on the general idea. You're right, that usage makes sense. And it's much easier to figure out with the new name; 'Atomizer' had me thinking from the beginning that it's sort of a functor for atomizing strings.
Comment on attachment 8649029 [details] [diff] [review]
Part 5: gtest for DeserializedStackFrame ubi::StackFrames

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

This is great, thanks!

::: toolkit/devtools/server/tests/gtest/DeserializedStackFrameUbiStackFrames.cpp
@@ +34,5 @@
> +    mocked.source = source;
> +    mocked.functionDisplayName = functionDisplayName;
> +
> +    DeserializedStackFrame& deserialized = mocked;
> +    JS::ubi::StackFrame ubi(&deserialized);

Do you prefer naming this variable ubi? I think I'd rather see frame or mockFrame.
Attachment #8649029 - Flags: review?(sphink) → review+
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #20)
> My vision for the latter tests with regards to allocation stacks in
> particular is:
> 
> * Start tracking allocations
> * Allocate objects with "interesting" [[Class]] names (some typed array or
> something) that are not present unless the JS explicitly allocates them
> (unlike say Function objects)
> * Take a census of the live heap graph
> * Save and read back an offline heap snapshot
> * Take a census of the offline heap snapshot
> * Compare the ByAllocationSite breakdowns of the two census (they should be
> the same)
> 
> So what do you think? Are the sanity integration test (part 4), the gtest
> for JS::ubi::StackFrame backed by a DeserializedStackFrame (part 5), and the
> future census tests good enough test coverage?

Totally, yes. The census thing covers everything I'd want. Thanks!
Flags: needinfo?(sphink)
Comment on attachment 8649434 [details] [diff] [review]
Part 5: gtest for DeserializedStackFrame ubi::StackFrames

Renamed to `ubiFrame`.
Attachment #8649434 - Flags: review+
As noted on IRC, mutable refcount seems right.  Also, while I'm leery about trusted/untrusted principals at all, I'm not confident in that assessment nor in my ability to intelligently comment on this -- so referred to bholley, who expressed similar leeriness and much better comments than I would have.  :-)  Over to him for any issues that could arise requiring discussion.
Flags: needinfo?(jwalden+bmo)
Ok so I tried making the refcount mutable, keeping the trustedPrincipals as const, and making SavedFrame and the things it calls take a const JSPrincipals* and it isn't panning out very well. I would have to make all of these take const JSPrincipals*:

- JS_HoldPrincipals
- JS_DropPrincipals
- JSSubsumesOp
- JSDestroyPrincipalsOp
- JS_DestroyPrincipals

Notably, the last two feel really wrong.

So unless y'all have really strong objections, I am going to keep the change to make the trustedPrincipals non-const.
Comment on attachment 8649608 [details] [diff] [review]
Part 3: Implement a concrete JS::ubi::StackFrame specialization backed by a frame deserialized from a core dump

Asking for re-review because this includes the ReconstructedSavedFramePrincipals singletons we discussed on IRC.
Attachment #8649608 - Flags: review?(sphink)
Comment on attachment 8649609 [details] [diff] [review]
Part 5: gtest for DeserializedStackFrame ubi::StackFrames

Turns out that I don't need to introduce special toy principals for gtests with the introduction of the ReconstructedSavedFramePrincipals in part 3, so this just removes those. Carrying the previous r+ forward.
Attachment #8649609 - Flags: review+
Comment on attachment 8649608 [details] [diff] [review]
Part 3: Implement a concrete JS::ubi::StackFrame specialization backed by a frame deserialized from a core dump

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

::: js/src/vm/SavedFrame.h
@@ +125,5 @@
> +        this->refcount = 1;
> +    }
> +
> +    static ReconstructedSavedFramePrincipals IsSystem;
> +    static ReconstructedSavedFramePrincipals NotIsSystem;

You prefer NotIsSystem to IsNotSystem? It doesn't matter to me.

::: js/src/vm/SavedStacks.cpp
@@ +449,5 @@
> +SavedFrameSubsumedByCaller(JSContext* cx, HandleSavedFrame frame)
> +{
> +    auto subsumes = cx->runtime()->securityCallbacks->subsumes;
> +    if (!subsumes)
> +        return true;

Strictly speaking, this check isn't necessary until just before the last line of the function. Whether it's actually useful to be able to run with a null subsumes check against deserialized snapshots, I kinda doubt.

@@ -743,3 @@
>          js::RootedSavedFrame parent(cx);
>          do {
> -            MOZ_ASSERT_IF(subsumes, (*subsumes)(principals, frame->getPrincipals()));

Wait, *was* there a reason why sometimes this would run with a null subsumes? I'm just wondering about the MOZ_ASSERT_IF.

@@ +1468,5 @@
> +        }
> +
> +        auto principals = ubiFrame.get().isSystem()
> +            ? &js::ReconstructedSavedFramePrincipals::IsSystem
> +            : &js::ReconstructedSavedFramePrincipals::NotIsSystem;

This would be easier to read as &js::ReconstructedSavedFramePrincipals.getSingleton(ubiFrame.get().isSystem). Or some other name.

Similarly, it'd be nice if there were a static 'is' function that checked against the two singletons.
Attachment #8649608 - Flags: review?(sphink) → review+
IIRC, the check for subsumes was from before the shell had principals and a subsumes callback. I'll test removing them in a follow up.
Comment on attachment 8651134 [details] [diff] [review]
Part 3: Implement a concrete JS::ubi::StackFrame specialization backed by a frame deserialized from a core dump

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88eeb038dff2
Attachment #8651134 - Flags: review+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: