1) We need to specify a format and create a library for making compact core dumps. 2) Next, we need to be able to deserialize core dumps into ubi::Node instances and provide a set of Debugger.Memory APIs on top of these deserialized ubi::Nodes. 3) Finally, we need to glue everything together: traverse the heap with ubi::Node and save the core dump to temporary storage on the debuggee device, then stream the core dump across the RDP to the debugger machine where we can deserialize and call those APIs we made above. ------------------------------------------------------ Skeleton of a core dump format: # Attributes * Attributes are names of a field in a structure. * Examples: node type, edge, compartment # Form * A specific representation of a value * Examples: null terminated string, 32 bit word, 64 bit word, variable length integer, block of bytes preceded by count # Entry * An actual thing in the heap. * Consists of: a) Shape number b) n times: * If it's the first time this shape number has occured in the core dump: - attr code - form code - value form * Else: -value form c) END sentinel value The shape number could have a "has children" bit and then any entries following an entry that has this bit set are considered children of that entry until we reach the END_CHILDREN sentinel value. This could be useful for describing a node's edges, for example.

(In reply to Nick Fitzgerald [:fitzgen] from comment #0) > c) END sentinel value Note that this only need appear the first time the shape number is used. After that, we know when the values end. (All forms must be self-terminating: a fixed width, an explicit count, or a null terminating byte.)

Would also be nice if we could implement a nice little serialization DSL like boost's: http://www.boost.org/doc/libs/1_55_0/libs/serialization/doc/index.html

Working on docs, at least for now, so I will take this for the mean time.

Here is a work in progress, but I think enough is there to get the idea. I'm starting to lean towards just using protobufs (which are already in the tree), and I'm going to investigate that now.

> TODO how many bits for these things? The "right" length for integers is always a tough question. Any fixed length is always wrong: the size has to accommodate the largest value you think might occur, which guarantees that the more significant bytes will usually be zero, and hence wasted. Both DWARF and protobufs use a variable-length integer encoding: https://developers.google.com/protocol-buffers/docs/encoding#varints (Amazingly, I believe they use exactly the same encoding, although I haven't read closely enough to be sure.)

So, the big difference between protobufs and our encoding is that protobufs repeat the shape metadata in every message, instead of just citing a previously established shape. The first version of DWARF did the same thing, and they found that the metadata took up more space than was acceptable. (I heard this from Michael Eager, but DWARF's Wikipedia page says the same thing.) Now, that's clearly a function of the kind of data DWARF was storing (the number of shapes was very small relative to the number of nodes), and perhaps even on the specific predilections of the producers. Now, tradeoffs have changed; storage is a lot cheaper than it was, and bandwidth is a lot better. I think the cost of implementing custom serialization (it's actually getting it tested properly that gives me pause) is high enough that it would make sense to give protobufs a chance, and then revise if the size (or, specifically, the metadata size) turns out to be relevant to developer experience. But I'm definitely still interested in your impressions after having read over the protobufs spec.

I wonder whether it's necessary to include the NodeId at the head of each node. Can't we just number them implicitly?

One simplification we could make would be to drop the node / edge distinction, calling everything an 'entry', and let entries have children. Then, edges would simply be the children of a node, and the referent would simply be the child of an edge. I think the type vocabulary could be simplified a lot. It's not important these days to have values the processor can just pluck out of the stream directly. We need signed and unsigned varints; bools; UTF-8 strings; node references, and that's it. Yes, we should specify the byte order. The nice thing about varints is that they are explicitly little-endian.

Ok this adds the IDL interface and skeleton of the class, renames the jsinspector xpcom module to devtools_server, and does parameter validation. Just want to get some early feedback, mostly on the module changes and interface glue stuff.

Comment on attachment 8496266 [details] [diff] [review] heap-snapshots.patch I've switchded from XPIDL to WebIDL, and a bunch of this patch is obsolete now.

Ok this does the heap graph traversal and converts everything into protobuf messages (although incomplete ones), but doesn't write the core dump to disk yet.

Ok this can write a heap snapshot to a core dump, but I don't have a concrete specialization that is backed by the core dump yet.

This is just the interface.

:mmc, can you give feedback on the use of protobufs in CoreDump.* and HeapSnapshotManager.*? Thanks!

Comment on attachment 8526236 [details] [diff] [review] Part 1: HeapSnapshotManager.webidl Review of attachment 8526236 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Bindings.conf @@ +461,5 @@ > 'headerFile': 'nsGeolocation.h' > }, > > +'HeapSnapshotManager': { > + 'concrete': False, I don't think this is right. ::: dom/webidl/HeapSnapshotManager.webidl @@ +6,5 @@ > + > +/** > + * A class for saving heap snapshots. > + */ > +[ChromeOnly, Exposed=(Window,System,Worker)] We need test coverage for each type of global this is exposed on, including window and workers. @@ +35,5 @@ > + * > + * * debuggees: Debugger object > + * Like "globals", but use the Debugger's debuggees as the globals. > + * > + * * runtime: true What do you mean by "true" here? It will default to false I think. @@ +48,5 @@ > + * incoming JS reference to the selected Zones. > + */ > +dictionary HeapSnapshotBoundaries { > + any globals; > + object debuggees; Why are these not sequences?

Comment on attachment 8526240 [details] [diff] [review] Part 2: Serialize heap snapshots to core dumps Review of attachment 8526240 [details] [diff] [review]: ----------------------------------------------------------------- I looked at HeapSnapshotManager and the protobuf stuff. The protobuf stuff looks fine, so does the approach of flushing each node during traversal. ::: toolkit/devtools/server/CoreDump.proto @@ +1,1 @@ > +option optimize_for = LITE_RUNTIME; I could do with more comments in this file. It should also probably have the usual preamble and license header. @@ +5,5 @@ > +message Metadata { > + optional uint64 timeStamp = 1; > +} > + > +message Node { At some point you may regret having multiple classes named "Node" @@ +8,5 @@ > + > +message Node { > + optional uint64 id = 1; > + // char16_t * > + optional bytes typeName = 2; why Node.typeName() and Edge.name()? I see, it's consistent with ubinode stuff. @@ +14,5 @@ > + repeated Edge edges = 4; > +} > + > +message Edge { > + optional uint64 referent = 1; Is this an id? If so, please include a comment or rename referentId ::: toolkit/devtools/server/HeapSnapshotManager.cpp @@ +132,5 @@ > + * globals, find the set of zones the globals allocate within. Returns false on > + * failure. > + */ > +static bool > +PopulateZonesWithGlobals(JSContext *cx, cx is unused @@ +151,5 @@ > +{ > + const char16_t *ss = s; > + while (*ss) > + ss++; > + return ss - s; This gives you 0 if s is null. Is that a bug? @@ +277,5 @@ > + if (!boundaries.mRuntime.WasPassed()) { > + if (!PopulateGlobalsWithBoundaries(cx, boundaries, globals) || > + !zones.init() || > + !PopulateZonesWithGlobals(cx, globals, zones)) > + { I don't understand the cuddle bracket heuristic here... @@ +303,5 @@ > + } > + > + protobuf::Metadata metadata; > + // TODO FITZGEN: figure out how the hell to get the time stamp. > + // metadata.set_timestamp(); gettimeofday? Maybe not Windows friendly. https://mxr.mozilla.org/mozilla-central/source/security/pkix/lib/pkixtime.cpp#36 ::: toolkit/devtools/server/HeapSnapshotManager.h @@ +23,5 @@ > + * Populate a `protobuf::Node` protobuf message with the given > + * `JS::ubi::Node`. Returns false on failure. > + */ > +extern bool PopulateProtobufNode(JSContext *cx, > + JS::ubi::Node ubiNode, pass by reference, unless it's going to change out from under you? @@ +36,5 @@ > +class MOZ_STACK_CLASS HeapSnapshotHandler { > + JSContext *cx; > + bool wantNames; > + CoreDump &coreDump; > + JS::ZoneSet *zones; I wonder if you want a maxNodes or maxSize here in case things go horribly wrong. Or maybe the traversal code handles that? @@ +97,5 @@ > + * A `CoreDump` class serializes and accumulates protobuf messages to disk, or > + * memory, or network, etc. It class must implement the following public > + * method: > + * > + * bool serialize(::google::protobuf::MessageLite &message); It may be more useful to return the size of the serialized string.

Comment on attachment 8526240 [details] [diff] [review] Part 2: Serialize heap snapshots to core dumps Review of attachment 8526240 [details] [diff] [review]: ----------------------------------------------------------------- Review in progress... ::: js/public/UbiNode.h @@ +477,5 @@ > typedef mozilla::Vector<SimpleEdge, 8, js::TempAllocPolicy> SimpleEdgeVector; > > +// An EdgeRange concrete class that holds a pre-existing vector of SimpleEdges. > +class PreComputedEdgeRange : public EdgeRange { > + SimpleEdgeVector &edges; Who owns this vector? This class won't free it: it only owns a reference to the vector, not the vector itself. ::: js/public/UbiNodeTraverse.h @@ +120,5 @@ > MOZ_ASSERT(!traversalBegun); > traversalBegun = true; > > + // While there are pending nodes, visit them, until we've found a path > + // to the target. Thanks. ::: toolkit/devtools/server/CoreDump.proto @@ +2,5 @@ > + > +package mozilla.devtools.protobuf; > + > +message Metadata { > + optional uint64 timeStamp = 1; You've got to specify units and epoch here. Seconds since 1970? Milliseconds? Pity the poor hacker trying to read this file to understand the format.

Comment on attachment 8526240 [details] [diff] [review] Part 2: Serialize heap snapshots to core dumps Review of attachment 8526240 [details] [diff] [review]: ----------------------------------------------------------------- More in-progress comments: ::: js/public/Debug.h @@ +303,5 @@ > > > // Return true if the given value is a Debugger object, false otherwise. > JS_PUBLIC_API(bool) > +IsDebugger(const JSObject *obj); nit: For pointers that aren't allowed to be null, SpiderMonkey encourages the use of references, as a convention. (Exceptions: JSRuntime *, the ubiquitous JSContext *, and pointers that represent the start of an array.) @@ +308,5 @@ > + > +// Append each of the debuggee global objects observed by the Debugger object > +// |dbgObj| to |vector|. Returns true on success, false on failure. > +JS_PUBLIC_API(bool) > +GetDebuggeeGlobals(JSContext *cx, const JSObject *dbgObj, AutoObjectVector &vector); Same here. ::: js/src/vm/Debugger.cpp @@ +7261,2 @@ > { > + return obj && js::Debugger::fromJSObject(obj) != nullptr; Doesn't this assert if obj isn't a Debugger object, instead of returning false? @@ +7269,5 @@ > + js::Debugger *dbg = js::Debugger::fromJSObject(dbgObj); > + > + if (!vector.reserve(vector.length() + dbg->debuggees.count())) > + // TODO FITZGEN: do I need to report the OOM on cx here? If so, how do I > + // avoid overwriting that in HeapSnapshotManager? In reporting failures, SpiderMonkey makes a distinction between simple containers (Vector; HashMap; etc.) that return false on OOM but don't report anything on the context; versus, um, Proper SpiderMonkey Fallible functions, which take a JSContext * and from which a false return means that an error *has been reported on the cx* if appropriate. In the present case: 1) The vector.reserve call is in the first category, so it's not going to report the OOM. 2) You need to decide which category GetDebuggeeGlobals falls in; I recommend the second. If that's the case, then it must report the OOM, and its callers should assume that a false return means that *cx has been set properly. In that case, it would almost always be a bug for a caller to overwrite cx's exception when GetDebuggeeGlobals returns false.

Comment on attachment 8526240 [details] [diff] [review] Part 2: Serialize heap snapshots to core dumps Review of attachment 8526240 [details] [diff] [review]: ----------------------------------------------------------------- This a good start, but I think we want to do things a bit differently. The code to handle the boundaries could be simplified a lot. We walk the array of globals once to check it, then walk it again to get globals, then walk those globals to get zones, and then possibly pass that to the RootList init; all this could be a single loop, eliminating the need for any temporary list of globals. mRuntime.WasPassed() is checked in four different places. Instead, let's have a single function: /** * Choose roots and limits for a traversal, given |boundaries|. * Set |roots| to the set of nodes within the boundaries that are referred to by * nodes outside. If |boundaries| does not include all JS zones, initialize * |zones| to the set of included zones; otherwise, leave |zones| uninitialized. * (You can use zones.initialized() to check.) * * If |boundaries| is incoherent, or we encounter an error while trying to * handle it, or we run out of memory, set |rv| appropriately. */ bool EstablishBoundaries(JSContext *cx, ErrorResult &rv, const HeapSnapshotBoundaries &boundaries, RootList &roots, ZoneSet &zones); I think this is going to end up being not a short function, but in the end much simpler than the series we go through now. I'm concerned about the out-of-memory handling, too. If the call to edges.append in RootList::init(JSContext *, ZoneSet &) fails, for example, it looks to me like that shows up as an NS_ERROR_UNEXPECTED result from SaveSnapshot. Shouldn't we return NS_ERROR_OUT_OF_MEMORY? My other concern is that I don't think we should use template parameters the way HeapSnapshotHandler and SerializeToCoreDump do now. I've put more specific comments by the code. ::: toolkit/devtools/server/CoreDump.proto @@ +6,5 @@ > + optional uint64 timeStamp = 1; > +} > + > +message Node { > + optional uint64 id = 1; The choice of id is interesting. If we assigned nodes sequentially increasing ids as we visited them (we'd just make HeapSnapshotHandler<T>::NodeData hold a size_t), then the variable-length integer encoding would save us a lot of space, compared to addresses, which will often use 48 bits. But if we could recognize object relocation and address re-use (a big 'if'), then using ubi::Node::identifier here would let us compare heaps. I'm inclined to say, let's assign sequential ids for now, and worry about providing ids that persist across snapshots once we have consumers for such, and solutions to the problems I mentioned above. @@ +7,5 @@ > +} > + > +message Node { > + optional uint64 id = 1; > + // char16_t * I would write |char16_t[]| here and in Edge, as there's no indirection involved. ::: toolkit/devtools/server/HeapSnapshotManager.cpp @@ +27,5 @@ > + > +/** > + * Validate the boundaries object. Returns false on unexpected > + * failure. Otherwise, `*validp` is true if the boundaries object is valid, and > + * false if it is invalid. Bobby suggested making the globals a .webidl sequence. How does that show up in the C++? If it's no longer a JavaScript array object, then perhaps this function wouldn't have any more "unexpected failure" conditions, and it could just return true iff the boundary is valid. @@ +80,5 @@ > + *validp = numPropsFound == 1; > + return true; > +} > + > +typedef mozilla::Maybe<AutoCheckCannotGC> MaybeAutoCheckCannotGC; If every user of RootList is going to wish they had this typedef, it seems like this probably belongs with the RootList declaration in the .h file. @@ +146,5 @@ > + return true; > +} > + > +size_t > +strlen16(const char16_t *s) We should use NS_strlen here. @@ +241,5 @@ > + bool result = message.SerializeToString(&str); > + NS_ENSURE_TRUE(result, false); > + > + int32_t nbytes = str.length(); > + int32_t amount = PR_Write(fd, str.data(), nbytes); PR_Write is an unbuffered 'write' system call. This is going to be very system-call-heavy. We should be writing to an nsIOutputStream, I would think. @@ +303,5 @@ > + } > + > + protobuf::Metadata metadata; > + // TODO FITZGEN: figure out how the hell to get the time stamp. > + // metadata.set_timestamp(); I think PR_Now is probably good. ::: toolkit/devtools/server/HeapSnapshotManager.h @@ +23,5 @@ > + * Populate a `protobuf::Node` protobuf message with the given > + * `JS::ubi::Node`. Returns false on failure. > + */ > +extern bool PopulateProtobufNode(JSContext *cx, > + JS::ubi::Node ubiNode, ubi::Node is meant to be usable as a value type, but it is two words long, so passing by const reference here is probably not a bad idea. @@ +32,5 @@ > + * A JS::ubi::BreadthFirst handler that serializes a snapshot of the heap into a > + * core dump. > + */ > +template<typename CoreDump> > +class MOZ_STACK_CLASS HeapSnapshotHandler { Rather than making this a template parameter, we should have: struct NodeWriter { virtual bool write(const JS::ubi::Node &node) = 0; }; and then have everyone take a pointer to that. We get better type checking and error messages; we can keep internal details in the .cpp where they belong; the mocking in the unit tests will be easier; and compared to the serialization, the virtual call overhead is insignificant. @@ +58,5 @@ > + > + // JS::ubi::BreadthFirst handler interface. > + > + class NodeData { }; > + bool operator() (JS::ubi::BreadthFirst<HeapSnapshotHandler> &traversal, Let's do: typedef JS::ubi::BreadthFirst<HeapSnapshotHandler> Traversal up towards the top. Then use that here, and say CoreDumpHandler::Traversal instead of the typedef down in SerializeToCoreDump. @@ +75,5 @@ > + if (zones) { > + // Don't serialize nodes that belong to a zone that is not an element of our > + // zone set. Do serialize nodes that are not associated with a zone, or are > + // associated with the atoms zone, on the assumption that they are shared > + // resources that our target zones are using. This is an interesting choice. It means that we'll get edges to nodes we didn't serialize, which is going to be a pain. And once we have better ubi::Node support for non-SpiderMonkey types, it means that we'll wander off into areas of the graph that aren't associated with any zone --- because we do serialize any node reachable through an arbitrary path of non-zone nodes. I think it would be better to serialize every node referenced by any edge we serialize; but omit outgoing edges of nodes that are not in a target zone. That way we have some data about everything our nodes of interest refer to; but when a referent is not of interest, we just don't have as much detail about it. @@ +115,5 @@ > + > + protobuf::Node protobufNode; > + NS_ENSURE_TRUE(PopulateProtobufNode(cx, node, wantNames, protobufNode), > + false); > + NS_ENSURE_TRUE(coreDump.serialize(protobufNode), false); We're not supposed to add new instances of NS_ENSURE_TRUE; see https://bugzilla.mozilla.org/show_bug.cgi?id=971673#c88 That also suggests an alternative. With NodeWriter, of course, this will turn into a single call to writer.write(...). @@ +132,5 @@ > +} > + > +// TODO fitzgen: this has to exist somewhere re-usable, already... can't use > +// js_strlen because it isn't exported... > +extern size_t strlen16(const char16_t *s); How about NS_strlen? ::: toolkit/devtools/server/generate-core-dump-sources.sh @@ +14,5 @@ > +fi > + > +if [ ! -e $PROTOC_PATH ]; then > + echo You must install the protocol compiler from > + echo https://code.google.com/p/protobuf/downloads/list If we require a very old version of protoc (as you've said on IRC), then this should document the version necessary, or point to where one can find the version of the protobuf-lite library that protoc must match.

Nick, since you're revising the main patch, I'm going to treat the other reviews as still needed, but lower priority for now. If that's not okay, let me know.

Comment on attachment 8555561 [details] [diff] [review] WIP: GZip serialized core dumps I'm getting linker errors with this patch: claims it can't find the GzipOutputStream's ctor's symbols. I'm looking into how the protobuf library is built in patch 0, but I haven't seen anything out of the ordinary yet.

Comment on attachment 8556601 [details] [diff] [review] Part 1: Add HeapSnapshotManager WebIDL interface. Review of attachment 8556601 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Bindings.conf @@ +446,5 @@ > }, > > +'HeapSnapshotManager': { > + # The codegen is dumb, and doesn't understand that this interface is only a > + # collection of static methods, so we have this `concrete: False` hack. Is there a bug on file for this?

Though actually, do we really want to be introducing entire WebIDL interfaces objects for trivial stuff like this? Wouldn't it make more sense to just have a single utils class or something? Smaug, what do you think?

Yeah, Part 1 doesn't look performance critical or anything. And the dictionary is rather simple, so saveSnapshot could just take all those dictionary members as parameters. sequence<object> globals; could be an nsIVariant... but sequence<> as nsIVariant is annoying to handle. So just because of that using .webidl would be ok to me. There is also the option to use .idl for saveSnapshot and have the disctionary, and then on C++ initialize the dictionary using a jsval. Do we really not have any helper interface for debugger stuff where saveSnapshot could be added?

The existing Debugger API is implemented in and exposed by SpiderMonkey, so there isn't a good existing WebIDL interface to expose this stuff on. I was following the pattern of the `PromiseDebugging` WebIDL interface. I suppose I could rename this interface to `DevTools` or something? Note that I am adding another method in later patches (patch 7), and potentially even more in the future.

(In reply to Nick Fitzgerald [:fitzgen] from comment #59) > The existing Debugger API is implemented in and exposed by SpiderMonkey, so > there isn't a good existing WebIDL interface to expose this stuff on. I was > following the pattern of the `PromiseDebugging` WebIDL interface. It seems like we should avoid proliferating these, and create something like ChromeUtils as a dumping ground for this stuff. We pay a per-interface in various places, so we should avoid adding new interfaces willy-nilly.

* pay a per-interface cost.

Comment on attachment 8560159 [details] [diff] [review] Part 7: Add HeapSnapshot WebIDL interface Review of attachment 8560159 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't seem to address comment 60.

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #71) > Comment on attachment 8560159 [details] [diff] [review] > Part 7: Add HeapSnapshot WebIDL interface > > Review of attachment 8560159 [details] [diff] [review]: > ----------------------------------------------------------------- > > This doesn't seem to address comment 60. I was going to rename "HeapSnapshotManager" to "DevToolsPlatformUtils" that can be shared for all devtools static functions (since we already have a DevToolsUtils js module), but I haven't done that yet. I don't see a way to avoid having a HeapSnapshot interface though, as this is going to be where we have methods for computing dominator trees, finding shortest paths from a root to an object, etc. What do you suggest in this case?

(In reply to Nick Fitzgerald [:fitzgen] from comment #72) > (In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect > things) from comment #71) > > Comment on attachment 8560159 [details] [diff] [review] > > Part 7: Add HeapSnapshot WebIDL interface > > > > Review of attachment 8560159 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This doesn't seem to address comment 60. > > I was going to rename "HeapSnapshotManager" to "DevToolsPlatformUtils" that > can be shared for all devtools static functions (since we already have a > DevToolsUtils js module), but I haven't done that yet. I think it should be even more general - is there any reason we can't just make something called ChromeUtils? It's not like these statics methods share state, so I don't see any reason why we need to separate them into separate interfaces. If we do DevToolsPlatformUtils, what happens when the Loop team needs to add a one-off method? > I don't see a way to avoid having a HeapSnapshot interface though, as this > is going to be where we have methods for computing dominator trees, finding > shortest paths from a root to an object, etc. Right - if this is for an actual concrete object, having an interface is fine.

Comment on attachment 8564356 [details] [diff] [review] Part 0: Upgrade the protobuf library. r=mmc Review of attachment 8564356 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure how or if configure is called from moz.build, but it doesn't seem to be. In that case we should not include any of its friends, either. ::: configure.in @@ +9136,5 @@ > AC_OUTPUT_SUBDIRS(modules/freetype2) > ) || exit 1 > fi > > + if test -z "$direct_nspr_config"; then This formatting change looks bogus. ::: toolkit/components/downloads/generate_csd.sh @@ +26,2 @@ > # Get the protocol buffer and compile it > +CMD='wget https://src.chromium.org/chrome/trunk/src/chrome/common/safe_browsing/csd.proto -O csd.proto' I think this link is now wrong since Chrome switched to gitiles. See https://bugzilla.mozilla.org/show_bug.cgi?id=1083085. What are the consequences of omitting the csd changes from this patch? If they don't include breaking backwards compatibility, the easiest thing to do is omit it for now and file a bug to take care of it later. ::: toolkit/components/protobuf/README.txt @@ +23,5 @@ > + `$MOZILLA_CENTRAL/toolkit/components/protobuf/src/google/**` except for test > + files and `$PROTOBUF/src/google/protobuf/compiler/**` files. > + > +3. Update the moz.build to export the new set of headers and add any new .cc > + files to the unified sources. If there aren't any manual steps required in 1. and 2., can we change the update script to do this automatically? ::: toolkit/components/protobuf/autom4te.cache/output.0 @@ +1,1 @@ > +@%:@! /bin/sh Is this file used in our build system? ::: toolkit/components/protobuf/autom4te.cache/requests @@ +1,3 @@ > +# This file was generated. > +# It contains the lists of macros which have been traced. > +# It can be safely removed. We should remove it if it's not being used. ::: toolkit/components/protobuf/autom4te.cache/traces.0 @@ +1,1 @@ > +m4trace:configure.ac:15: -1- AC_INIT([Protocol Buffers], [2.6.1], [protobuf@googlegroups.com], [protobuf]) Are we using this? ::: toolkit/components/protobuf/configure @@ +1,1 @@ > +#! /bin/sh Same question ::: toolkit/components/protobuf/configure.ac @@ +1,1 @@ > +## Process this file with autoconf to produce configure. Same question ::: toolkit/components/protobuf/src/google/protobuf/SEBS @@ +5,5 @@ > +# This is an experimental build definition file using the SEBS build system. > +# I (Kenton Varda, maintainer of Protocol Buffers) happen to be the author of > +# SEBS, though SEBS is not a Google project. I'm sticking this file in > +# protobuf's SVN because that's the easiest place for me to put it, and it > +# shouldn't harm anyone. This file is not included in the distribution. We shouldn't include this. ::: toolkit/components/protobuf/vs2013.patch @@ -1,1 @@ > -diff --git a/toolkit/components/protobuf/google/protobuf/wire_format_lite_inl.h b/toolkit/components/protobuf/google/protobuf/wire_format_lite_inl.h Did you check that this works with VS2013?

Comment on attachment 8564356 [details] [diff] [review] Part 0: Upgrade the protobuf library. r=mmc Review of attachment 8564356 [details] [diff] [review]: ----------------------------------------------------------------- I have nothing to add to Monica's comments. I'll just note that autom4te.cache files shouldn't even be in the upstream releases. If they are, they should fix their release scripts.

Comment on attachment 8566738 [details] [diff] [review] Part 2: Implement a google::protobuf::ZeroCopyOutputStream wrapper around nsIOutputStream Review of attachment 8566738 [details] [diff] [review]: ----------------------------------------------------------------- I get that protobuf just returns false when something goes wrong, but I think we can do a better job here propagating error codes out from the underlying nsIOutputStream to our callers. Let's change 'failed' from a bool to an nsresult, and add a public accessor function for it. writeBuffer would save the result from nsIOutputStream::write there. When a protobuf operation fails, we can check the underlying ZeroCopyNSIOutputStream's accessor to see if it can provide any more detail on the cause. 'flush' and 'writeBuffer' would return nsresult, and it would be in the 'Next' implementation that we throw away detail, as required by the interface it's implementing. ::: toolkit/devtools/server/ZeroCopyNSIOutputStream.cpp @@ +10,5 @@ > +namespace mozilla { > +namespace devtools { > + > +ZeroCopyNSIOutputStream::ZeroCopyNSIOutputStream(nsIOutputStream &out) > + : ZeroCopyOutputStream() I think you can leave this out when it has no arguments. @@ +23,5 @@ > +} > + > +ZeroCopyNSIOutputStream::~ZeroCopyNSIOutputStream() > +{ > + (void) writeBuffer(); I have suggestions elsewhere for how to avoid throwing away this error result; but if we don't go with that, this should at least NS_WARN_IF. @@ +44,5 @@ > + return true; > + > + uint32_t amountWritten = 0; > + while (amountWritten < amountUsed) { > + uint32_t justWritten = 0; size_t is generally the right type for any count of bytes in memory, but I see that nsIOutputStream::Write requires uint32_t. Oh well. @@ +46,5 @@ > + uint32_t amountWritten = 0; > + while (amountWritten < amountUsed) { > + uint32_t justWritten = 0; > + > + if (NS_WARN_IF(NS_FAILED(out.Write(reinterpret_cast<char *>(buffer) + amountWritten, If you make buffer a char[], as recommended below, then this can just be |buffer + amountWritten|, because arrays are converted to pointers to their first element. @@ +52,5 @@ > + &justWritten)))) > + return fail(); > + > + if (NS_WARN_IF(justWritten == 0)) > + return fail(); I can't find anything that says that a zero written count is an error. I think it is just an extreme case of "partial write". Is there a reason we shouldn't simply retry? @@ +58,5 @@ > + amountWritten += justWritten; > + } > + > + if (NS_WARN_IF(NS_FAILED(out.Flush()))) > + return fail(); We should only flush the underlying stream if we ourselves are flushed, it seems to me. @@ +83,5 @@ > + } > + > + *data = &buffer; > + *size = BUFFER_SIZE; > + amountUsed = BUFFER_SIZE; I think this isn't correct. If anyone calls BackUp, and then calls Next, then amountUsed will be less than BUFFER_SIZE on entry to this function; we won't call writeBuffer, and we'll hand out the same section of the buffer we did before. I think *data and *size need to reflect the portion of buffer after the first amountUsed bytes. ::: toolkit/devtools/server/ZeroCopyNSIOutputStream.h @@ +18,5 @@ > +// `nsIOutputStream` under the covers. > +// > +// This class will automatically write and flush its data to the > +// `nsIOutputStream` in its destructor, but if you care whether that call > +// succeeds or fails, then you should call the `flush` method yourself. I really dislike eating errors like this. It always wastes some poor schmuck's time. I considered recommending that the destructor assert that there is no unflushed data; but then abandoning a ZeroCopyNSIOutputStream because of some other error would cause assertions. Would it be feasible to have that assert, and add an "abandon" method that is called when one is deliberately dropping a ZeroCopyNSIOutputStream because of some other error? Are the streams' lifetimes controlled enough to make that practical? @@ +25,5 @@ > +{ > + static const int BUFFER_SIZE = 8192; > + > + // The nsIOutputStream we are streaming to. > + nsIOutputStream &out; This should probably be nsCOMPtr, right? @@ +28,5 @@ > + // The nsIOutputStream we are streaming to. > + nsIOutputStream &out; > + > + // The buffer we write data to before passing it to the output stream. > + uint8_t buffer[BUFFER_SIZE]; If you make this just |char buffer[BUFFER_SIZE]|, then you can avoid a bunch of casts everywhere. @@ +57,5 @@ > + // ZeroCopyOutputStream Interface > + virtual ~ZeroCopyNSIOutputStream() MOZ_OVERRIDE; > + virtual bool Next(void **data, int *size) MOZ_OVERRIDE; > + virtual void BackUp(int count) MOZ_OVERRIDE; > + virtual ::google::protobuf::int64 ByteCount() const MOZ_OVERRIDE; Spacing is weird here.

RootList's dependence on Maybe<AutoCheckCannotGC> (which is not understood by the static checker) made me sad, so I filed bug 1137389.

Comment on attachment 8566741 [details] [diff] [review] Part 3: Serialize heap snapshots. Review of attachment 8566741 [details] [diff] [review]: ----------------------------------------------------------------- Partial review; will actually look at the meat of the patch next. ::: js/public/UbiNode.h @@ +492,5 @@ > + { > + settle(); > + } > + > + void popFront() MOZ_OVERRIDE { i++; settle(); } Might want to assert !empty here. @@ +496,5 @@ > + void popFront() MOZ_OVERRIDE { i++; settle(); } > +}; > + > + > +typedef mozilla::Maybe<AutoCheckCannotGC> MaybeAutoCheckCannotGC; You probably wanted to use this typedef in the RootList definition, right? (I don't object! but I find these typedefs that save two characters and introduce a shadow of doubt as to their definition kind of annoying.) ::: js/public/UbiNodeTraverse.h @@ +120,5 @@ > MOZ_ASSERT(!traversalBegun); > traversalBegun = true; > > + // While there are pending nodes, visit them, until we've found a path > + // to the target. Since we're here, could you just drop the "until we've" clause altogether? There's no 'target' in BreadthFirst at all, so I think that comment is a vestige of its origins in the implementation of findPath. ::: js/src/vm/Debugger.cpp @@ +7686,5 @@ > return false; > + } > + > + for (GlobalObjectSet::Range r = dbg->debuggees.all(); !r.empty(); r.popFront()) > + vector.append(static_cast<JSObject *>(r.front())); This needs a read barrier. Filed as bug 1137478; full explanation there. Also, you should use Vector::infallibleAppend here. ::: toolkit/devtools/server/CoreDump.proto @@ +41,5 @@ > +// In practice, certain message fields have a lot of duplication (such as type > +// or edge name strings). Rather than try and de-duplicate this information at > +// the protobuf message and field level, core dumps should be written with > +// `google::protobuf::io::GzipOutputStream` and read from > +// `google::protobuf::io::GZipInputStream`. "GzipInputStream" (Oh, Jim, thank you so much for catching all these subtle bugs!) @@ +43,5 @@ > +// the protobuf message and field level, core dumps should be written with > +// `google::protobuf::io::GzipOutputStream` and read from > +// `google::protobuf::io::GZipInputStream`. > + > +// We only have the lite protobuf runtime in mozilla-central. Is this still true? I thought we had to drop the lite runtime to get the GzipOutputStream. ::: toolkit/devtools/server/generate-core-dump-sources.sh @@ +2,5 @@ > + > +# A script to generate toolkit/devtools/server/CoreDump.pb.{h,cc} from > +# toolkit/devtools/server/CoreDump.proto. This script assumes you have > +# downloaded and installed the protocol buffer compiler, and that it is either > +# on your $PATH or located at $PROTOC_PATH. When I ran toolkit/components/downloads/generate_csd.sh, I got confused about which directory I was supposed to run it in. It might be friendly to just "cd $(dirname $0)" at the top.

Comment on attachment 8566741 [details] [diff] [review] Part 3: Serialize heap snapshots. Review of attachment 8566741 [details] [diff] [review]: ----------------------------------------------------------------- This looks totally great. Some necessary changes noted, but r=me with those taken care of. ::: toolkit/devtools/server/ChromeUtils.cpp @@ +25,5 @@ > + > +namespace mozilla { > +namespace devtools { > + > +#define HEAP_SNAPSHOT_ERROR_MESSAGE_LENGTH 1024 This isn't used. @@ +32,5 @@ > +using namespace dom; > + > + > +// If we are only taking a snapshot of the heap affected by the given set of > +// globals, find the set of zones the globals allocate within. Returns false on "are allocated within" @@ +199,5 @@ > + , wantNames(wantNames) > + , stream(stream) > + { } > + > + virtual ~StreamWriter() MOZ_OVERRIDE { } We seem to be following SpiderMonkey style in this file; there, one includes either 'virtual' or 'MOZ_OVERRIDE' but not both. @@ +221,5 @@ > + mozilla::MallocSizeOf mallocSizeOf = dbg::GetDebuggerMallocSizeOf(rt); > + MOZ_ASSERT(mallocSizeOf); > + protobufNode.set_size(ubiNode.size(mallocSizeOf)); > + > + UniquePtr<ubi::EdgeRange, JS::FreePolicy> edges(ubiNode.edges(cx, wantNames)); Shouldn't this be inside the 'if (includeEdges)' too? @@ +222,5 @@ > + MOZ_ASSERT(mallocSizeOf); > + protobufNode.set_size(ubiNode.size(mallocSizeOf)); > + > + UniquePtr<ubi::EdgeRange, JS::FreePolicy> edges(ubiNode.edges(cx, wantNames)); > + if (!edges) NS_WARN_IF, like below? @@ +279,5 @@ > + { > + // We're only interested in the first time we reach edge.referent, not > + // in every edge arriving at that node. > + if (!first) > + return true; This isn't right, is it? We need all the edges, or else we can't do findPath or dominator tree analyses on the graph. @@ +300,5 @@ > + JS::Zone *zone = referent.zone(); > + > + if (zones->has(zone)) { > + return writer.writeNode(referent, CoreDumpWriter::INCLUDE_EDGES); > + } SpiderMonkey style says no braces when both 'if' and consequent are single lines. @@ +360,5 @@ > + > + nsCOMPtr<nsIOutputStream> outputStream; > + rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream), file, > + PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE, > + -1, 0); Those last two are the default values for those arguments, aren't they? @@ +381,5 @@ > + zones.initialized() ? &zones : nullptr, > + maybeNoGC.ref())) > + { > + rv.Throw(NS_ERROR_UNEXPECTED); > + return; Here's where we would check zeroCopyStream for any saved error code, if we make the changes I suggest in comment 91. ::: toolkit/devtools/server/ChromeUtils.h @@ +20,5 @@ > +namespace devtools { > + > +// A `CoreDumpWriter` is given the data we wish to save in a core dump and > +// serializes it to disk, or memory, or a socket, etc. > +class CoreDumpWriter This is really nice.

Comment on attachment 8566743 [details] [diff] [review] Part 4: Add an xpcshell test for saving heap snapshots. Review of attachment 8566743 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/tests/unit/test_SaveHeapSnapshot.js @@ +16,5 @@ > + testGoodParameters(filePath); > + do_test_finished(); > +} > + > +function testBadParameters(filePath) { How about: - boundaries.globals is not an object - boundaries.globals is not an array - boundaries.globals is an array containing a non-object - boundaries.debugger is Debugger.prototype - some boundaries property is a getter (what does WebIDL do with that?) @@ +54,5 @@ > +function testGoodParameters(filePath) { > + let sandbox = Cu.Sandbox(CC('@mozilla.org/systemprincipal;1', 'nsIPrincipal')()); > + const dbg = new Debugger(sandbox); > + ChromeUtils.saveHeapSnapshot(filePath, { debugger: dbg }); > + ok(true, "Should be able to save a snapshot for debuggee globals."); Debugger with multiple debuggees? Debugger with no debuggees?

Comment on attachment 8566748 [details] [diff] [review] Part 9: Add JS::ubi::TypeNameRegistry Review of attachment 8566748 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/UbiNode.cpp @@ +238,5 @@ > + TypeNameRegistry::Register(MOZ_UTF16("js::Shape")); > +template<> const char16_t *TracerConcrete<js::BaseShape>::concreteTypeName = > + TypeNameRegistry::Register(MOZ_UTF16("js::BaseShape")); > +template<> const char16_t *TracerConcrete<js::ObjectGroup>::concreteTypeName = > + TypeNameRegistry::Register(MOZ_UTF16("js::ObjectGroup")); We are not allowed to use static constructors in Firefox. Let's talk on IRC about how to accomplish what you're trying to do here.

(In reply to Jim Blandy :jimb from comment #96) > Comment on attachment 8566748 [details] [diff] [review] > Part 9: Add JS::ubi::TypeNameRegistry > > Review of attachment 8566748 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/vm/UbiNode.cpp > @@ +238,5 @@ > > + TypeNameRegistry::Register(MOZ_UTF16("js::Shape")); > > +template<> const char16_t *TracerConcrete<js::BaseShape>::concreteTypeName = > > + TypeNameRegistry::Register(MOZ_UTF16("js::BaseShape")); > > +template<> const char16_t *TracerConcrete<js::ObjectGroup>::concreteTypeName = > > + TypeNameRegistry::Register(MOZ_UTF16("js::ObjectGroup")); > > We are not allowed to use static constructors in Firefox. Let's talk on IRC > about how to accomplish what you're trying to do here. Ok, so Jim and I talked on IRC and came to the conclusion that we don't need to make DeserializedNodes is<T>() for the T that they represent a deserialized version of yet. Every use of is<T>() is a guard before an as<T>() which you can't do with a non-live ubi::Node. Instead, we should add ubi::Node methods/accessors for the various bits of info we as<T>() for right now, and then serialize and deserialize and re-expose that data with heap snapshots.

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #75) > ::: toolkit/components/downloads/generate_csd.sh > @@ +26,2 @@ > > # Get the protocol buffer and compile it > > +CMD='wget https://src.chromium.org/chrome/trunk/src/chrome/common/safe_browsing/csd.proto -O csd.proto' > > I think this link is now wrong since Chrome switched to gitiles. See > https://bugzilla.mozilla.org/show_bug.cgi?id=1083085. Updated. (Wow, gitiles is lame.) > What are the consequences of omitting the csd changes from this patch? If > they don't include breaking backwards compatibility, the easiest thing to do > is omit it for now and file a bug to take care of it later. Unfortunately, the old generated files won't work with this newer runtime, so we have to regenerate them. > ::: toolkit/components/protobuf/README.txt > @@ +23,5 @@ > > + `$MOZILLA_CENTRAL/toolkit/components/protobuf/src/google/**` except for test > > + files and `$PROTOBUF/src/google/protobuf/compiler/**` files. > > + > > +3. Update the moz.build to export the new set of headers and add any new .cc > > + files to the unified sources. > > If there aren't any manual steps required in 1. and 2., can we change the > update script to do this automatically? Added a fairly comprehensive upgrade script. > ::: toolkit/components/protobuf/vs2013.patch > @@ -1,1 @@ > > -diff --git a/toolkit/components/protobuf/google/protobuf/wire_format_lite_inl.h b/toolkit/components/protobuf/google/protobuf/wire_format_lite_inl.h > > Did you check that this works with VS2013? For some reason I thought that this got fixed upstream, but it appears it didn't. Folded this patch into m-c-changes.patch.

Comment on attachment 8570809 [details] [diff] [review] Part 0: Upgrade the protobuf library. r=mmc Review of attachment 8570809 [details] [diff] [review]: ----------------------------------------------------------------- rs=me ::: toolkit/components/protobuf/README.txt @@ +21,5 @@ > + > +2. Export the `$PROTOBUF_LIB_PATH` variable as the absolute path to that release > + checkout. > + > +3. Run `./toolkit/components/protobuf/upgrade_protobuf.sh`. It would probably be simpler to pass the path of the release checkout as an argument to the script, instead of having to set an environment variable.

(In reply to Nick Fitzgerald [:fitzgen] from comment #101) > Created attachment 8571605 [details] [diff] [review] > Part 0: Upgrade the protobuf library. Carrying over glandium r+. This version passes the protobuf path as an argument, and additionally has better usage error reporting.

Comment on attachment 8571605 [details] [diff] [review] Part 0: Upgrade the protobuf library. Review of attachment 8571605 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! LG pending green try.

Try push for part 0: https://treeherder.mozilla.org/#/jobs?repo=try&revision=938f961061d2

Trying to fix the compiler warnings that are causing build failures. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fda3a090506

Comment on attachment 8572080 [details] [diff] [review] Part 2: Implement a google::protobuf::ZeroCopyOutputStream wrapper around nsIOutputStream Review of attachment 8572080 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. r=me with minor comments addressed. ::: toolkit/devtools/server/ZeroCopyNSIOutputStream.cpp @@ +22,5 @@ > +} > + > +ZeroCopyNSIOutputStream::~ZeroCopyNSIOutputStream() > +{ > + NS_WARN_IF(NS_FAILED(writeBuffer())); Let's only try to writeBuffer if !failed(). Otherwise we're going to warn twice if a write to the underlying nsIOutputStream fails: once when the operation actually fails, and then again when we run the destructor and writeBuffer returns early. @@ +81,5 @@ > + MOZ_ASSERT(count > 0, > + "Must back up a positive number of bytes."); > + MOZ_ASSERT(amountUsed == BUFFER_SIZE, > + "Can only call BackUp directly after calling Next."); > + MOZ_ASSERT(uint32_t(count) < amountUsed, Not '<='?

Nother try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a947db2c9c9

Alright this latest version should hopefully fix the static analysis complaints about isNaN and the LOGLEVEL_0 compiler errors on windows. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c564e626976f