Closed Bug 1083456 Opened 10 years ago Closed 10 years ago

[jsdbg2] Debugger.Memory.prototype.takeCensus doesn't find all the roots

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jimb, Assigned: fitzgen)

References

Details

Attachments

(2 files, 8 obsolete files)

Debugger.Memory.prototype.takeCensus does not find objects reachable only via the stack, PersistentRooted instances, or XPCOM objects.

It should use JS_TraceRuntime to gather rooted objects in the target zones, and treat those both as things to be counted, and start nodes for the traversal.
It would be nice to have a fake 'root list' JS::ubi::Node type, that could be pre-loaded with a select group of ubi::Nodes and edge names, and reports them as its edges. We could have a utility function that calls JS_TraceRoots to populate such a list, and provide an interface to manually add entries.

This would mesh nicely with the BreadthFirst algorithm, which doesn't report start nodes to the handler unless they're, in fact, reachable from themselves --- another reason takeCensus is currently broken.
Assignee: nobody → nfitzgerald
Blocks: 1024774
Status: NEW → ASSIGNED
Jim, I feel I have a solid understanding of how to get the roots from JS_TraceRuntime and accumulate that into a SimpleEdgeVector. Got a great little FilteringTracer<Predicate, Tracer> template going to build the debuggee zone edge filtering stuff on top of in a nice way.

However, I'm getting a bogged down and lost writing the glue code to integrate this into ubi::Node. I'm feeling a bit overwhelmed by all the inheritance, composition, CRTP, traits templates, etc all coming together at once in a giant tempest of C++-y polymorphism.

Originally, I was trying to just trace the edges, accumulate them in a vector and pass that to a ubi::Node/Base subclass whose edge range would just iterate over that. Doesn't work because Node shouldn't be extended, just Base, and Base subclasses can't have extra data members. Plus, lifetime of the traced edges gets really wacky since ubi::Node is supposed to be a value type.

Then I start thinking: "well, what I really want at a conceptual level is a ubi::Node for the JSRuntime, so why not make a Concrete<JSRuntime> specialization?" This seems pretty good but (a) it forces us to do the edges-filtering by hand everytime we want to use the root node because we can't just use the DebuggeeZoneSetRootNode, and (b) we need to trace the runtime's edges immediately right off the bat (rather than waiting for edges() to be called) so that we get that required nursery eviction out of the way before we start using ubi::Nodes.

So, now I'm thinking of creating some kind class that is just a wrapper over the root edges (which can be found by the somewhat-existing FilteringTracer I mentioned above) and then having a Concrete<> specialization for this root-edges-wrapping class. Then you would use it something like this:

  RootEdgesWrapper edges(getRootDebuggeeEdges(debuggees));
  ubi::Node root(&edges);
  doTraversalsAndStuff(root);

I had previously wanted a function that just returned a ubi::Node, but that seems like it doesn't jive well with cleaning up the root edges when we are done.

Maybe I can just use SimpleEdgeVector as the hypothetical RootEdgesWrapper class and have a Concrete<SimpleEdgeVector> specialization?

Is this making sense? Am I on the right path, or going off into the weeds?
Flags: needinfo?(jimb)
Gah, but if we pre-compute the edges, then we don't know whether to generate names for the edges! Hm...
Maybe it is OK to call JS_TraceRuntime in edges() because it will be before we get any other ubi::Nodes? But will the rooting/hazard analysis complain?
Talked to Jim on irc. Going to do something similar to the RootEdgesWrapper approach I mentioned above.
Flags: needinfo?(jimb)
Attached patch fictional-root.patch (obsolete) — Splinter Review
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=21ec39c3fa23

Couldn't think of other good ways to test this stuff. Suggestions welcome.

Going to try and write one for cross compartment wrapper roots, but I don't think that should hold up initial review.
Attachment #8517603 - Flags: review?(jimb)
For js::gc::GCRuntime::markRuntime, traceOrMark defaults to TraceRuntime which skips CCWs (and so my CCW test is failing right now). It makes sense to skip CCWs when tracing every zone, but not when we are only doing a subset. We should probably be more discriminate on how we trace the runtime (and which TraceOrMark flag we pass) based on which version of RootList::init is called.
Attached patch fictional-root-pt-1.patch (obsolete) — Splinter Review
Attachment #8517603 - Attachment is obsolete: true
Attachment #8517603 - Flags: review?(jimb)
Attachment #8517841 - Flags: review?(terrence)
Attached patch fictional-root-pt-2.patch (obsolete) — Splinter Review
Attachment #8517842 - Flags: review?(jimb)
Comment on attachment 8517841 [details] [diff] [review]
fictional-root-pt-1.patch

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

