Closed Bug 1473272 Opened 6 years ago Closed 6 years ago

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


(DevTools :: Console, defect, P3)



(firefox63 fixed)

Firefox 63
Tracking Status
firefox63 --- fixed


(Reporter: nchevobbe, Assigned: jimb)




(1 file, 2 obsolete files)

**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: 
  class WithConstructor {
    constructor() {}
  class 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).
Priority: -- → P3
Here's where we build the packet for a function 

For classes without constructor `dbgGlobal.makeDebuggeeValue(obj.unsafeDereference()).script` is probably falsy and we don't create the location property.
See Also: → 1443923
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, but I'm a bit stuck in the investigation right now.
Flags: needinfo?(jimb)
Blocking Bug 1448847 since this prevents the 'open in debugger' link from showing up on Custom Elements if they haven't specified a constructor.
if the work on this bug could benefit Bug 1351635 as well that would be great.
Does it help to replace `dbgGlobal.makeDebuggeeValue(obj.unsafeDereference())` with `dbg.adoptDebuggeeValue(obj)`?
Flags: needinfo?(jimb) → needinfo?(nchevobbe)
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
Oh, it's obvious.

Without a constructor, a class is not a function. It has no script.
Oh, I see. It *is* a function; but WithoutConstructor gets the implicit constructor, which is defined here:

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

I think this means that constructor-less classes are implemented in a way which leaves no indication of where they are defined.
Here's the hack that allows them to find their original source, even though they're self-hosted:
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:

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.
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:
Summary: constructor-less ES6 classes RDP packet does not contain location information → Debugger.Object claims constructor-less ES6 classes have no script
Summary: Debugger.Object claims constructor-less ES6 classes have no script → [jsdbg2] Debugger.Object claims constructor-less ES6 classes have no script
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:

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.
Assignee: nobody → jimb
Clearing flag as it looks you figured it out
Flags: needinfo?(nchevobbe)
See Also: → 1478999
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;
Flags: needinfo?(jimb)
I think my patch should fix that. I'll be sure to include it in the tests, thanks!
Flags: needinfo?(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.)
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.
Attachment #8995776 - Attachment is obsolete: true
Attachment #9002508 - Flags: review?(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:
  - Maybe mention here that as a result we no longer need it marked as self-hosted.
Attachment #9002508 - Flags: review?(tcampbell) → review+
Pushed by
Don't treat classes with default constructors as self-hosted code. r=tcampbell
Pushed part 1 on Jim's request
Keywords: leave-open
Backed out changeset 2d615f488d9f (bug 1473272) for failures at jit-test\tests\class\bug1473272-default-constructors.js on a CLOSED TREE

Backout link:

Push with failures:

Log link:

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
Flags: needinfo?(jimb)
Test case splits the error stack using ':' but that is part of the filename path on Windows and everything goes downhill from there..
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
I took a slightly different approach. In the interest of avoiding another backout:
Flags: needinfo?(jimb)
Makes sense.
Carrying over r+ from Ted's review, since he's looked over the new version.
Attachment #9002508 - Attachment is obsolete: true
Attachment #9003296 - Flags: review+
Pushed by
Don't treat classes with default constructors as self-hosted code. r=tcampbell
Keywords: checkin-needed
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.