1908650 - Debugger.findObjects class filter could also match prototype/constructor name

Status: RESOLVED FIXED

(DevTools :: Debugger, task)

Description

[Nicolas Chevobbe [:nchevobbe]]

At the moment, the findObjects class filter make the function only return objects whose internal [[Class]]'s name matches the given string.
This means that if you have something like class MyClass {} globalThis.myInstance = new MyClass(), findObjects({class: "MyClass"}) will return an empty array, although I was expecting an array with myInstance in it.

Ideally, with the proper change, the modified test would pass:

diff --git a/js/src/jit-test/tests/debug/Debugger-findObjects-07.js b/js/src/jit-test/tests/debug/Debugger-findObjects-07.js
--- a/js/src/jit-test/tests/debug/Debugger-findObjects-07.js
+++ b/js/src/jit-test/tests/debug/Debugger-findObjects-07.js
@@ -7,9 +7,20 @@ var gw = dbg.addDebuggee(g);
 
 g.eval('this.re = /foo/;');
 g.eval('this.d = new Date();');
+g.eval(`
+  class MyClass {
+    constructor(name) {
+      this.name = name;
+    }
+  }
+  this.myInstanceA = new MyClass("a");
+  this.myInstanceB = new MyClass("b");
+`);
 
 var rew = gw.makeDebuggeeValue(g.re);
 var dw = gw.makeDebuggeeValue(g.d);
+var myInstanceAw = gw.makeDebuggeeValue(g.myInstanceA);
+var myInstanceAw = gw.makeDebuggeeValue(g.myInstanceB);
 
 var objects;
 
@@ -20,3 +31,8 @@ assertEq(objects.indexOf(dw) == -1, true
 objects = dbg.findObjects({ class: "Date" });
 assertEq(objects.indexOf(dw) != -1, true);
 assertEq(objects.indexOf(rew) == -1, true);
+
+objects = dbg.findObjects({ class: "MyClass" });
+assertEq(objects.length, 2);
+assertEq(objects.includes(myInstanceAw), true);
+assertEq(objects.includes(myInstanceAw), true);

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

I plan to use this function for Bug 1891686.
Once it lands, we should make sure to coordinate this bug and Bug 1526688, otherwise we could have confusing results:

> queryObjects(MyClass)
< [ Object {} ]

or the other way around

> queryObjects(Object)
< [ MyClass {} ]

Comment 2

[Tooru Fujisawa [:arai]]

The question here would be what to do for subclasses, especially built-in subclasses.

class MyArray extends Array {
}
var myArrayA = new MyArray();

Currently, dbg.findObjects({ class: "Array" }) returns the instance in myArrayA,
this is because the "class name" used by the query is the object's built-in class's name, which is "Array" for MyArray class instance, because MyArray is Array subclass.

Possible options would be the following:

• (a) if constructor function is found on the prototype chain, use the function name (MyArray in the above case). otherwise use the built-in class's name (Array in the above case)
• (b) use both constructor function's name and the built-in class's name
• (c) use all prototype chain's constructor name, and the built-in class's name

with (a), dbg.findObjects({ class: "Array" }) doesn't return myArrayA but dbg.findObjects({ class: "MyArray" }) returns myArrayA.

with (b), both dbg.findObjects({ class: "Array" }) and dbg.findObjects({ class: "MyArray" }) return myArrayA,
but the superclass's name is used only for built-in subclasses, which means, in the following case:
dbg.findObjects({ class: "MySubClass" }) returns mySubClassA but dbg.findObjects({ class: "MySuperClass" }) doesn't.

class MySuperClass {
}
class MySubClass extends MySuperClass {
}
var mySubClassA = new MySubClass();

with (c), both dbg.findObjects({ class: "Array" }) and dbg.findObjects({ class: "MyArray" }) return myArrayA,
and also both dbg.findObjects({ class: "MySubClass" }) and dbg.findObjects({ class: "MySuperClass" }) return mySubClassA,
but the situation becomes more complex.

If the query's class name is intended only for the direct prototype, which is, Object.getPrototypeOf(value).constructor.name == "ClassName", (a) fits.
if the query's class name is intended as value instanceof ClassName expression, it would need (c)

Comment 3

[Tooru Fujisawa [:arai]]

another option would be to add a new query parameter than class property for the constructor name, and/or all constructor names in the prototype chain.
that way the behavior can easily be controllable from the consumer.

In order to decide the design, it would be nice to figure out the requirements.

According to the Chrome's document, the queryObject console function requires all the constructor names in the prototype chain.

queryObjects(HTMLElement). Returns all HTML elements.

If there's any other consumer that requires different behavior, we should reflect that to the design.

Comment 4

[Tooru Fujisawa [:arai]]

Chrome's queryObjects receives a constructor or a prototype object, instead of a class name.

In the following case, queryObjects(Foo) and queryObjects(Foo.prototype) shows an array with x.

class Foo {
}
var x = new Foo();

Then, in the following case, queryObjects(class1) shows an array with x, but queryObjects(class2) shows an empty array,
which means the classes with same name aren't mixed up.
Thus, the filtering should be performed by the actual constructor/prototype comparison.

var class1 = class Foo {
};
var class2 = class Foo {
};
var x = new class1();

Also, in the following case, the array shown by queryObjects(Object) contains x, but not y,
that would mean the actual prototype chain is used for the comparison.
(I'm not sure if there's any way to get y in Chrome. queryObjects(null) throws)

var x = {};
var y = { __proto__: null };

The SpiderMonkey's findObjects return both x and y for dbg.findObjects({ class: "Object" }), because the JSClass is Object regardless of prototype chain.

Given the above, string comparison doesn't fit, and also using the JSClass name doesn't also fit.
So, none of (a), (b), (c) above doesn't work for this purpose (as long as we want the compatibility).

Possible options would be the following:

• (A) add another query parameter that receives a debuggee object, and perform the constructor/prototype comparison in C++ side, in findObjects
• (B) call findObjects() without query, which returns all objects, and perform the filtering in the JS side

In order to avoid side effects, performing it in C++ side might be easier

Comment 5

[Tooru Fujisawa [:arai]]

I have a prototype that can match with ctor/proto in prototype chain for static prototype cases.
I'll check if it's sufficient for regular DOM objects (no dynamic prototype appears) and then attach the patch.

Comment 6

[Tooru Fujisawa [:arai]]

The query works as long as the DOM reflector object is created for the DOM objects.

ref. mozilla::dom::GetOrCreateDOMReflector, nsINode::WrapObject, nsINode::WrapNode

so, findObjects({ class: HTMLElement }) returns a subset of all HTMLElements, where those items already have DOM reflector created.

For example, if the web page contains a <script> element but it hasn't never accessed from JS, findObjects({ class: HTMLScriptElement }) returns an empty array.
Then, for example, document.querySelectorAll("script") creates the reflector for all <script> elements, in order to expose the script to JS side, and after that, findObjects({ class: HTMLScriptElement }) returns an array with the <script> element in it.

Then, the same behavior is observed also with Chrome, so at least for Chrome-parity, the above behavior would be acceptable.
If we want to return all objects, including reflector objects that are not yet created, then it would require support also from DOM/binding side.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Bug 1908650 - Add support for querying with constructor/prototype in findObjects. r?bthrall!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/597f37b32488
Add support for querying with constructor/prototype in findObjects. r=bthrall,nchevobbe

Comment 9

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/597f37b32488