DOM Promise handlers should be inspectable in the debugger to find memory leaks. This is currently possible in Promise.jsm: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise#Debugging STEPS TO REPRODUCE: - Open the Web Console (CTRL+SHIFT+K) - Type let p = new Promise(() => {}); p.then(() => "Kept in memory."); p - Click [object Promise] ACTUAL RESULTS: - No useful information for the object appears. EXPECTED RESULTS: - To compare with Promise.jsm, open the Browser Console (CTRL+SHIFT+J) - Type Components.utils.import("resource://gre/modules/Promise.jsm"); - Type let p = Promise.defer().promise; p.then(() => "Kept in memory."); p - Click [object Object] - Handlers should be visible To actually see the source, you currently need this sub-optimal operation: - Find the number within "{private:handlers:NN}". - Type p["{private:handlers:NN}"][0].onResolve.toSource();

Again, we can add APIs, but the debugger will need to actually use them.

(In reply to Boris Zbarsky [:bz] from comment #1) > Again, we can add APIs, but the debugger will need to actually use them. Do we plan something like inspectable Symbols / Private Names?

On the SpiderMonkey end? I don't know. Jason?

Yes, I am going to implement symbols, I think in Q2. After modules, before classes.

I misread the question. Private symbols are not in ES6 and they're kind of a nice-to-have feature at this point. It might be tricky to get them right. I don't plan to work on them. For WeakMaps there's Components.utils.nondeterministicGetWeakMapKeys(). This is analogous. No need to get fancy. That is, a language feature for granting a particular capability to chrome and only chrome code---that we have. The only thing we don't have is a nice principled place to put it. :-|

To put the question in context, we're looking for a way for the debugger to display some properties of an object that are however generally inaccessible from code, for debugging Promises. See the property "{private:status:7}" in the example here: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise#Debugging This is currently a real property in Promise.jsm, with a randomized name. How should we proceed for implementing this type of inspectability for a DOM Promise? Are you suggesting to use a chrome-only method to inspect an object's internal state, that the C++ code exposes using some special JS primitive? I think we'd like this state to be protected from accidental usage even in chrome code... it should not be easy to obtain (a Components.utils method could maybe serve the purpose). Private symbols was just a suggestion that had this characteristic.

What's the bar for ease of obtaining? If it can be found on the object with getOwnPropertyNames, are we ok with it having a deterministic name in chrome? If not, then is something like Promise.getInternalStateWhatever(myPromise) obfuscated enough? That's basically the same thing as a components.utils method but with less code scattered about...

(In reply to Boris Zbarsky [:bz] from comment #7) > If it can be found on the object with getOwnPropertyNames, are we ok with it > having a deterministic name in chrome? What I can say is that we'd like to avoid this: let p = Promise.resolve(myValue); if (p["internalValueAbsolutelyDontUseThis"] == 1) { // I know that the above shouldn't be used but figured out I'd do that for simplicity } I'd rather have the properties inaccessible to anything that isn't the debugger (which would request a special privilege or use a special API). > If not, then is something like Promise.getInternalStateWhatever(myPromise) > obfuscated enough? That's basically the same thing as a components.utils > method but with less code scattered about... We'd need to hard-wire the debugger to use the method on Promise objects in that case.

> I'd rather have the properties inaccessible to anything that isn't the debugger That argues for exposing the properties via some debugger-only API, then. > We'd need to hard-wire the debugger to use the method on Promise objects in that case. You need that anyway, since Promise objects at the moment don't have any JS-exposed state of the sort you're looking for.

And by "JS-exposed" I mean "exposed to the JS engine".

Oh, and I was wondering why I couldn't see any closures in the debugger when execution was paused!

I just looked at the Debugger.Object API and half the stuff in there is type-specific accessors. Maybe we should have a general mechanism: struct JS::Class { ... JSPropertySpec *debuggerAccessors; }; These accessors would be installed only on Debugger.Objects pointing to objects of that class. We could create prototype objects to hold them, as needed. We could maybe even move the implementation of Debugger.Object.prototype.{name,displayName,parameterNames,script,environment} to jsfun.cpp, and so on... though that is uncertain because those accessors need access to both Function guts and Debugger functionality. What do you think? Jim? Boris? Jim, I also noticed hostAnnotations. Is that supposed to be for this sort of thing? https://wiki.mozilla.org/Debugger

Hanging that sort of thing off generated WebIDL JSClasses would be pretty simple. So to the extent that a JS property is good enough to expose some functionality to the debugger, we could totally add [DebuggerOnly] IDL annotations and use them to annotate stuff for a setup like you propose. Certainly for things which are tightly coupled to the object implementation but not really to the debugger internals that would work great.

Yes, that's exactly what hostAnnotations is meant for.

The advantage of hostAnnotations over extending the prototype is that you get a separate namespace for the host annotations, rather than having to worry that additions to Debugger.Object.prototype are going to conflict with some host-provided property out there.

Given the above comments, can you provide a summary of what will be needed to obtain the Promise inspectability, and which other bugs might make sense to file as dependencies? I might be able to write some code starting next week, though I'd need some guidance on the details.

Jason and I need to talk about the design a bit. hostAnnotations needs to furnish structured data in a form appropriate to present to Debugger API consumers, while remaining innocent of the hairy details of Debugger.Object, cross-compartment wrappers, and so on. Once that's settled, we need to add a hostAnnotations hook to js::Class. There are other changes to js::Class being contemplated, which may affect the design. Then, we need to extend Debugger to actually call that new hook. Finally, we need to implement that hook in js::Classes of interest, like promises.

(In reply to Jim Blandy :jimb from comment #17) > Jason and I need to talk about the design a bit. Has any progress been made on the design here?

So exposing Promise internals using some [ChromeOnly] properties wouldn't be enough here?

(In reply to Olli Pettay [:smaug] from comment #19) > So exposing Promise internals using some [ChromeOnly] properties wouldn't be > enough here? I'd be all in favor of implementing the "simplest thing that works" if the "right way" proves to be complex and has too much dependencies. So, what do people think about Olli's suggestion? Or exposing an object with properties that are made code-inaccessible with a WeakMap or randomized names? Or something else?

Olli's suggestion is obviously the simplest to implement; that's what I was asking about in comment 7, more or less. It will basically stick some enumerable, deterministic-name properties on Promise.prototype, which totally fails your information-hiding requirements, but has the benefit of being doable in a very short timeframe. The chromeonly static method thing I suggested in comment 7 is also pretty simple to do. I'm pretty happy with the chromeonly static method approach, personally. We can try to do the naming in a way that makes it clear non-debugger code shouldn't use them.

(In reply to Boris Zbarsky [:bz] from comment #21) > I'm pretty happy with the chromeonly static method approach, personally. We > can try to do the naming in a way that makes it clear non-debugger code > shouldn't use them. Sounds good to me, given that comment 17 seems to suggest that proper information hiding is rocket science :-) We should hard-wire the debugger to extract that information from Promises.

Sounds great. Next question: what information do we want to expose? Presumably the list of resolve handlers and the list of rejection handlers, with the understanding that these will only be meaningful if the promise has not yet been resolved or rejected. Do we want to expose the resolution/rejection value if the promise has been resolved or rejected? Do we want to expose an explicit (resolved, rejected, pending) tristate?

(In reply to Boris Zbarsky [:bz] from comment #23) > Do we want to expose the > resolution/rejection value if the promise has been resolved or rejected? Do > we want to expose an explicit (resolved, rejected, pending) tristate? I think so! That's filed separately as bug 966471 but I think we can implement both at the same time if it turns out to be simpler.

To be clear, I think we can file a followup bug on the hostAnnotations bit, but we shouldn't block on it...

Where do we hook this into the debugger code?

Comment on attachment 8399612 [details] [diff] [review] Promise part of this Review of attachment 8399612 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/Promise.webidl @@ +46,5 @@ > static Promise race(sequence<any> iterable); > + > + // Static to keep people from just poking it by accident on objects > + [ChromeOnly] > + static PromiseDebuggerData getDebuggerData(Promise promise); Would it be easy to make this a static method of another object rather than a static method of Promise? @@ +62,5 @@ > + // > + // XXXbz Do we care when the callback is really another Promise, like in the > + // Promise.all()/Promise.race() cases? This API will just silently ignore > + // those, but we could make this a sequence<(Promise or AnyCallback)> if we > + // care about them. I didn't know this was possible in this implementation, but I'd say that at least the presence of a callback should be indicated. It might be a code correctness indication to the person using the debugger.

> Would it be easy to make this a static method of another object rather than a static > method of Promise? Generally, sure. What object are you thinking, specifically? I mean, we could create a separate [ChromeOnly] PromiseDebugger interface with a single static method... > I didn't know this was possible in this implementation, Well, there's no JS function that gets called when the promises passed to all() are resolved. Instead there's a C++ function which manages the counter on the return value of all(). Similar for race(). The question is what to return there. We could pretty easily return the promise that was the return value of all() or race(). Apart from that, anything that would be useful?

And also, I've been trying to figure out where Promise.jsm stuff is hooked into the debugger, but I can't find any useful hits on "private" or "handlers" in the debugger code... Any ideas where that's done right now?

(In reply to Boris Zbarsky [:bz] from comment #28) > Generally, sure. What object are you thinking, specifically? Components.utils, just because we have various other debugging / JS-engine-related methods there. > I mean, we could create a separate [ChromeOnly] PromiseDebugger interface > with a single static method... This would also work. > The question is what to return there. We could pretty easily return the > promise that was the return value of all() or race(). Apart from that, > anything that would be useful? That's probably the most useful reference we can provide. (In reply to Boris Zbarsky [:bz] from comment #29) > And also, I've been trying to figure out where Promise.jsm stuff is hooked > into the debugger, but I can't find any useful hits on "private" or > "handlers" in the debugger code... Any ideas where that's done right now? We use ordinary instance properties with randomized names.

> Components.utils Sure. It's pretty easy to hang it off there. Jim, any idea how best to expose this stuff in the debugger in the short term?

Jim is the expert here, but if we want something quick and dirty, I guess exposing a Debugger.Object.prototype.getPromiseDebuggerData() would fit the bill. The debugger data object returned would contain Debugger.Object wrappers for the debuggee values of course.

Debugger.Object.prototype.getPromiseDebuggerData() would return null when the Debugger.Object's referent isn't a DOM Promise object.

jorendorff and I have a date to talk about this tomorrow morning. In the proposals I've written in the past, the object's js::Class could provide a hook that produces an object retrieved by a Debugger.Object.prototype.hostAnnotations getter; said object could provide whatever introspection properties make sense for the given object class. Jason points out that many of our extant D.O accessors are actually class-specific already. The following apply only to Function instances: name, displayName, parameterNames, script, and environment. The following apply only to proxies: proxyHandler, proxyCallTrap, proxyConstructTrap. And so on. Some D.O methods are also class-specific: decompile, call, apply (for Function); evalInGlobal and evalInGlobalWithBindings (for global objects). He then argues that Function and JSCLASS_IS_GLOBAL should not be special, and that every class should be able to define its own accessors to appear on Debugger.Object instances. There's a risk of name collisions between generic Debugger.Object accessors, but I think we can manage that. As laid out in bug 961325 comment 43, we should aggressively specify well-defined common meanings for property names to help different classes avoid giving conflicting meanings to a given name, but we'll always have D.O.p.class as a discriminant if things get ugly.

Florian, afaik, the feature does not exist yet, right? If it is the case, it is a bit premature to add it in the release notes.

Sorry, I didn't expect this to pop up on your dashboard until the bug is resolved. I just wanted to make sure this gets relnoted once it's done, 'cause it's a huge win (and must) for WebDevs working with current and future APIs. Once this is finished, please renom for relnote.

Thanks. Btw, I agree with you, it is something what we want in the release notes!

So do we want to just check in handler inspector stuff building on the patch in bug 966471 and then just do debugger stuff on top of that? I'm really not quite sure who owns this bug at this point and what's left to be done on the DOM end.

Just a note: we definitely shouldn't put this on Components.utils because we're working on getting the debugger server to run in a worker, and Cu isn't available there. CC'ing :ejpbruel.

Right. The static PromiseDebugging namespace seems like a better choice there. That said, actually building functionality like "get all the live promises" will require some worker-specific bits, probably.

Making this bug for the platform bits and bug 1087624 for the devtools RDP + frontend.

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046 Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5. If you have questions, please contact :mdaly.