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

RESOLVED FIXED in Firefox 63

Status

P3
normal
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: nchevobbe, Assigned: jimb)

Tracking

unspecified
Firefox 63

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 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

8 months ago
Priority: -- → P3
(Reporter)

Comment 1

8 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: → bug 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.
Blocks: 1448847
(Reporter)

Comment 4

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

Comment 5

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

Comment 6

7 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

7 months ago
Oh, it's obvious.

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

Comment 8

7 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

7 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

7 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

7 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

7 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

7 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

7 months ago
Created attachment 8995776 [details] [diff] [review]
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.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
(Reporter)

Comment 13

7 months ago
Clearing flag as it looks you figured it out
Flags: needinfo?(nchevobbe)
See Also: → bug 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

6 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

6 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

6 months ago
Created attachment 9002508 [details] [diff] [review]
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.
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

6 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

6 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

6 months ago
Created attachment 9003296 [details] [diff] [review]
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.
Attachment #9002508 - Attachment is obsolete: true
Attachment #9003296 - Flags: review+
(Assignee)

Updated

6 months ago
Keywords: leave-open → checkin-needed

Comment 27

6 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

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