::: js/src/jsapi-tests/testGCMarking.cpp
@@ +63,5 @@
> +
> +    void *thing = wrapper.get();
> +    CCWTestTracer trc(cx, &thing, JSTRACE_OBJECT);
> +    JS_TraceIncomingCCWs(&trc, zones);
> +    CHECK(trc.numberOfThingsTraced == 1);

Ah, I need to `CHECK(trc.okay);` here. Will wait to upload a new patch until after I get the rest of your review comments.
Comment on attachment 8517841 [details] [diff] [review]
fictional-root-pt-1.patch

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

Beautiful!

::: js/public/TracingAPI.h
@@ +221,5 @@
> +
> +// Trace every value within |zones| that is wrapped by a cross-compartment
> +// wrapper from a zone that is not an element of |zones|.
> +extern JS_PUBLIC_API(void)
> +JS_TraceIncomingCCWs(JSTracer *trc, JS::ZoneSet &zones);

It should be fine to make |zones| const as we're only reading from it.

::: js/src/jsapi-tests/testGCMarking.cpp
@@ +29,5 @@
> +        okay(true),
> +        numberOfThingsTraced(0),
> +        expectedThingp(expectedThingp),
> +        expectedKind(expectedKind)
> +    { };

No ; required after method definition.
Attachment #8517841 - Flags: review?(terrence) → review+
Attached patch fictional-root-pt-1.patch (obsolete) — Splinter Review
Thanks for the quick review, Terrence!

Fixed up and carrying over r+.
Attachment #8517841 - Attachment is obsolete: true
Attachment #8518261 - Flags: review+
Attached patch fictional-root-pt-1.patch (obsolete) — Splinter Review
Woops, I messed up generating that last patch...
Attachment #8518261 - Attachment is obsolete: true
Attachment #8518266 - Flags: review+
Adding namespace prefix so certain (non-unified?) builds don't complain.

Try push for part 1 only: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=43752fd111ab
Attachment #8518266 - Attachment is obsolete: true
Attachment #8519093 - Flags: review+
Comment on attachment 8519093 [details] [diff] [review]
fictional-root-pt-1.patch

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

::: js/public/UbiNode.h
@@ +16,5 @@
>  #include "jspubtd.h"
>  
>  #include "js/GCAPI.h"
>  #include "js/HashTable.h"
> +#include "js/TracingAPI.h"

Reviewing the wrong patch, but:

Does UbiNode.h really need to include js/TracingAPI.h any more? If not, could we take that out?
Comment on attachment 8517842 [details] [diff] [review]
fictional-root-pt-2.patch

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

Lurvely.

A few comments; r=me with those addressed.

::: js/public/UbiNode.h
@@ +476,5 @@
> +//        JS::ubi::RootList rootList(cx);
> +//        if (!rootList.init(cx, maybeNoGC))
> +//            return false;
> +//
> +//        // The AutoCheckCannotGC is gauranteed to exist if init returned true.

"guaranteed"

@@ +494,5 @@
> +    bool init(JSContext *cx, Maybe<AutoCheckCannotGC> &gcp);
> +    // Find only GC roots in the provided set of |Zone|s.
> +    bool init(JSContext *cx, ZoneSet &debuggees, Maybe<AutoCheckCannotGC> &gcp);
> +    // Find only GC roots in the given Debugger object's set of debuggee zones.
> +    bool init(JSContext *cx, HandleObject debuggees, Maybe<AutoCheckCannotGC> &gcp);

Should RootList be MOZ_STACK_CLASS?

We need gcp's lifetime to extend beyond the RootList's. For example, code like this is incorrect:

  RootList rootList(cx);
  {
    Maybe<JS::AutoCheckCannotGC> mcgc;
    ... rootList.init(cx, mcgc) ...
  }
  ... use rootList ...

One way to help enforce this is to let RootList's constructor take the M<ACCGC> &, use that to initialize a private member of that same type, and then omit the M<ACCGC> & from the init methods' arguments. Then, you would be forced to have the M<ACCGC> before you declare the RootList.

Also, we definitely need to explain what's going on with gcp. Perhaps something like:

RootList::init itself causes a minor collection, but once the list of roots has been created, GC must not occur, as the referent ubi::Nodes are not stable across GC. The init calls emplace |gcp|'s AutoCheckCannotGC, whose lifetime must extend at least as long as the RootList itself.

