Closed
Bug 1316265
Opened 9 years ago
Closed 6 years ago
Object actor should have properties to indicate if a function is a generator or an async function
Categories
(DevTools :: Shared Components, enhancement, P3)
DevTools
Shared Components
Tracking
(firefox73 fixed)
RESOLVED
FIXED
Firefox 73
Tracking | Status | |
---|---|---|
firefox73 | --- | fixed |
People
(Reporter: ntim, Assigned: nchevobbe)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
var a = async function() {}
a;
should display: async function a() or something similar
Comment 1•9 years ago
|
||
Nicolas, would this require a change in Reps to show async functions somehow differently?
Severity: normal → enhancement
Flags: needinfo?(chevobbe.nicolas)
Priority: -- → P3
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #1)
> Nicolas, would this require a change in Reps to show async functions somehow
> differently?
Yes, we need to make a change in http://searchfox.org/mozilla-central/source/devtools/client/shared/components/reps/function.js#33 . I guess we have the info (that the function is async) in the grip, so it should be pretty straightforward.
Should we display generator function differently too ( `function* gen_a()` )?
Assignee: nobody → chevobbe.nicolas
Flags: needinfo?(chevobbe.nicolas)
Assignee | ||
Comment 3•9 years ago
|
||
> Should we display generator function differently too ( `function* gen_a()` )?
FWIW, we don't in the old console, but I think this would be nice
Comment 4•9 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)
> (In reply to Brian Grinstead [:bgrins] from comment #1)
> > Nicolas, would this require a change in Reps to show async functions somehow
> > differently?
>
> Yes, we need to make a change in
> http://searchfox.org/mozilla-central/source/devtools/client/shared/
> components/reps/function.js#33 . I guess we have the info (that the function
> is async) in the grip, so it should be pretty straightforward.
>
Thanks, going to move this into shared components as a reps bug
> Should we display generator function differently too ( `function* gen_a()` )?
Yes, I'd say so (if we have the data in the grip).
Updated•9 years ago
|
Component: Developer Tools: Console → Developer Tools: Shared Components
Summary: Console should display when a function is async → Reps: display when a function is async
Assignee | ||
Comment 5•9 years ago
|
||
Unfortunately, we don't have those data in the grip. I'll see if we can get them on the server side.
Reporter | ||
Updated•9 years ago
|
Assignee: chevobbe.nicolas → ntim.bugs
Reporter | ||
Updated•9 years ago
|
Summary: Reps: display when a function is async → Reps: display when a function is a generator or an async function
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•9 years ago
|
||
mozreview-review |
Comment on attachment 8820224 [details]
Bug 1316265 - Reps: display when a function is a generator or an async function.
https://reviewboard.mozilla.org/r/99752/#review100176
::: devtools/server/actors/object.js:1131
(Diff revision 1)
> + grip.isAsync = false;
> + grip.isGenerator = false;
I've chosen 2 flags, because AsyncGeneratorFunction will also be a thing soon. The current code in reps should just work once they are implemented.d
https://github.com/tc39/proposal-async-iteration
Assignee | ||
Comment 8•9 years ago
|
||
mozreview-review |
Comment on attachment 8820224 [details]
Bug 1316265 - Reps: display when a function is a generator or an async function.
https://reviewboard.mozilla.org/r/99752/#review100184
Looks good overall, thanks !
No r+ because i'd like to see the test changes in a separate commit and it would be great to have consistency in how we display function in tiny mode
::: devtools/client/shared/components/reps/function.js:42
(Diff revision 1)
> + }
> + return keyword;
> + },
> +
> + getTitle: function (grip, mode) {
> + if (mode == MODE.TINY) {
nit: use `===`
::: devtools/client/shared/components/reps/function.js:57
(Diff revision 1)
> - let name = grip.userDisplayName || grip.displayName || grip.name || "function";
> + let defaultName = mode === MODE.TINY ? "function" : "";
> + let name = grip.userDisplayName || grip.displayName || grip.name || defaultName;
> + if (mode === MODE.TINY) {
> + if (name == "function") {
> + return this.getKeyword(grip) + "()";
> + }
> +
> + if (grip.isGenerator) {
> + name = "*" + name;
> + }
> + if (grip.isAsync) {
> + name = "async " + name;
> + }
> +
> + }
> return cropString(name + "()", 100);
I find this a bit confusing.
We're targetting the tiny mode to give a dedicated defaultName, but we're not using it, since in l.60 we check the name, and if it's `"function"`, we call `getKeyword`.
I think what is causing the confusion is that there are 2 places where we try to handle the `async` and the `*` : both in `getTitle` and `summarizeFunction`.
I think we could move the logic of `summarizeFunction` into `getTitle` and have the objectLink wrap the entire text.
Thinking about this, I even think we could remove `getKeyword`
```
let name = grip.userDisplayName || grip.displayName || grip.name || "";
let asyncLabel = grip.isAsync ? "async ": "";
let keyword = mode !== MODE.TINY ? "function " : ""; // Implement the logic for anon function in TINY mode here
let generatorLabel = grip.isGenerator ? "*": "";
let title = cropString(`${asyncLabel}${keyword}${generatorLabel}${name}()`,100);
// wrap in objectLink if necessary
```
What do you think of this ?
::: devtools/client/shared/components/test/mochitest/test_reps_function.html:19
(Diff revision 1)
> -window.onload = Task.async(function* () {
> +window.onload = function () {
> let { Rep } = browserRequire("devtools/client/shared/components/reps/rep");
> let { Func } = browserRequire("devtools/client/shared/components/reps/function");
> + const { MODE } = browserRequire("devtools/client/shared/components/reps/constants");
>
> const componentUnderTest = Func;
>
> try {
> // Test that correct rep is chosen
> const gripStub = getGripStub("testNamed");
> const renderedRep = shallowRenderComponent(Rep, { object: gripStub });
> is(renderedRep.type, Func.rep, `Rep correctly selects ${Func.rep.displayName}`);
>
> - yield testNamed();
> - yield testVarNamed();
> - yield testAnon();
> - yield testLongName();
> + testNamed();
> + testVarNamed();
> + testAnon();
> + testLongName();
I thinks that it's a good change here, it would be great if it was done in its own commit.
::: devtools/client/shared/components/test/mochitest/test_reps_function.html:110
(Diff revision 1)
> - const defaultOutput = `function()`;
> + const defaultOutput = "function ()";
>
> const modeTests = [
> {
> mode: undefined,
> expectedOutput: defaultOutput,
> - }
> + },
> + {
> + mode: MODE.TINY,
> + expectedOutput: defaultOutput,
> + },
I think we could be more consistent here.
We have kind of different outputs in the same mode, if the function has a name or not
anon : `function ()`
named: `myFunc()`
Could we do something better with the anonymous function ?
I see a couple of solutions :
- have the same reasoning that in named function, hence displaying `()` only
- clearly labelling it as an anonymous function `anoynmousFunction()`.
I don't have strong opinion on either of those 2 solutions though, so do it as you think it's the best.
::: devtools/server/actors/object.js:1131
(Diff revision 1)
> + grip.isAsync = false;
> + grip.isGenerator = false;
> +
> + switch (type) {
> + case "AsyncFunction":
> + grip.isAsync = true;
> + break;
> + case "GeneratorFunction":
> + grip.isGenerator = true;
> + break;
> + }
Is there a reason to do this in 2 steps ?
Instead of doing something like :
```
grip.isAsync = type === "AsyncFunction";
grip.isGenerator = type === "GeneratorFunction";
```
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•9 years ago
|
||
mozreview-review |
Comment on attachment 8820224 [details]
Bug 1316265 - Reps: display when a function is a generator or an async function.
https://reviewboard.mozilla.org/r/99752/#review100246
::: devtools/server/actors/object.js:1132
(Diff revision 2)
> grip.parameterNames = obj.parameterNames;
> }
>
> + let type = DevToolsUtils.getProperty(obj, "constructor").name;
> + grip.isAsync = type === "AsyncFunction";
> + grip.isGenerator = type === "GeneratorFunction";
I'm seeing here (http://searchfox.org/mozilla-central/source/devtools/client/debugger/new/source-map-worker.js#885) that we check constructor.displayName too in the debugger.
I don't really know why (we do respect what is said here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/displayName in the console)
I guess it might be a good thing to test displayName too.
Attachment #8820224 -
Flags: review?(chevobbe.nicolas) → review+
Assignee | ||
Comment 12•9 years ago
|
||
mozreview-review |
Comment on attachment 8820257 [details]
Bug 1316265 - Remove unnecessary Task.async() and yield from test.
https://reviewboard.mozilla.org/r/99798/#review100248
looks good
Attachment #8820257 -
Flags: review?(chevobbe.nicolas) → review+
Reporter | ||
Comment 13•9 years ago
|
||
mozreview-review-reply |
Comment on attachment 8820224 [details]
Bug 1316265 - Reps: display when a function is a generator or an async function.
https://reviewboard.mozilla.org/r/99752/#review100246
> I'm seeing here (http://searchfox.org/mozilla-central/source/devtools/client/debugger/new/source-map-worker.js#885) that we check constructor.displayName too in the debugger.
> I don't really know why (we do respect what is said here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/displayName in the console)
>
> I guess it might be a good thing to test displayName too.
Since the AsyncFunction/GeneratorFunction constructors and the name property are both standard, I wouldn't worry too much about displayName (which is non-standard)
Comment 14•9 years ago
|
||
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6b0e836a33a9
Reps: display when a function is a generator or an async function. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/70b75b80fb80
Remove unnecessary Task.async() and yield from test. r=nchevobbe
![]() |
||
Comment 15•9 years ago
|
||
Backed out for leaks in Linux x64 devtools tests:
https://hg.mozilla.org/integration/autoland/rev/5ff154ef84c1e6579061548a96207f33b98e341c
https://hg.mozilla.org/integration/autoland/rev/15fa05ff3644cacf1097388591092a98409566ad
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=70b75b80fb800ba4284fbe1640ad0f8d2a5cf781
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=8186774&repo=autoland
[task 2016-12-20T18:05:53.827734Z] 18:05:53 INFO - TEST-START | devtools/client/debugger/new/test/mochitest/browser_dbg-scopes.js
[task 2016-12-20T18:05:53.859022Z] 18:05:53 INFO - ++DOCSHELL 0x7fb3bba87000 == 12 [pid = 1257] [id = {28fb6959-3080-4be2-b53e-1d060b236de3}]
[task 2016-12-20T18:05:53.860958Z] 18:05:53 INFO - ++DOMWINDOW == 56 (0x7fb3bba88000) [pid = 1257] [serial = 242] [outer = (nil)]
[task 2016-12-20T18:05:53.890315Z] 18:05:53 INFO - ++DOMWINDOW == 57 (0x7fb3bbc09000) [pid = 1257] [serial = 243] [outer = 0x7fb3bba88000]
[task 2016-12-20T18:05:54.089619Z] 18:05:54 INFO - ++DOMWINDOW == 58 (0x7fb3bbc1a800) [pid = 1257] [serial = 244] [outer = 0x7fb3bba88000]
[task 2016-12-20T18:05:54.358203Z] 18:05:54 INFO - ++DOCSHELL 0x7fb3bbe88800 == 13 [pid = 1257] [id = {880063f3-1161-4939-8557-eeaf1edc7dec}]
[task 2016-12-20T18:05:54.358298Z] 18:05:54 INFO - ++DOMWINDOW == 59 (0x7fb3bbe89000) [pid = 1257] [serial = 245] [outer = (nil)]
[task 2016-12-20T18:05:54.358361Z] 18:05:54 INFO - ++DOMWINDOW == 60 (0x7fb3bbe8b000) [pid = 1257] [serial = 246] [outer = 0x7fb3bbe89000]
[task 2016-12-20T18:05:54.405679Z] 18:05:54 INFO - ++DOMWINDOW == 61 (0x7fb3bbeaa800) [pid = 1257] [serial = 247] [outer = 0x7fb3bbe89000]
[task 2016-12-20T18:05:54.464505Z] 18:05:54 INFO - [1257] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8007000E: file /home/worker/workspace/build/src/dom/xul/nsXULPrototypeCache.cpp, line 323
[task 2016-12-20T18:05:54.481681Z] 18:05:54 INFO - [1257] WARNING: Ignoring duplicate observer.: file /home/worker/workspace/build/src/modules/libpref/nsPrefBranch.cpp, line 640
[task 2016-12-20T18:05:54.559787Z] 18:05:54 INFO - ++DOCSHELL 0x7fb3bbc1e800 == 14 [pid = 1257] [id = {9c19679f-ea46-417f-90f1-6d6659de923e}]
[task 2016-12-20T18:05:54.561395Z] 18:05:54 INFO - ++DOMWINDOW == 62 (0x7fb3bcc8c800) [pid = 1257] [serial = 248] [outer = (nil)]
[task 2016-12-20T18:05:54.562641Z] 18:05:54 INFO - ++DOMWINDOW == 63 (0x7fb3bcc96000) [pid = 1257] [serial = 249] [outer = 0x7fb3bcc8c800]
[task 2016-12-20T18:05:58.353843Z] 18:05:58 INFO - --DOMWINDOW == 62 (0x7fb3c7321800) [pid = 1257] [serial = 226] [outer = (nil)] [url = chrome://devtools/content/debugger/new/index.html]
[task 2016-12-20T18:05:58.355726Z] 18:05:58 INFO - --DOMWINDOW == 61 (0x7fb3bca15000) [pid = 1257] [serial = 207] [outer = (nil)] [url = about:blank]
[task 2016-12-20T18:05:58.367964Z] 18:05:58 INFO - --DOMWINDOW == 60 (0x7fb3be00c800) [pid = 1257] [serial = 209] [outer = (nil)] [url = http://example.com/browser/devtools/client/debugger/new/test/mochitest/examples/doc-exceptions.html]
[task 2016-12-20T18:05:58.372218Z] 18:05:58 INFO - --DOMWINDOW == 59 (0x7fb3c3e7c800) [pid = 1257] [serial = 220] [outer = (nil)] [url = http://example.com/browser/devtools/client/debugger/new/test/mochitest/examples/doc-minified.html]
[task 2016-12-20T18:05:58.374227Z] 18:05:58 INFO - --DOMWINDOW == 58 (0x7fb3c7128000) [pid = 1257] [serial = 223] [outer = (nil)] [url = about:blank]
[task 2016-12-20T18:05:58.375467Z] 18:05:58 INFO - --DOMWINDOW == 57 (0x7fb3bbeb5800) [pid = 1257] [serial = 215] [outer = (nil)] [url = chrome://devtools/content/debugger/new/index.html]
[task 2016-12-20T18:05:58.376959Z] 18:05:58 INFO - --DOMWINDOW == 56 (0x7fb3be3a0800) [pid = 1257] [serial = 212] [outer = (nil)] [url = about:blank]
[task 2016-12-20T18:05:58.378585Z] 18:05:58 INFO - --DOMWINDOW == 55 (0x7fb3bf298000) [pid = 1257] [serial = 194] [outer = (nil)] [url = http://example.com/browser/devtools/client/debugger/new/test/mochitest/examples/doc-scripts.html]
[task 2016-12-20T18:05:58.380051Z] 18:05:58 INFO - --DOMWINDOW == 54 (0x7fb3bba77000) [pid = 1257] [serial = 218] [outer = (nil)] [url = about:blank]
[task 2016-12-20T18:06:03.320575Z] 18:06:03 INFO - [1257] WARNING: Trying to expose eval or Function to non-subsuming content!: file /home/worker/workspace/build/src/js/xpconnect/wrappers/WrapperFactory.cpp, line 530
[task 2016-12-20T18:06:03.320683Z] 18:06:03 INFO - Assertion failure: !handler->hasSecurityPolicy(), at /home/worker/workspace/build/src/js/xpconnect/wrappers/WrapperFactory.cpp:334
[task 2016-12-20T18:06:31.820174Z] 18:06:31 INFO - #01: JSCompartment::getOrCreateWrapper [js/src/jscompartment.cpp:410]
[task 2016-12-20T18:06:31.820398Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.822199Z] 18:06:31 INFO - #02: JSCompartment::wrap [js/src/jscompartment.cpp:456]
[task 2016-12-20T18:06:31.822774Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.823830Z] 18:06:31 INFO - #03: JSCompartment::wrap [js/src/jscompartmentinlines.h:119]
[task 2016-12-20T18:06:31.824604Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.825418Z] 18:06:31 INFO - #04: JSCompartment::wrap [js/src/jscompartment.cpp:518]
[task 2016-12-20T18:06:31.826203Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.827054Z] 18:06:31 INFO - #05: js::Proxy::getOwnPropertyDescriptor [js/src/proxy/Proxy.cpp:118]
[task 2016-12-20T18:06:31.827797Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.828723Z] 18:06:31 INFO - #06: js::GetOwnPropertyDescriptor [js/src/jsobj.cpp:2666]
[task 2016-12-20T18:06:31.829408Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.830459Z] 18:06:31 INFO - #07: js::DebuggerObject::getOwnPropertyDescriptor [js/src/vm/Debugger.cpp:10015]
[task 2016-12-20T18:06:31.831198Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.831956Z] 18:06:31 INFO - #08: js::DebuggerObject::getOwnPropertyDescriptorMethod [js/src/vm/Debugger.cpp:9166]
[task 2016-12-20T18:06:31.832657Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.833274Z] 18:06:31 INFO - #09: js::CallJSNative [js/src/jscntxtinlines.h:240]
[task 2016-12-20T18:06:31.833687Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.834210Z] 18:06:31 INFO - #10: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:457]
[task 2016-12-20T18:06:31.834929Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.836319Z] 18:06:31 INFO - #11: js::Call [js/src/vm/Interpreter.cpp:521]
[task 2016-12-20T18:06:31.836959Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.837032Z] 18:06:31 INFO - #12: js::Wrapper::call [js/public/RootingAPI.h:794]
[task 2016-12-20T18:06:31.837719Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.838402Z] 18:06:31 INFO - #13: js::CrossCompartmentWrapper::call [js/src/proxy/CrossCompartmentWrapper.cpp:333]
[task 2016-12-20T18:06:31.838469Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.839166Z] 18:06:31 INFO - #14: js::Proxy::call [js/src/proxy/Proxy.cpp:400]
[task 2016-12-20T18:06:31.839229Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.839932Z] 18:06:31 INFO - #15: js::proxy_Call [js/src/proxy/Proxy.cpp:689]
[task 2016-12-20T18:06:31.839996Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.840700Z] 18:06:31 INFO - #16: js::CallJSNative [js/src/jscntxtinlines.h:240]
[task 2016-12-20T18:06:31.840762Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.841452Z] 18:06:31 INFO - #17: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:445]
[task 2016-12-20T18:06:31.841517Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.844204Z] 18:06:31 INFO - #18: js::Call [js/src/vm/Interpreter.cpp:521]
[task 2016-12-20T18:06:31.845797Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.847456Z] 18:06:31 INFO - #19: js::jit::InvokeFunction [js/src/jit/VMFunctions.cpp:114]
[task 2016-12-20T18:06:31.848953Z] 18:06:31 INFO -
[task 2016-12-20T18:06:31.850599Z] 18:06:31 INFO - #20: ??? (???:???)
[task 2016-12-20T18:06:31.852226Z] 18:06:31 INFO - ExceptionHandler::GenerateDump cloned child 1422
[task 2016-12-20T18:06:31.853846Z] 18:06:31 INFO - ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2016-12-20T18:06:31.855456Z] 18:06:31 INFO - ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2016-12-20T18:06:31.857103Z] 18:06:31 INFO - TEST-INFO | Main app process: exit 11
[task 2016-12-20T18:06:31.858547Z] 18:06:31 WARNING - TEST-UNEXPECTED-FAIL | ShutdownLeaks | process() called before end of test suite
Flags: needinfo?(ntim.bugs)
Updated•9 years ago
|
Flags: needinfo?(poirot.alex)
Comment 16•9 years ago
|
||
This isn't a leak failure but an assertion that you hit:
I imagine that is related to your modification made to object.js.
You may want to try:
let type = DevToolsUtils.getProperty(DevToolsUtils.getProperty(obj, "constructor"), "name");
If that's not enough, you should dump obj.class and DevToolsUtils.getProperty(obj, "constructor").class to have more information about what your are inspecting.
Flags: needinfo?(poirot.alex)
Comment 17•9 years ago
|
||
Also, I'm wondering if you could infer the constructor from obj.class directly, or is it always just Object or Function?
Or from obj.name or obj.displayName?
http://searchfox.org/mozilla-central/source/js/src/doc/Debugger/Debugger.Object.md#71-118
Or obj.proto.name/obj.proto.displayName?
We should always prefer using Debugger API when we can. As they are safe and prevent unwrapping references.
Reporter | ||
Comment 18•9 years ago
|
||
Thanks Alex! Will take a look at those solutions.
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 19•9 years ago
|
||
Reporter | ||
Comment 20•9 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #16)
> This isn't a leak failure but an assertion that you hit:
>
> I imagine that is related to your modification made to object.js.
> You may want to try:
> let type = DevToolsUtils.getProperty(DevToolsUtils.getProperty(obj,
> "constructor"), "name");
> If that's not enough, you should dump obj.class and
> DevToolsUtils.getProperty(obj, "constructor").class to have more information
> about what your are inspecting.
Unfortunately the assertion failure is still there (see comment 19).
(In reply to Alexandre Poirot [:ochameau] from comment #17)
> Also, I'm wondering if you could infer the constructor from obj.class
> directly, or is it always just Object or Function?
It's always Function unfortunately.
> Or from obj.name or obj.displayName?
> http://searchfox.org/mozilla-central/source/js/src/doc/Debugger/Debugger.
> Object.md#71-118
obj.name is the same as obj.displayName, and they don't include any info about the function type.
> Or obj.proto.name/obj.proto.displayName?
Those are undefined.
> We should always prefer using Debugger API when we can. As they are safe and
> prevent unwrapping references.
Unfortunately, constructor is the only place where I can get the relevant info, is there any other way to get it without hitting the assertion failure?
Flags: needinfo?(poirot.alex)
Comment 21•9 years ago
|
||
I'm running short of suggestions here... You might have to tweak the Debugger API to be safe. Or may be the assertion you hit highlights something wrong in C++?
I think Jim is a better contact for questions about the Debugger API.
Flags: needinfo?(poirot.alex) → needinfo?(jimb)
Assignee | ||
Comment 22•9 years ago
|
||
Hey Tim,
As you may know, we are planning to move Reps to Github (https://github.com/jasonLaster/devtools-reps) soon (hopefully in the next couple weeks).
Do you mind if I create a PR with the reps change (to function.js and the test) from your patch and re-purpose this bug to only add the async properties on the server side ?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 23•9 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #22)
> Hey Tim,
>
> As you may know, we are planning to move Reps to Github
> (https://github.com/jasonLaster/devtools-reps) soon (hopefully in the next
> couple weeks).
> Do you mind if I create a PR with the reps change (to function.js and the
> test) from your patch and re-purpose this bug to only add the async
> properties on the server side ?
Sure, go ahead :)
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 24•9 years ago
|
||
Assignee | ||
Comment 25•9 years ago
|
||
Re-purposing this as server-side only since work on Reps have been ported to GH https://github.com/devtools-html/devtools-reps/pull/54
Jim, would you have some time to have a look at the failure detailed in Comment 15 ?
Summary: Reps: display when a function is a generator or an async function → Object actor should have properties to indicate if a function is a generator or an async function
Reporter | ||
Updated•8 years ago
|
Assignee: ntim.bugs → nobody
Comment 26•8 years ago
|
||
Sorry for dropping this.
I think there are two action items for me here:
1) The Debugger API should have methods for recognizing async functions and generators. These should be straightforward.
2) There's the assertion failure in comment 15. I'll see if I can still reproduce that.
Flags: needinfo?(jimb)
Comment 27•8 years ago
|
||
Filed bug 1380498 for the additions to the Debugger API.
Depends on: 1380498
Comment 28•8 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #16)
> This isn't a leak failure but an assertion that you hit:
I've been able to reproduce this locally. I used:
$ hg checkout 70b75b80fb80
$ mach clobber
$ mach configure --disable-rust
$ mach test devtools/client/debugger/new/test/mochitest/browser_dbg-scopes.js
I don't know why the `--disable-rust` argument should be necessary, but that revision won't even build on my machine without it.
Comment 29•8 years ago
|
||
Okay, bug 1380498 has landed, so the new accessor properties isAsyncFunction and isGeneratorFunction are now available on Debugger.Object and Debugger.Script on M-C. This should help simplify the server side code for this bug.
These accessors will never throw. They simply return false when applied to a proxy.
Assignee | ||
Comment 30•8 years ago
|
||
Tim, do you want to take a look at this again ? If not I'll do it
Flags: needinfo?(ntim.bugs)
Comment 32•8 years ago
|
||
(In reply to Jim Blandy :jimb from comment #28)
> I've been able to reproduce this locally. I used:
This should be:
$ hg checkout 70b75b80fb80
$ mach clobber
$ mach configure --disable-rust
$ mach build
$ mach test devtools/client/debugger/new/test/mochitest/browser_dbg-scopes.js
(missing "mach build", perhaps obvious)
Comment 33•8 years ago
|
||
The mochitest that crashes is checking that the two scope nodes display the function's locals and then the global scope. This generates a preview of the global, which in a mochitest includes the SpecialPowers object:
https://developer.mozilla.org/en-US/docs/SpecialPowers
The SpecialPowers object has an accessor property named "Components". We try to create a value grip for its getter. Given a Debugger.Object DO referring to the getter, we call DO.getOwnPropertyDescriptor("constructor") to see what sort of object it is. This then crashes attempting to rewrap the property descriptor for the debug server compartment, failing this assertion:
} else if (CompartmentPrivate::Get(origin)->forcePermissiveCOWs) {
// Similarly, if this is a privileged scope that has opted to make itself
// accessible to the world (allowed only during automation), unwrap should
// be allowed.
MOZ_ASSERT(!handler->hasSecurityPolicy());
From: http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/js/xpconnect/wrappers/WrapperFactory.cpp#371
Here, 'origin' is the compartment to which the object being wrapped belongs. This apparently has forcePermissiveCOWs set, which I think means that it is the SpecialPowers compartment: http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/testing/specialpowers/content/specialpowersAPI.js#38
This makes sense: we're looking at cons_desc.value in:
let sp_desc = Object.getOwnPropertyDescriptor(window, 'SpecialPowers');
let getter = sp_desc.get;
let cons_desc = Object.getOwnPropertyDescriptor(getter, 'constructor');
So what I guess I need to understand is why WrapperFactory::ReWrap has selected a wrapper handler that implements some security policy, even though the object's compartment says that it should not.
Comment 34•8 years ago
|
||
Okay, and now it makes sense why the change to the reps code would cause this crash. It adds the following lines:
--- a/devtools/server/actors/object.js
+++ b/devtools/server/actors/object.js
@@ -1127,11 +1127,15 @@
grip.parameterNames = obj.parameterNames;
}
+ let type = DevToolsUtils.getProperty(obj, "constructor").name;
+ grip.isAsync = type === "AsyncFunction";
+ grip.isGenerator = type === "GeneratorFunction";
+
// Check if the developer has added a de-facto standard displayName
// property for us to use.
let userDisplayName;
Using the "constructor" property to produce meaningful previews for functions ought to be fine, but it uncovered an inconsistency in our wrapper machinery.
:ntim, since bug 1380498 has landed, this patch can be rewritten without touching the 'constructor' property, which will work around this crash. I'll file a separate bug for the crash, but you should be able to revise and resubmit this patch. The new isGeneratorFunction and isAsyncFunction accessors are documented here:
https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Object
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 35•8 years ago
|
||
Hey Jim,
I took over this bug some time ago and tried to use the 2 new isAsyncFunction and isGeneratorFunction.
It was partially working, i.e. I could get isAsyncFunction and al. to return true only if I was paused in the debugger.
If I was just evaluating `let fn = async () => {};fn;` in the console, isAsyncFunction returned false.
I think this has to do with the fact that we require the referent to be a debuggee function, but I'm not sure since it's a part of the devtools I don't really know about.
Could you explain me why we need the referent to be a debuggee function for those ? Thanks !
Flags: needinfo?(jimb)
Comment 36•8 years ago
|
||
We don't actually need those to check debuggee-ness; I implemented it that way because that's what we do for properties like script, etc. But other accessors don't; for example, Debugger.Object.prototype.isCallable doesn't check whether the referent is a debuggee function. Perhaps isAsyncFunction and isGeneratorFunction should behave more like isCallable.
I'll file a follow-up bug.
Flags: needinfo?(jimb)
Updated•8 years ago
|
Flags: needinfo?(ntim.bugs)
Comment 37•8 years ago
|
||
Err, not a follow-up but a blocker.
Comment 38•8 years ago
|
||
I've reproduced the crash described in comment 16; filed as bug 1394598. It has nothing to do with Debugger.
Comment 39•8 years ago
|
||
jimb | bgrins: Does the web console not make the tab's window objects debuggees? Nicolas' comment 35 here suggests it does not
I think it does?
The console uses the debugger object
http://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole.js#64
function WebConsoleActor(...) {
...
this.dbg = this.parentActor.makeDebugger();
defined on the tab actor, like this, which should cover all tab window objects (not just the top level one)
http://searchfox.org/mozilla-central/source/devtools/server/actors/tab.js#211-216
this.makeDebugger = makeDebugger.bind(null, {
findDebuggees: () => {
return this.windows.concat(this.webextensionsContentScriptGlobals);
},
shouldAddNewGlobalAsDebuggee: this._shouldAddNewGlobalAsDebuggee
});
makeDebugger ends up calling Debugger API addDebuggee
http://searchfox.org/mozilla-central/source/devtools/server/actors/utils/make-debugger.js#78-81
dbg.addDebuggees = function () {
for (let global of findDebuggees(this)) {
safeAddDebuggee(this, global);
}
http://searchfox.org/mozilla-central/source/devtools/server/actors/utils/make-debugger.js#78-81
function safeAddDebuggee(dbg, global) {
try {
let wrappedGlobal = dbg.addDebuggee(global);
dbg.addDebuggees is called on first window ready, when the thread actor is attached
http://searchfox.org/mozilla-central/source/devtools/server/actors/tab.js#1253-1257
if (threadActor.attached) {
threadActor.dbg.addDebuggees();
}
And we attach the thread actor on toolbox startup:
http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#431
this._threadClient = yield attachThread(this);
I verified and I see dbg.addDebuggee() being called with the top level window object on a simple document.
So I would say, yes, tab window should be a debuggee, but may be there is a race?
Comment 40•8 years ago
|
||
Does the console perhaps add and remove the global as a debuggee around evaluations? Being a debuggee used to have a performance impact.
Comment 41•8 years ago
|
||
(In reply to Jim Blandy :jimb from comment #40)
> Does the console perhaps add and remove the global as a debuggee around
> evaluations? Being a debuggee used to have a performance impact.
It seems it does not. So I'm mystified as to why that's not a debuggee function in comment 35.
Comment 42•8 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #35)
> I took over this bug some time ago and tried to use the 2 new
> isAsyncFunction and isGeneratorFunction.
> It was partially working, i.e. I could get isAsyncFunction and al. to return
> true only if I was paused in the debugger.
> If I was just evaluating `let fn = async () => {};fn;` in the console,
> isAsyncFunction returned false.
>
> I think this has to do with the fact that we require the referent to be a
> debuggee function, but I'm not sure since it's a part of the devtools I
> don't really know about.
Could you post the working patch you had? I'd like to investigate this further.
I think it should be fine to actually have isAsyncFunction not care whether the referent is a debuggee function or not; in other words, bug 1390955 would be a good change. But I'm unhappy that the server isn't behaving as I expect, so I'd like to debug that.
Flags: needinfo?(nchevobbe)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•8 years ago
|
||
Sure, it's really small and only map the new properties in the function grip. The test is passing I assume because the debugger is paused when we evaluate the packet.
If you apply the patch locally, you can do the following.
1. open the console
2. evaluate `async () => {}`
3. it will output "function {}"
4. evaluate `debugger;`, this will pause
5. evaluate `async () => {}` again in the console while paused
6. it will output the expected "async function {}"
Flags: needinfo?(nchevobbe)
Updated•7 years ago
|
Product: Firefox → DevTools
Assignee | ||
Comment 45•6 years ago
|
||
This will allow Reps to consume those and
display more accurate functions informations.
Updated•6 years ago
|
Assignee: nobody → nchevobbe
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Attachment #8913642 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8820257 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8820224 -
Attachment is obsolete: true
Comment 46•6 years ago
|
||
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a92a3752b5b4
Surface isAsync and isGenerator properties in function grips. r=Honza.
Comment 47•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox73:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 73
You need to log in
before you can comment on or make changes to this bug.
Description
•