Make `ObjectActor.onDisplayString` not call debuggee code

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: benvie, Assigned: benvie)

Tracking

Trunk
Firefox 28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 829648 [details] [diff] [review]
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.
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
Attachment #829648 - Flags: review?(nfitzgerald)
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.
Attachment #829648 - Flags: review?(nfitzgerald) → review+
(Assignee)

Comment 3

4 years ago
Created attachment 830407 [details] [diff] [review]
display-string.patch

Addresses feedback.

https://tbpl.mozilla.org/?tree=Try&rev=716b91cc9f99
Attachment #829648 - Attachment is obsolete: true
Attachment #830407 - Flags: review+
(Assignee)

Comment 4

4 years ago
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
(Assignee)

Comment 5

4 years ago
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
(Assignee)

Updated

4 years ago
Blocks: 937765
(Assignee)

Comment 6

4 years ago
Created attachment 831682 [details] [diff] [review]
display-string.patch

This should fix the last failure.

https://tbpl.mozilla.org/?tree=Try&rev=eef81350b42e
Attachment #830407 - Attachment is obsolete: true
Attachment #831682 - Flags: review+
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/3cc8009a5f89
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3cc8009a5f89
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.