1473272 - [jsdbg2] Debugger.Object claims constructor-less ES6 classes have no script

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

**Steps to reproduce**
1. Go to `data:text/html,<meta charset=utf8><script>class WithConstructor {constructor() {}}class WithoutConstructor {}console.log(WithConstructor);console.log(WithoutConstructor);</script>`

pretty print of the JS: 
```js
  class WithConstructor {
    constructor() {}
  }
  class WithoutConstructor {}

  console.log(WithConstructor);
  console.log(WithoutConstructor);
```

2. Open the console
3. Spot the 2 logs (WithConstructor and WithoutConstructor)

**Expected results**

Both WithConstructor and WithoutConstructor have an arrow icon to the right that allow the user to jump to the class definition in the debugger.

**Actual results**

Only the WithConstructor function has the arrow icon displayed

---

Here are the RDP packets we receive for resp WithConstructor and WithoutConstructor: 



      {
        "type": "object",
        "actor": "server1.conn0.child1/obj28",
        "class": "Function",
        "extensible": true,
        "frozen": false,
        "sealed": false,
        "ownPropertyLength": 3,
        "name": "WithConstructor",
        "displayName": "WithConstructor",
        "location": {
          "url": "data:text/html,<meta charset=utf8><script>class WithConstructor {constructor() {}}class WithoutConstructor {}console.log(WithConstructor);console.log(WithoutConstructor);</script>",
          "line": 1
        }
      }

      {
        "type": "object",
        "actor": "server1.conn0.child1/obj29",
        "class": "Function",
        "extensible": true,
        "frozen": false,
        "sealed": false,
        "ownPropertyLength": 3,
        "name": "WithoutConstructor",
        "displayName": "WithoutConstructor"
      }

We can see that the WithoutConstructor packet misses the location object (which is what enables the jump to definition icon).

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Here's where we build the packet for a function https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/devtools/server/actors/object/previewers.js#80-89 

For classes without constructor `dbgGlobal.makeDebuggeeValue(obj.unsafeDereference()).script` is probably falsy and we don't create the location property.

Comment 2

[Julian Descottes [:jdescottes]]

Jim: can you share pointers to help us fix this? As Nicolas hinted, calling `makeDebuggeeValue` for an es6 class that has no constructor returns an object with no "script" attached. I tried to track this down in https://searchfox.org/mozilla-central/source/js/src/vm/Debugger.cpp, but I'm a bit stuck in the investigation right now.

Comment 3

[Brian Grinstead [:bgrins]]

Blocking Bug 1448847 since this prevents the 'open in debugger' link from showing up on Custom Elements if they haven't specified a constructor.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

if the work on this bug could benefit Bug 1351635 as well that would be great.

Comment 5

[Jim Blandy :jimb]

Does it help to replace `dbgGlobal.makeDebuggeeValue(obj.unsafeDereference())` with `dbg.adoptDebuggeeValue(obj)`?

Comment 6

[Jim Blandy :jimb]

I can reproduce this in the JavaScript shell:

js> var g = newGlobal();
js> var dbg = new Debugger(g);
js> g.evaluate('class WithConstructor {constructor() {}}class WithoutConstructor {}');
js> g.WithConstructor
js> g.WithoutConstructor
js> var gDO = dbg.addDebuggee(g)
js> gDO.executeInGlobal('WithConstructor').return.script
({})
js> gDO.executeInGlobal('WithoutConstructor').return.script
null
js>

Comment 7

[Jim Blandy :jimb]

Oh, it's obvious.

Without a constructor, a class is not a function. It has no script.

Comment 8

[Jim Blandy :jimb]

Oh, I see. It *is* a function; but WithoutConstructor gets the implicit constructor, which is defined here:

https://searchfox.org/mozilla-central/source/js/src/builtin/Classes.js#17

Debugger refuses to look at self-hosted code, so the script accessor gets swatted by this line here:

https://searchfox.org/mozilla-central/source/js/src/vm/Debugger.cpp#9135

I think this means that constructor-less classes are implemented in a way which leaves no indication of where they are defined.

Comment 9

[Jim Blandy :jimb]

Here's the hack that allows them to find their original source, even though they're self-hosted:

https://searchfox.org/mozilla-central/source/js/src/vm/JSFunction.cpp#969-972

Comment 10

[Jim Blandy :jimb]

So, a default constructor is a clone of a self-hosted script (see comment 8) that has been cloned into the user's compartment and had its source object and span whacked to point to the user's code:

https://searchfox.org/mozilla-central/rev/704612cf4426f0f0510b3e160895578c319a3270/js/src/vm/Interpreter.cpp#310-311

