1082761 - [jsdbg2] Add an API to get all live objects that match some query (Debugger.prototype.findObjects)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

For example:

  let promises = dbg.findObjects({
    prototype: makeDebuggeeValue(debuggee.Promise.prototype)
  });

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: wip

WIP

Getting dupes (I think due to wrappers, but there are so many of them that maybe it's something else)

Doesn't seem to be throwing an exception properly on bad query parameters.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: find-by-prototype.patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d5cbb725e17f

Comment 3

[Jim Blandy :jimb]

Comment on attachment 8505487 [details] [diff] [review]
find-by-prototype.patch

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

Soooo lovely. Some comments. Two follow-up bugs.

::: js/public/UbiNodeTraverse.h
@@ +96,5 @@
>      bool addStart(Node node) { return pending.append(node); }
>  
> +    // Add |node| as a starting point for the traversal (see addStart) and also
> +    // add it to the |visited| set. Return false on OOM.
> +    bool addStartVisited(Node node) {

Filed follow-up bug 1083379 about the inconsistent OOM handling here.

::: js/src/doc/Debugger/Debugger.md
@@ +380,5 @@
>  
> +<code>findObjects([<i>query</i>])</code>
> +:   Return an array of [`Debugger.Object`][object] instances referring to each
> +    live object allocated in the scope of the debuggee globals matching
> +    *query*. Each instance appears only once in the array. *Query* is an object

How about "in the scope of the debuggee globals that matches *query*", to make it clearer that "matches query" qualifies "object" not "globals"?

@@ +390,5 @@
> +    The *query* object may have the following properties:
> +
> +    `class`
> +    :   If present, only return objects whose internal `[[Class]]`'s name
> +        matches the given string.

Could we add: "Note that in some cases, the prototype object for a given constructor has the same `[[Class]]` as the instances that refer to it, but cannot itself be used as a valid instance of the class. Code gathering objects by class name may need to examine them further before trying to use them."

@@ +396,5 @@
> +    All properties of *query* are optional. Passing an empty object returns all
> +    objects in debuggee globals.
> +
> +    Unlike `findScripts`, this function is deterministic and will never return
> +    [`Debugger.Object`s][object] referrencing unreachable objects that have not

s/referrencing/referring to/

::: js/src/jscompartment.h
@@ +202,5 @@
>      js::types::TypeCompartment   types;
>  
>      void                         *data;
>  
> +    js::WrapperMap               crossCompartmentWrappers;

Could you use the already-exposed JSCompartment::WrapperEnum constructor instead of making this public? That's very much like a js::WrapperMap::Range (Enums are just Ranges that have the ability to handle map mutations.)

::: js/src/vm/Debugger.cpp
@@ +3048,5 @@
> +
> +            /*
> +             * Iterate over all compartments and add traversal start points at
> +             * objects that have CCWs in other compartments keeping them alive.
> +             */

This rooting is inadequate --- as is the existing code in takeCensus. Filed as follow-up bug 1083456. No change needed in this bug.

@@ +3081,5 @@
> +                JSObject *obj = node.as<JSObject>();
> +
> +                if (!className.isUndefined()) {
> +                    const char *objClassName = obj->getClass()->name;
> +                    if (strncmp(objClassName, classNameCString.ptr(), classNameLength) != 0)

If I'm reading right, strncmp will match any class name of which the string given in the query is a prefix.
I think you want to just use strcmp here. Then you can drop classNameLength.

Add test coverage if you want: findObjects({ class: "Obj" })

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: find-by-prototype.patch

Carrying over r+.

New try push to be sure: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7785757aaf65

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f19523dc6bf

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f19523dc6bf