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

RESOLVED FIXED in Firefox 63

Status

defect
P3
normal
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: nchevobbe, Assigned: jimb)

Tracking

unspecified
Firefox 63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

11 months ago
**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).
Reporter

Updated

11 months ago
Priority: -- → P3
Reporter

Comment 1

11 months ago
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.
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 https://searchfox.org/mozilla-central/source/js/src/vm/Debugger.cpp, 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.
Reporter

Comment 4

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

Comment 5

10 months ago
Does it help to replace `dbgGlobal.makeDebuggeeValue(obj.unsafeDereference())` with `dbg.adoptDebuggeeValue(obj)`?
Flags: needinfo?(jimb) → needinfo?(nchevobbe)
Assignee

Comment 6

10 months ago
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>
Assignee

Comment 7

10 months ago
Oh, it's obvious.

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

Comment 8

10 months ago
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.
Assignee

Comment 9

10 months ago
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
Assignee

Comment 10

10 months ago
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.
Assignee

Comment 11

10 months ago
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
Assignee

Updated

10 months ago
Summary: constructor-less ES6 classes RDP packet does not contain location information → Debugger.Object claims constructor-less ES6 classes have no script
Assignee

Updated

10 months ago
Summary: Debugger.Object claims constructor-less ES6 classes have no script → [jsdbg2] Debugger.Object claims constructor-less ES6 classes have no script
Assignee

Comment 12

10 months ago
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.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Reporter

Comment 13

10 months ago
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)
Assignee

Comment 15

9 months ago
I think my patch should fix that. I'll be sure to include it in the tests, thanks!
Flags: needinfo?(jimb)
Assignee

Comment 16

9 months ago
(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.)
Assignee

Comment 17

9 months ago
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:
- 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.
Attachment #9002508 - Flags: review?(tcampbell) → review+

Comment 19

9 months ago
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d615f488d9f
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: 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
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
Assignee

Comment 24

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87c1b1d4591ec11350548e7719604913c79c2fc3
Flags: needinfo?(jimb)
Makes sense.
Assignee

Comment 26

9 months ago
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+
Assignee

Updated

9 months ago

Comment 27

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/714f1a873154
Don't treat classes with default constructors as self-hosted code. r=tcampbell
Keywords: checkin-needed

Comment 28

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/714f1a873154
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.