Closed Bug 966471 Opened 11 years ago Closed 10 years ago

DOM Promise state, value and reason should be inspectable in the debugger (like Promise.jsm)

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: Paolo, Assigned: bzbarsky)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-needed, Whiteboard: [Async])

Attachments

(1 file)

promise-debugging-bits
(deleted), patch
peterv
: review+
nsm
: review+
Details | Diff | Splinter Review 
DOM Promise state should be inspectable in the debugger, while the state should be inaccesible from code. 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 Promise.resolve(1);
- 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 Promise.resolve(1);
- Click [object Object]
- Promise state should be visible

The state is currently represented sub-optimally:
- The number near "{private:status:NN}" indicates pending, resolved, or rejected.
- The value of "{private:value:NN}" is the resolution value or rejection reason.
We could add some sort of chromeonly APIs here or whatnot, but the console will need to actually call into them, right?
Whiteboard: [Async]
FWIW the canonical form used by libraries for such "synchronous inspection" is to return one of the following:

{ state: "pending" }
{ state: "fulfilled", value: x }
{ state: "rejected", reason: r }

Be sure not to confuse states and fates, as you do in comment 0. https://github.com/domenic/promises-unwrapping/blob/master/docs/states-and-fates.md
Blocks: 1019721
It would be pretty trivial to add an API like this on Promise instances:

  [ChromeOnly]
  PromiseStateHolder getState();

where:

  dictionary PromiseStateHolder {
    PromiseState state = "pending";
    any value;
    any reason;
  };
  enum PromiseState { "pending", "fulfilled", "rejected" };

This would be much like comment 2, except that "value" and "reason" would always be set, to undefined in the cases when they're not appicable.

Alternately we could just have explicit separate getters for state/value/reason:

  [ChromeOnly]
  readonly attribute PromiseState state;
  [ChromeOnly]
  readonly attribute any fullfillmentValue;
  [ChromeOnly]
  readonly attribute any rejectionReason;

again, with fullfillmentValue and rejectionReason returning undefined if the state doesn't match up correctly.

Either one of those is pretty trivial to implement, for what it's worth.
Flags: needinfo?(paolo.mozmail)
How soon do we want this thing working on Web Workers? attribute access would be a little trickier to implement there.
Ah, yes.  What to do in workers is especially exciting, given that we're still figuring out our debugging story there in general...

