936101 - Make `ObjectActor.onDisplayString` not call debuggee code

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Brandon Benvie [:benvie]]

As a short term solution, `ObjectActor.onDisplayString` was created to call debuggee code (the "toString" method of the object). Instead of doing this, we should manually generate the string depending on the type of object it is.

Comment 1

[Brandon Benvie [:benvie]]

Attachment: display-string.patch

This patch introduces a `stringify` function that is able to stringify most things the way they would be by calling their `toString` method, but without calling debuggee code. This is the first step in producing prettier output for debuggees. This patch simply tries to emulate how the builtin toString functions work, without getting fancy.

Comment 2

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

Comment on attachment 829648 [details] [diff] [review]
display-string.patch

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

r+ with comments addressed.

::: toolkit/devtools/server/actors/script.js
@@ +2673,5 @@
> +  return fn && fn.callable && fn.class == "Function" && !fn.script;
> +}
> +
> +/**
> + * Safely get the value for a Debugger.Object given a key. Walks the prototype

getNittiest of nits, but I still think it is worthwhile.

"Safely get the value for a Debugger.Object given a key."

I feel like this should mention you are getting the value of a _property_ in the debugger object. Basically I was confused that the word "property" was missing after looking at the code.

Maybe:

"Safely the the value of the property keyed by 'key' in the given Debugger.Object."

?

@@ +2697,5 @@
> +      aObj = aObj.proto;
> +    } while (aObj);
> +  } catch (e) {
> +    // If anything goes wrong just return undefined.
> +    dumpn(e);

DevToolsUtils.reportException("getProperty", e);

@@ +2715,5 @@
> +  return aObj => aCtor.prototype.toString.call(aObj.unsafeDereference());
> +}
> +
> +/**
> + * Stringify Debugger.Object that points to an instance of a builtin error.

Dude, sorry I am nitting all over your comments, but:

Is "Stringify a Debugger.Object-wrapped Error instance" better?

@@ +2741,5 @@
> + *         The stringification for the object.
> + */
> +function stringify(aObj) {
> +  if (Cu.isDeadWrapper(aObj)) {
> +    return "";

This makes me unhappy. I agree we shouldn't throw or anything, but maybe we can still use DevToolsUtils.reportException to dump and console.log the error?

Maybe we should return "[object Dead]" or something?

Meh :-/

@@ +2750,5 @@
> +
> +// Used to prevent infinite recursion when an array is found inside itself.
> +let seen = null;
> +
> +const stringifiers = {

Beautiful.

@@ +2761,5 @@
> +  URIError: errorStringify,
> +  Boolean: createBuiltinStringifier(Boolean),
> +  Function: createBuiltinStringifier(Function),
> +  Number: createBuiltinStringifier(Number),
> +  String: createBuiltinStringifier(String),

I love it.

@@ +2778,5 @@
> +
> +    const len = getProperty(obj, "length");
> +    let string = "";
> +    if (len > 0) {
> +      for (let i = 0; i < len; i++) {

Check if len is a number as well.

@@ -2848,5 @@
>  
>        obj = obj.proto;
>        level++;
>      }
> -

Nit: Leave the whitespace.

@@ +3046,5 @@
>        if (!desc || desc.value !== undefined || !("get" in desc)) {
>          continue;
>        }
>  
> +      if (hasSafeGetter(desc)) {

Awesome.

::: toolkit/devtools/server/tests/unit/test_objectgrips-12.js
@@ +50,5 @@
> +    recursiveArray.push(recursiveArray);
> +    const customError = new Error("bar");
> +    customError.name = "CustomError";
> +
> +    const values = [ 

trailing whitespace

@@ +66,5 @@
> +      new SyntaxError(),
> +      new ReferenceError(""),
> +      customError,
> +      () => {},
> +      Array

Can we also add "normal" (not nested in an array or object, new Constructor()'d):

* string

* number

* true

* false

* null

* undefined

* regular expressions

* Proxy?

* non arrow anonymous function

* non arrow named function

@@ +70,5 @@
> +      Array
> +    ];
> +
> +    if (typeof stopMe == "undefined") {
> +      return values.map(value => {

Instead of this crazy toSource and eval sometimes, run normally other times, lets use a data driven approach:

const values = [
  { input: "Object.create(null)", output: "[object Object]" },
  // etc...
];

Slightly more typing, but it is easier to grok and extensible for the next bug when we want to do better than being equivalent to toString.

Comment 3

[Brandon Benvie [:benvie]]

Attachment: display-string.patch

Addresses feedback.

https://tbpl.mozilla.org/?tree=Try&rev=716b91cc9f99

Comment 4

[Brandon Benvie [:benvie]]

Interesting... `const stringifiers = ...` at the top level blows a bunch of things up due to const redeclaration. I guess it's run multiple times for some reason somehow? Anyway, another try run using let instead of const.

https://tbpl.mozilla.org/?tree=Try&rev=e5795bbb56b9

Comment 5

[Brandon Benvie [:benvie]]

Running it through try one before time before I try to debug anything because those fails don't look related. https://tbpl.mozilla.org/?tree=Try&rev=4c1943b96784

Comment 6

[Brandon Benvie [:benvie]]

Attachment: display-string.patch

This should fix the last failure.

https://tbpl.mozilla.org/?tree=Try&rev=eef81350b42e

Comment 7

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/3cc8009a5f89

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3cc8009a5f89