Debugger#findObjects doesn't find all expected objects when debugging all globals

RESOLVED FIXED in Firefox 48



4 years ago
4 years ago


(Reporter: till, Assigned: fitzgen)


Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)



(2 attachments, 3 obsolete attachments)

While implementing Promise in the JS engine, I encountered a problem where in test_promises_actor_list_promises.js the initial self-test in line 20 failed because it couldn't find the Promise created in the test's compartment. The actual test in lines 27-30 works fine, OTOH.

The test can be made to passed in two ways:
- by storing a reference to the Promise in a top-level var (or let, const).
- by disabling the traversal.abandonReferent() optimization in the Debugger's ObjectQuery::operator().

Further testing revealed that the same issue occurrs with RegExp objects, so my theory is that the test currently passes because Promise is cycle-collected in the current implementation, and that somehow makes it reachable here, while GC'd objects aren't.
This is a test case that demonstrates this failing for RegExp objects. I tried creating one that works in the JS shell, but failed to do so. Presumably xpconnect-provided wrappers play a role here.

I also have results from explorations that show that the path to the missing object would be through 25 hops, the first 18 of which we abandon with the abandonReferent optimization. Not applying the optimization on Proxy, Object, Array, and Set makes the test pass.

Nick, is this something you could look at? I'm stumped and feel like I'd have to learn about fairly large amounts of code to make progress on this. I do have the above-mentioned exploration results in the form of (obj,classname,compartment,zone)-(obj,classname,compartment,zone) edges and whether we'd abandon them. Ping me if that's useful to have.
Attachment #8727360 - Flags: feedback?(nfitzgerald)
Comment on attachment 8727360 [details] [diff] [review]
Test case for findObjects on all-compartments debugger

Review of attachment 8727360 [details] [diff] [review]:

Thanks for the test case, that will help. I will try and dig into this in the next week or so.
Attachment #8727360 - Flags: feedback?(nfitzgerald)
Flags: needinfo?(nfitzgerald)
I commented out the abanadonReferent() branch in ObjectQuery::findObjects and the test still fails. This is troubling.
Flags: needinfo?(nfitzgerald)
Oh nevermind, it was an unrelated assertion.
Edges which are traced via TraceCrossCompartmentEdges() are not showing up as roots in JS::ubi::RootList when they should. We rely on the CrossCompartmentWrapperMap for finding cross compartment edges in JS::ubi::RootList. Below is the only caller of TraceCrossCompartmentEdges. I'm not one hundred percent sure why this is such a special snowflake...
Ok so TraceCrossCompartmentEdges is for (maybe) tracing *outgoing* cross-compartment edges (depending on if the referent is in our set of compartments we want to trace).

So we are either (a) missing an entry in the wrapper map, or (b) JS_TraceIncomingCCWs (used by JS::ubi::RootList and *not* the main GC tracing path) is too selective about the edges it follows and is missing entries. (b) seems more likely to me.
It is definitely not the (a) case: the wrapper map entry exists.
There can be multiple compartments within the same zone, only one of which is a
debuggee. In this scenario, CCWs from other compartments into the debuggee
compartment should be traced and treated as roots. Therefore, dealing with CCWs
at the JS::Zone level is incorrect, and this patch changes the granularity level
to JSCompartments. If you look at the callers and uses of the function, it makes
much more sense now.

Additionally, it renames `JS_TraceIncomingCCWs` to `JS::TraceIncomingCCWs`.

Try push:

With this patch applied, the RegExp object from the other attachment on this bug
is found.
Attachment #8729720 - Flags: review?(jimb)
Forgot to re-build and re-run the gtests with the last patch. And, of course,
they were broken (as the previous try push showed). This patch fixes the gtests,
which also involved renaming them since they are testing cross-compartment
sub-graphs rather than cross-zone sub-graphs now.

New try push:
Attachment #8730269 - Flags: review?(jimb)
Attachment #8729720 - Attachment is obsolete: true
Attachment #8729720 - Flags: review?(jimb)
Comment on attachment 8729720 [details] [diff] [review]
TraceIncomingCCWs should work at the JSCompartment level of granularity

Review of attachment 8729720 [details] [diff] [review]:

Looks great!

::: js/public/TracingAPI.h
@@ +325,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)
> +TraceIncomingCCWs(JSTracer* trc, const JS::CompartmentSet& compartments);

Need to fix the comment as well.

::: js/src/gc/Tracer.cpp
@@ +131,4 @@
>  {
> +    for (js::CompartmentsIter c(trc->runtime(), SkipAtoms); !c.done(); {
> +        JSCompartment* comp = c.get();
> +        if (!comp || compartments.has(comp))

The other uses of CompartmentsIter I see don't check whether c.get() is nullptr.

Also, the other uses seem to behave as if CompartmentsIter coerces implicitly to JSCompartment *; see, for example, js::NukeCrossCompartmentWrappers. So perhaps `comp` isn't necessary at all, and we can just use `c` everywhere.
Attachment #8729720 - Attachment is obsolete: false
Assignee: till → nfitzgerald
Attachment #8729720 - Attachment is obsolete: true
Attachment #8730269 - Attachment is obsolete: true
Attachment #8730269 - Flags: review?(jimb)
Attachment #8730337 - Flags: review?(jimb) → review+
Turns out we still need to consider Zones inside RootList::init for when we get
things that do not have compartments, but do have zones that are not any of our
debuggee compartments' owning zones. This was why the tests were failing (yay
tests that do their job!)

New try push:
Attachment #8730432 - Flags: review+
Attachment #8730337 - Attachment is obsolete: true
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.