::: js/src/vm/Debugger.h
@@ +441,5 @@
>  
>      bool hasMemory() const;
>      DebuggerMemory &memory() const;
>  
> +    const GlobalObjectSet::Range allDebuggees() const { return debuggees.all(); }

I don't think a 'const' qualifier on GlobalObjectSet::Range matters here, because the result is a copy, not a reference: no harm can come from mutating it. (The 'const' for 'this', of course, is appropriate.)

::: js/src/vm/DebuggerMemory.cpp
@@ +767,5 @@
> +    Debugger *dbg = memory->getDebugger();
> +
> +    // Populate census.debuggeeZones and ensure that all of our debuggee globals
> +    // are rooted so that they are visible in the RootList.
> +    JS::AutoObjectVector debuggees(cx);

The way you're using 'debuggees' here is a nice hack. I think, though, in the long term, we ought to instead let one explicitly add edges to a RootList. Then we could give it a more helpful edge name like "debuggee global", rather than "js::AutoObjectVector.vector". But that can be a follow-up.

@@ +787,5 @@
>          if (!traversal.init())
>              return false;
>          traversal.wantNames = false;
>  
> +        if (!traversal.addStart(JS::ubi::Node(&rootList)) || !traversal.traverse())

Lovely.

nit: Could we break the line after the ||, so the central function call of the whole function gets its own line?

::: js/src/vm/UbiNode.cpp
@@ +343,5 @@
> +};
> +
> +const char16_t Concrete<RootList>::concreteTypeName[] = MOZ_UTF16("RootList");
> +
> +EdgeRange *Concrete<RootList>::edges(JSContext *cx, bool wantNames) const {

The return type gets its own line in SpiderMonkey.
Attachment #8517842 - Flags: review?(jimb) → review+
Attached patch fictional-root-pt-2.patch (obsolete) — Splinter Review
Carrying over r=jimb.

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b1d677af61f6
Attachment #8517842 - Attachment is obsolete: true
Attachment #8524014 - Flags: review+
Attached patch fictional-root-pt-2.patch (obsolete) — Splinter Review
Carrying over r=jimb.

Adding missing include that caused some builds to fail in the last try push.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=816bb79e8e32
Attachment #8524014 - Attachment is obsolete: true
Attachment #8524167 - Flags: review+
Attached patch fictional-root-pt-2.patch (obsolete) — Splinter Review
Carrying over r+.

Adding missing namespace qualification that caused some failures on some platforms in that last try push.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3c60b597ae4d
Attachment #8524167 - Attachment is obsolete: true
Attachment #8524247 - Flags: review+
Carrying over r=jimb.

Sigh, fixing another missing namespace qualification that causes failures on certain builds/platforms on try.

New try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=646ee0faee97
Attachment #8524247 - Attachment is obsolete: true
Attachment #8524290 - Flags: review+
Hi Nick, 

seems the patch don't apply cleanly:

applying fictional-root-pt-1.patch
patching file js/public/TracingAPI.h
Hunk #1 FAILED at 3
Hunk #2 FAILED at 206
2 out of 2 hunks FAILED -- saving rejects to file js/public/TracingAPI.h.rej
patching file js/public/UbiNode.h
Hunk #1 FAILED at 11
Hunk #2 FAILED at 129
2 out of 2 hunks FAILED -- saving rejects to file js/public/UbiNode.h.rej
patching file js/src/gc/Tracer.cpp
Hunk #1 FAILED at 12
Hunk #2 FAILED at 113
2 out of 2 hunks FAILED -- saving rejects to file js/src/gc/Tracer.cpp.rej
patching file js/src/gc/Zone.h
Hunk #1 FAILED at 10
Hunk #2 FAILED at 253
2 out of 2 hunks FAILED -- saving rejects to file js/src/gc/Zone.h.rej
patching file js/src/jsapi-tests/moz.build
Hunk #1 FAILED at 32
1 out of 1 hunks FAILED -- saving rejects to file js/src/jsapi-tests/moz.build.rej
file js/src/jsapi-tests/testGCMarking.cpp already exists
1 out of 1 hunks FAILED -- saving rejects to file js/src/jsapi-tests/testGCMarking.cpp.rej
patching file js/src/vm/DebuggerMemory.cpp
Hunk #1 FAILED at 10
Hunk #2 FAILED at 298
2 out of 2 hunks FAILED -- saving rejects to file js/src/vm/DebuggerMemory.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh fictional-root-pt-1.patch
Tomcats-MacBook-Pro:mozilla-inbound Tomcat$ 

could you take a look please, thanks!
Flags: needinfo?(nfitzgerald)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/84d6178e3be2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.