Since it's been eagerly cloned and its source location has been whacked, I can't think of any reason it needs to be marked as self-hosted code any more. Debugger assiduously avoids self-hosted code, since it's really an implementation detail of the system and doesn't make sense to display any more than C++ code would make sense to display.

Experimenting right now with simply clearing the script's self-hosted bit at the same time we whack its source location. That seems to fix Debugger's behavior, at least.

Comment 11

[Jim Blandy :jimb]

Oooh. JSScript::setDefaultClassConstructorSpan sets JSScript::toStringStart_ and toStringEnd_, but not JSScript::sourceStart_ and sourceEnd_. It's the latter that Debugger.Script.prototype.sourceStart and sourceLength consult, unfortunately. The two pairs differ as described here:

https://searchfox.org/mozilla-central/rev/704612cf4426f0f0510b3e160895578c319a3270/js/src/vm/JSScript.h#972-997

Comment 12

[Jim Blandy :jimb]

Attachment: Don't treat classes with default constructors as self-hosted code.

Here's the patch I'm trying out right now. It seems to fix the problem. It needs tests.

At the moment, SpiderMonkey handles classes without constructors by cloning the appropriate default constructor function from the self-hosted code compartment, one of the two functions defined here:

https://searchfox.org/mozilla-central/source/js/src/builtin/Classes.js

However, Debugger assiduously ignores self-hosted code: when the Debugger.Object.prototype.script accessor sees that the Debugger.Object's referent is a function whose script is self-hosted, it returns null.

The patch changes JSScript::setDefaultClassConstructorSpan - the function that sets the source object and toString range for the cloned script to point at the class declaration (the one without a constructor) - to also clear the script's self-hosted bit.

I audited all uses of JSScript::selfHosted in the code. They fall into the following categories, only the last of which could be problematic:

- We avoid listing stack frames in self-hosted code. Actually, in this case, clearing the cloned constructor's self-hosted bit is a good thing: we want the stack to show the frame.

- We avoid using stack frames running self-hosted code to choose the current realm. Since we've always cloned the script into the realm of the class, it should be fine to use the script's realm.

- We can re-lazify self-hosted code. I think it's not the end of the world if we can't re-lazify these trivial constructors.

- We propagate or preserve the self-hosted bit when we copy code, as in CreateEmptyScriptForClone and XDR. This isn't relevant to us.

- We avoid warning about problems in self-hosted code. This should be fine.

- The Debugger API and the decompiler avoid exposing internal details of self-hosted operations. This one I'm not so sure is okay. The default constructor for subclasses has arguments and code, and it would be a bit mysterious if those showed up. Although, I'm not sure what else we could do here.

Tests we should have:

- Obviously, check that we can fetch the script of a default constructor.

- In the derived class case, check that the stack frame for the default constructor shows up on the stack.

- Check that the bytecode / source mappings for DefaultDerivedClassConstructor are okay.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Clearing flag as it looks you figured it out

Comment 14

[Jason Orendorff [:jorendorff]]

In that bug, bgrins asked:
> Jason, will Bug 1473272 fix the wrong line number issue we are seeing in this case, or should we have a new bug to track this?

Jim, which do you prefer? Here's one test, in case it helps:

----
// Test the source location info in a derived-class default constructor.

function Y() { test(); }
class X extends Y {}  // line 4

function test() {
    for (let line of new Error().stack.split('\n'))
        if (line.startsWith("X@"))
            assertEq(line.split(":")[1], "4");
}

new X;
----

Comment 15

[Jim Blandy :jimb]

I think my patch should fix that. I'll be sure to include it in the tests, thanks!

Comment 16

[Jim Blandy :jimb]

(I'm on PTO today and tomorrow, but I am going to finish this today, as I promised various folks. This is not such a severe change that it couldn't go in after the soft freeze, either.)

Comment 17

[Jim Blandy :jimb]

Attachment: Don't treat classes with default constructors as self-hosted code.

The way this addresses the self-hosted script's line and column offsets affecting positions reported after it has been transplanted into the class definition's location is not great, but this bug is a priority for the devtools team, so I'd like to address that in a follow-up bug.

Comment 18

[Ted Campbell [:tcampbell]]

Comment on attachment 9002508 [details] [diff] [review]
Don't treat classes with default constructors as self-hosted code.

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

I appreciate the diligence in Comment 12. Generally it seems the flag is used to hide implementation details from frame iteration which as you point out we probably don't want to hide, and secondly for handling lazy source stuff which is what setDefaultClassConstructorSpan was disconnecting anyways.

Follow-up doc fixes to consider:
- https://searchfox.org/mozilla-central/rev/847b64cc28b74b44c379f9bff4f415b97da1c6d7/js/src/vm/JSFunction.cpp#969-972
- https://searchfox.org/mozilla-central/rev/847b64cc28b74b44c379f9bff4f415b97da1c6d7/js/src/vm/Interpreter.cpp#303-305
  - Maybe mention here that as a result we no longer need it marked as self-hosted.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d615f488d9f