That said, we have [ChromeOnly] in workers.  What we don't have are ways to then inspect such properties on objects in non-system workers.
(In reply to Boris Zbarsky [:bz] from comment #3)
> It would be pretty trivial to add an API like this on Promise instances:
> 
>   [ChromeOnly]
>   PromiseStateHolder getState();
> 
> where:
> 
>   dictionary PromiseStateHolder {
>     PromiseState state = "pending";
>     any value;
>     any reason;
>   };
>   enum PromiseState { "pending", "fulfilled", "rejected" };
> 
> This would be much like comment 2, except that "value" and "reason" would
> always be set, to undefined in the cases when they're not appicable.

I prefer this version because it discourages getting the resolution value without checking the state first.
(In reply to Boris Zbarsky [:bz] from comment #3)
>   [ChromeOnly]
>   PromiseStateHolder getState();

While [ChromeOnly] protects content from accessing the property, it would be easily accessible to privileged JavaScript code (and even suggested by autocomplete in the Browser Console), which would enable practices like promise state polling which we don't want, neither in content nor privileged code.

I think something like "PromiseDebugging.getState(Promise p)" could be better. It would be cool if we could implement an additional check so that this method can only be called by devtools, and raises an exception in other contexts.

The returned object looks good to me.
Flags: needinfo?(paolo.mozmail)
> I think something like "PromiseDebugging.getState(Promise p)" could be better

Pretty easy to do, sure.

> additional check so that this method can only be called by devtools

Define "devtools"?  I mean in terms that one can determine by examining things available to the callee in this case.
I guess a devtools-only restriction could be implemented if we added the helper function to the Debugger API. I think a Debugger.Object.prototype.isPromise and a Debugger.Object.prototype.getPromiseState would work. But I'm not sure how feasible it is to call the host object's methods (or those of a utility class) from SpiderMonkey. I know Jim had plans to expose a Debugger.Object.prototype.hostAnnotations property for such cases that were recently changed, but I don't know what the new plan is.
Flags: needinfo?(jimb)
Attached patch promise-debugging-bits (deleted) — — Splinter Review 
I don't think we should block this on complicated infrastructure like that that would take weeks to months to create even if we have a concrete plan.

This is a simple patch to implement the API we just talked about.

Note that there is similar stuff in https://bug966472.bugzilla.mozilla.org/attachment.cgi?id=8399612 as well that's been sitting there for months waiting on this generic infrastructure stuff...
The other downside of attaching this to the Debugger API is that it would require the page to be in debug mode and get kicked out of ion (for now at least).
Oh, one more thing.  With that patch, if you do |var p = Promise.resolve(5)| and then examine its state, it will show up as "pending".  This is because the actual promise implementation in this case does the change to a resolved state off a task, not synchronously.

This is not observable black-box without this debugging API, fwiw.
OK, if we don't want to further restrict the API to devtools then this approach sounds fine.
Flags: needinfo?(jimb)
(In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> The other downside of attaching this to the Debugger API is that it would
> require the page to be in debug mode and get kicked out of ion (for now at
> least).

When you call getState, you're already debugging anyways, since it is the debugger that gave you the reference to the Promise object in the first place. (Otherwise, you're using the API for state polling in running code, which must be avoided.)

Can we put a hard check in the method, verifying that it is indeed called in an environment suspended by the debugger?

(In reply to Boris Zbarsky [:bz] from comment #12)
> Oh, one more thing.  With that patch, if you do |var p = Promise.resolve(5)|
> and then examine its state

Good point, a breakpoint or "debugger" keyword placed after such a statement may stop execution when the promise "state" is "pending" instead of "fulfilled". Its "fate" would be "resolved", but we don't see that with the current API (I don't know if there is a need for that).
(In reply to :Paolo Amadini from comment #14)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> > The other downside of attaching this to the Debugger API is that it would
> > require the page to be in debug mode and get kicked out of ion (for now at
> > least).
> 
> When you call getState, you're already debugging anyways, since it is the
> debugger that gave you the reference to the Promise object in the first
> place. (Otherwise, you're using the API for state polling in running code,
> which must be avoided.)

Not necessarily. Although we're now leaning to making a promise tab inside the debugger's sidebar, initially we were considering making a whole promise panel in the devtools.

We don't just want to be able to inspect promises that happen to be in scope, we want to list all live promises and inspect their statuses, etc similar to the ember tool: https://bug1019721.bugzilla.mozilla.org/attachment.cgi?id=8433444. Thus, we need an API to get all promises for the page. See bug 1019721 for details.
> Can we put a hard check in the method, verifying that it is indeed called in an
> environment suspended by the debugger?

See end of comment 8, and keep in mind people debug via the console as well.

> I don't know if there is a need for that

Note that if needed we can change the underlying implementation here too.  I'm just pointing out that this is not a function of the API but of the underlying state of the promise's members.
(In reply to Nick Fitzgerald [:fitzgen] from comment #15)
> We don't just want to be able to inspect promises that happen to be in
> scope, we want to list all live promises and inspect their statuses, etc
> similar to the ember tool: Thus, we
> need an API to get all promises for the page. See bug 1019721 for details.

It seems this list would be similar or the same of what is needed for bug 989960, to make tests fail if there is any unhandled rejection still present at the end of a test. Good to see possible convergence here.
(In reply to Boris Zbarsky [:bz] from comment #16)
> > Can we put a hard check in the method, verifying that it is indeed called in an
> > environment suspended by the debugger?
> 
> See end of comment 8, and keep in mind people debug via the console as well.

Yeah, I see how there might no easy way to do this accurately in all the cases we support.

A common case that maybe we might avoid more easily is the promise being inspected as part of normal execution by the same code that created it. I'm not familiar with JS engine internals (this is why I'm asking questions rather than providing answers), but maybe there is something, like a discrepancy in the context/window/principal/anything, that can be used to approximate this check. If not, it's not a big deal.
There isn't anything obvious that wouldn't be very fragile.
Attachment #8442163 - Attachment is patch: true
Attachment #8442163 - Attachment mime type: message/rfc822 → text/plain
Attachment #8442163 - Flags: review?(peterv)
Attachment #8442163 - Flags: review?(nsm.nikhil)
Keywords: dev-doc-needed
Note that this still needs debugger integration, obviously.  I'm probably not the right person to do that.  Who is?
Blocks: 966472
Attachment #8442163 - Flags: review?(peterv) → review+
https://hg.mozilla.org/mozilla-central/rev/51654bc68de2
Assignee: nobody → bzbarsky
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: