Closed
Bug 1473272
Opened 6 years ago
Closed 6 years ago
[jsdbg2] Debugger.Object claims constructor-less ES6 classes have no script
Categories
(DevTools :: Console, defect, P3)
DevTools
Console
Tracking
(firefox63 fixed)
RESOLVED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: nchevobbe, Assigned: jimb)
References
Details
Attachments
(1 file, 2 obsolete files)
7.28 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
**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•6 years ago
|
Priority: -- → P3
Reporter | ||
Comment 1•6 years 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.
Comment 2•6 years ago
|
||
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)
Comment 3•6 years ago
|
||
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: devtools-webcomponents-63
Reporter | ||
Comment 4•6 years ago
|
||
if the work on this bug could benefit Bug 1351635 as well that would be great.
Assignee | ||
Comment 5•6 years ago
|
||
Does it help to replace `dbgGlobal.makeDebuggeeValue(obj.unsafeDereference())` with `dbg.adoptDebuggeeValue(obj)`?
Flags: needinfo?(jimb) → needinfo?(nchevobbe)
Assignee | ||
Comment 6•6 years 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•6 years ago
|
||
Oh, it's obvious. Without a constructor, a class is not a function. It has no script.
Assignee | ||
Comment 8•6 years 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•6 years 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•6 years 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•6 years 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•6 years 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•6 years 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•6 years 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•6 years ago
|
||
Clearing flag as it looks you figured it out
Flags: needinfo?(nchevobbe)
Comment 14•6 years ago
|
||
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 years 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 years 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 years 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 18•6 years ago
|
||
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 years 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
Comment 21•6 years ago
|
||
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)
Comment 22•6 years ago
|
||
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•6 years ago
|
||
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 years 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)
Assignee | ||
Comment 26•6 years 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•6 years ago
|
Keywords: leave-open → checkin-needed
Comment 27•6 years 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 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/714f1a873154
Status: ASSIGNED → RESOLVED
Closed: 6 years 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.
Description
•