Don't treat classes with default constructors as self-hosted code. r=tcampbell

Comment 20

[Ted Campbell [:tcampbell]]

Pushed part 1 on Jim's request

Comment 21

[Raul Gurzau (:RaulG)]

Backed out changeset 2d615f488d9f (bug 1473272) for failures at jit-test\tests\class\bug1473272-default-constructors.js on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/8229a612b15e5f233a33e8756c9dba9c07c57a83

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2d615f488d9faf8040e77b954ee9c82549feb543

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=194922182&repo=mozilla-inbound&lineNumber=32790

Log snippet: 

20:00:46     INFO -  TEST-PASS | tests\jit-test\jit-test\tests\class\bug1359622.js | Success (code 0, args "--baseline-eager") [0.1 s]
20:00:46     INFO -  {"action": "test_start", "jitflags": "--baseline-eager", "pid": 2268, "source": "jittests", "test": "class\\bug1359622.js", "thread": "main", "time": 1534795246.611}
20:00:46     INFO -  {"action": "test_end", "extra": {"jitflags": "--baseline-eager"}, "jitflags": "--baseline-eager", "message": "Success", "pid": 2268, "source": "jittests", "status": "PASS", "test": "class\\bug1359622.js", "thread": "main", "time": 1534795246.667}
20:00:46     INFO -  TEST-PASS | tests\jit-test\jit-test\tests\class\bug1359622.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads") [0.1 s]
20:00:46     INFO -  {"action": "test_start", "jitflags": "--ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads", "pid": 2268, "source": "jittests", "test": "class\\bug1359622.js", "thread": "main", "time": 1534795246.614}
20:00:46     INFO -  {"action": "test_end", "extra": {"jitflags": "--ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads"}, "jitflags": "--ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads", "message": "Success", "pid": 2268, "source": "jittests", "status": "PASS", "test": "class\\bug1359622.js", "thread": "main", "time": 1534795246.671}
20:00:46     INFO -  Z:\task_1534794350\build\tests\jit-test\jit-test\tests\class\bug1473272-default-constructors.js:12:13 Error: Assertion failed: got "\\task_1534794350\\build\\tests\\jit-test\\jit-test\\tests\\class\\bug1473272-default-constructors.js", expected "4"
20:00:46     INFO -  Stack:
20:00:46     INFO -    test@Z:\task_1534794350\build\tests\jit-test\jit-test\tests\class\bug1473272-default-constructors.js:12:13
20:00:46     INFO -    W@Z:\task_1534794350\build\tests\jit-test\jit-test\tests\class\bug1473272-default-constructors.js:3:16
20:00:46     INFO -    Z@Z:\task_1534794350\build\tests\jit-test\jit-test\tests\class\bug1473272-default-constructors.js:4:49
20:00:46     INFO -    Y@Z:\task_1534794350\build\tests\jit-test\jit-test\tests\class\bug1473272-default-constructors.js:5:49
20:00:46     INFO -    X@Z:\task_1534794350\build\tests\jit-test\jit-test\tests\class\bug1473272-default-constructors.js:7:49
20:00:46     INFO -    @Z:\task_1534794350\build\tests\jit-test\jit-test\tests\class\bug1473272-default-constructors.js:20:1
20:00:46     INFO -  Exit code: 3

Comment 22

[Ted Campbell [:tcampbell]]

Test case splits the error stack using ':' but that is part of the filename path on Windows and everything goes downhill from there..

Comment 23

[Ted Campbell [:tcampbell]]

Comment on attachment 9002508 [details] [diff] [review]
Don't treat classes with default constructors as self-hosted code.

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

::: js/src/jit-test/tests/class/bug1473272-default-constructors.js
@@ +8,5 @@
> +
> +function test() {
> +    for (let line of new Error().stack.split('\n')) {
> +        if (line.startsWith("Z@"))
> +            assertEq(line.split(":")[1], "4");

r=me for |line.split(":").reverse()[1]| to fix, if you agree

Comment 24

[Jim Blandy :jimb]

I took a slightly different approach. In the interest of avoiding another backout:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87c1b1d4591ec11350548e7719604913c79c2fc3

Comment 25

[Ted Campbell [:tcampbell]]

Makes sense.

Comment 26

[Jim Blandy :jimb]

Attachment: Don't treat classes with default constructors as self-hosted code.

Carrying over r+ from Ted's review, since he's looked over the new version.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/714f1a873154
Don't treat classes with default constructors as self-hosted code. r=tcampbell

Comment 28

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/714f1a873154