1084525 - Implement remote debugging protocol support for observing promises and their fulfillments/rejections

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Warning: a bunch of brain dump going on here.

Types of things we should support over the RDP, and how we can go about it:

a) When the frontend first starts observing promises, we can find all live promises in the
   debuggees via `Debugger.prototype.findObjects({ class: "Promise" })` to populate the
   frontend's display.

b) We can use the Debugger.prototype.onNewPromise hook to notify the frontend of new promise
   allocations, so it can keep its list of promises up to date.

c) We can use the Debuger.prototype.onPromiseSettled hook to notify the frontend of state
   changes, so it can keep each promise it is displaying up to date.

d) To get the various bits of internal data out of promises, we have the PromiseDebugging
   webidl interface. This requires operating on actual Promise instances, not Debugger.Object
   wrappers. Rather than unwrapping D.Os everywhere, I think we should monkey patch the
   `Debugger.Object.prototype` object with a corresponding method for each of the methods on
   PromiseDebugging that does the unwrapping, gets the data and returns it. This way we can
   encapsulate the unwrapping and the debugger logic doesn't ever have to deal with unwrapped
   debuggee objects directly.

   We should use those above methods to extend the ObjectActor's form + request types as it
   makes sense to expose data about individual promises over the RDP. This might happen in
   another bug. Originally, the plan was to do this part in bug 1033153, but it seems taken
   over as a meta bug for general "private data that can be exposed as PromiseDebugging
   methods" type bugs. Maybe that's still the best place to do it.

Still need to think about the exact implementation of packet forms, notifications, and request/response interactions. I'm leaning towards a "live list" (like the root actor's tab list), where you can get the whole list up front and then once you've done that you can get a single notification when that the list has changed and if you want the new list after that, you just re-request the whole list (or maybe the subset of the list that is new?). This avoids too much RDP traffic for every single change, and doesn't require any synchronization work by the client. I think sending a packet per promise (like we do with sources, for example) could become too much RDP traffic.

Maybe we can do the same thing for settlements? A single notification when at least one promise we are observing has settled, and then you can request all the promise states at once or maybe just the states of promises that have settled since last time you requested these states. The thing is, we will definitely need a per-promise state request type just to show promise state for promises that come in scope as you step in the debugger. Maybe we should only do that latter, per-promise state request option, if we have to implement it anyways. Why duplicate work?

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Create initial PromiseActor skeleton

List of modifications:
- Updated the unit test to the new style
- I renamed and abstracted startTestDebuggerServer from startTestPromiseServer since we can easily reuse this function in other tests

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Refactor expectState from memory-bridge.js to common.js

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8611440 [details] [diff] [review]
Part 1: Create initial PromiseActor skeleton

Review of attachment 8611440 [details] [diff] [review]:
-----------------------------------------------------------------

v nice

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8611457 [details] [diff] [review]
Part 2: Refactor expectState from memory-bridge.js to common.js

Review of attachment 8611457 [details] [diff] [review]:
-----------------------------------------------------------------

Yup.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Add attach and detach methods to PromiseActor

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8611499 [details] [diff] [review]
Part 3: Add attach and detach methods to PromiseActor

Review of attachment 8611499 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1084525-part-4.patch

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Add listPromises method to Promise Actor

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Add listPromises method to Promise Actor [1.0]

Added docs

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Add onNewPromise event handler to Promise Actor

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 6: Add onPromiseSettled event handler to Promise Actor

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Add onNewPromise event handler to Promise Actor WIP

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Add onNewPromise event handler to Promise Actor WIP2

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8613733 [details] [diff] [review]
Part 5: Add onNewPromise event handler to Promise Actor WIP2

Review of attachment 8613733 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/promise.js
@@ +26,5 @@
>  
> +  events: {
> +    // Event emitted for new promises allocated in debuggee and bufferred so
> +    // we send them in a batch.
> +    "new-promise": {

Nit: Since we send an array of new promises, we should probably name the event "new-promises" rather than "new-promise".

@@ +183,5 @@
> +
> +      if (needsScheduling) {
> +        DevToolsUtils.executeSoon(() => {
> +          events.emit(this, eventName, array);
> +          array = [];

We need to use the splice trick here, or else we will leak the very first array of promises that are still attached to the actor at `this._newPromises` because this line only rebinds the local `array` binding and not the reference from `this`.

Alternatively, pass in a property name and replace this line with:

  this[arrayPropertyName] = [];

And change the other references to `array` in a similar way throughout the body of this method.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8613178 [details] [diff] [review]
Part 4: Add listPromises method to Promise Actor [1.0]

Review of attachment 8613178 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

Aside: let's rename this file to promises.js and the actor to PromisesActor. PromiseActor implies an actor for a single promise / per promise, when the reality is that we just reuse ObjectActor for each promise. This is an actor for all debuggee promises -- plural -- and the name should reflect that.

::: toolkit/devtools/server/actors/promise.js
@@ +118,5 @@
> +        createValueGrip(v, this._navigationLifetimePool, this.objectGrip),
> +      sources: () => DevToolsUtils.reportException("PromiseActor",
> +        Error("sources not yet implemented")),
> +      createEnvironmentActor: () => DevToolsUtils.reportException(
> +        "PromiseActor", Error("sources not yet implemented"))

s/sources/createEnvironmentActor/

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Add onNewPromise event handler to Promise Actor [1.0]

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Create initial PromisesActor skeleton [2.0]

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Refactor expectState from memory-bridge.js to common.js [2.0]

Rebased

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Add attach and detach methods to PromisesActor [2.0]

Rebased and renamed to PromisesActor

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Add listPromises method to PromisesActor [2.0]

Rebased and renamed to PromisesActor. Addressed feedback.

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Add onNewPromise event handler to PromisesActor [2.0]

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8613882 [details] [diff] [review]
Part 5: Add onNewPromise event handler to PromisesActor [2.0]

Review of attachment 8613882 [details] [diff] [review]:
-----------------------------------------------------------------

Nice

Comment 23

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 6: Add onPromiseSettled event handler to Promise Actor [1.0]

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8614186 [details] [diff] [review]
Part 6: Add onPromiseSettled event handler to Promise Actor [1.0]

Review of attachment 8614186 [details] [diff] [review]:
-----------------------------------------------------------------

Great! One nit and one requested change below.

::: toolkit/devtools/server/actors/promises.js
@@ +30,5 @@
>      "new-promises": {
>        type: "new-promises",
>        data: Arg(0, "array:ObjectActor"),
> +    },
> +    // Event emitted for promise settlements

Nit: missing a period here.

::: toolkit/devtools/server/tests/unit/test_promises_actor_onpromisesettled.js
@@ +72,5 @@
> +            p.promiseState.value === resolution) {
> +          resolve(true);
> +        } else if (p.promiseState.state === "rejected" &&
> +                   p.promiseState.reason === resolution) {
> +          resolve(true);

Instead of always resolving if we find a fulfillment or rejection with the expected value, let's explicitly pass in a parameter specifying whether we expect it to be fulfilled or rejected. This makes the test just a little tighter.

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 6: Add onPromiseSettled event handler to Promise Actor [2.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96209a1ec09b

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/30d14e139b54
https://hg.mozilla.org/integration/fx-team/rev/46dc2eb4a78f
https://hg.mozilla.org/integration/fx-team/rev/42cc92d26e81
https://hg.mozilla.org/integration/fx-team/rev/cd7f45fd5149
https://hg.mozilla.org/integration/fx-team/rev/acf78260ac79
https://hg.mozilla.org/integration/fx-team/rev/31de5b06a647

Comment 27

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Expose Promise life time in object grip

Comment 28

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 8: Expose Promise time to settle in object grip

Comment 29

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8614421 [details] [diff] [review]
Part 7: Expose Promise life time in object grip

Review of attachment 8614421 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with quick test asserting that

p = new Promise
expected = Date.now()

onNewPromise(p) => {
  assert p.creationTime about equal to expected (say within 2 ms)
}

Comment 30

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8614422 [details] [diff] [review]
Part 8: Expose Promise time to settle in object grip

Review of attachment 8614422 [details] [diff] [review]:
-----------------------------------------------------------------

Would like to do a quick re-review when you have tests for:

* Promise.resolve() and Promise.reject() should both have a time-to-settle of 0

* return new Promise(resolve => setTimeout(resolve, 100)) should have time-to-settle of about 100 ms

* A promise that is not settled should not have a time-to-settle

Comment 31

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #29)
> Comment on attachment 8614421 [details] [diff] [review]
> Part 7: Expose Promise life time in object grip
> 
> Review of attachment 8614421 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with quick test asserting that
> 
> p = new Promise
> expected = Date.now()
> 
> onNewPromise(p) => {
>   assert p.creationTime about equal to expected (say within 2 ms)
> }

Or, even better:

start = Date.now()
p = new Promise
end = Date.now();

onNewPromise:
  assert that start <= p.creationTimestamp <= end

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30d14e139b54
https://hg.mozilla.org/mozilla-central/rev/46dc2eb4a78f
https://hg.mozilla.org/mozilla-central/rev/42cc92d26e81
https://hg.mozilla.org/mozilla-central/rev/cd7f45fd5149
https://hg.mozilla.org/mozilla-central/rev/acf78260ac79
https://hg.mozilla.org/mozilla-central/rev/31de5b06a647

Comment 33

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Implemeneted getDependentPromises method in ObjectClient

Comment 34

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Implement getDependentPromises method in ObjectClient [1.0]

Comment 35

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8614966 [details] [diff] [review]
Part 9: Implement getDependentPromises method in ObjectClient [1.0]

Review of attachment 8614966 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work!

::: toolkit/devtools/client/dbg-client.jsm
@@ +2426,5 @@
> +  }, {
> +    before: function(aPacket) {
> +      if (this._grip.class !== "Promise") {
> +        throw new Error("getDependentPromises is only valid for promise " +
> +          "grips.");

You can use template (backtick) strings for multiline strings.

::: toolkit/devtools/server/actors/object.js
@@ +528,5 @@
> +  onDependentPromises: function() {
> +    if (this.obj.class != "Promise") {
> +      return { error: "objectNotPromise",
> +               message: "'dependentPromises' request is only valid for " +
> +                        "object grips with a 'Promise' class." };

ditto re: multiline strings

Comment 36

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8614966 [details] [diff] [review]
Part 9: Implement getDependentPromises method in ObjectClient [1.0]

Review of attachment 8614966 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/client/dbg-client.jsm
@@ +2426,5 @@
> +  }, {
> +    before: function(aPacket) {
> +      if (this._grip.class !== "Promise") {
> +        throw new Error("getDependentPromises is only valid for promise " +
> +          "grips.");

Template strings would put "grips" on a new line and we don't actually want multiple lines here.

Comment 37

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Gabriel Luong [:gl] from comment #36)
> Comment on attachment 8614966 [details] [diff] [review]
> Part 9: Implement getDependentPromises method in ObjectClient [1.0]
> 
> Review of attachment 8614966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +2426,5 @@
> > +  }, {
> > +    before: function(aPacket) {
> > +      if (this._grip.class !== "Promise") {
> > +        throw new Error("getDependentPromises is only valid for promise " +
> > +          "grips.");
> 
> Template strings would put "grips" on a new line and we don't actually want
> multiple lines here.

Ah, you are of course correct.

Comment 38

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 10: Implement getAllocationStack method in ObjectClient WIP

Comment 39

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 10: Implement getAllocationStack method in ObjectClient WIP

Comment 40

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 10: Implement getAllocationStack method in ObjectClient WIP

Comment 41

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8616099 [details] [diff] [review]
Part 10: Implement getAllocationStack method in ObjectClient WIP

Review of attachment 8616099 [details] [diff] [review]:
-----------------------------------------------------------------

Extra f? jlongster because he is the other person who really knows what's going on with sources these days, especially when used outside of the debugger. I think we need a way to ensure that TabSources can be populated on demand, and doesn't rely on the ThreadActor. The more I think about it, the more I think we might want to organize a vidyo meeting (and possibly postpone source mapping for now).

::: toolkit/devtools/server/actors/object.js
@@ +543,5 @@
> +
> +  /**
> +   * Handle a protocol request to get the allocation stack of a promise.
> +   */
> +  onAllocationStack: Task.async(function*() {

It used to be that we couldn't use Task.jsm in actors used by the debugger, and I'm not sure if that restriction has been lifted, or if it still even applies anymore now that we moved the ObjectActor out to its own module.

I think the issue might have been that Task.jsm uses Promise.jsm (and not native DOM Promises) and Promise.jsm is not worker-compatible, which the ThreadActor and related actors must be.

Additional f? ejpbruel for more info / guidance here.

@@ +566,5 @@
> +  }),
> +
> +  _getSourceOriginalLocation: function(stack) {
> +    return this.hooks.sources().getOriginalLocation(new GeneratedLocation(
> +      this.hooks.sources().createNonSourceMappedActor(stack.source),

createNonSourceMappedActor takes a Debugger.Source instance, not a source url/filename string which is what SavedFrame.prototype.source gives. I think the best thing we can do now is hope that the source actor for the generated source has already been created and do TabSources.prototype.getSourceActorByURL.

Note that because we don't have a Debugger.Source (which has the full source text / etc needed to make a SourceActor) we can't create the actual SourceActor ourselves. I think that fixing this properly would involve some platform work, and perhaps some TabSources refactoring as well. I don't think we want to go too far down this rabbit hole here.

::: toolkit/devtools/server/tests/unit/test_promises_client_getallocationstack.js
@@ +62,5 @@
> +      for (let p of promises) {
> +        if (p.preview.ownProperties.name &&
> +            p.preview.ownProperties.name.value === "p") {
> +          grip = p;
> +          resolve();

Why not `resolve(p)` and then do `let grip = yield onNewPromise;` below? I think explicitly returning values is much more clear and less error prone than mutating shared variables.

Comment 42

[James Long (:jlongster)]

Comment on attachment 8616099 [details] [diff] [review]
Part 10: Implement getAllocationStack method in ObjectClient WIP

Review of attachment 8616099 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/client/dbg-client.jsm
@@ +2435,5 @@
> +
> +  /**
> +   * Request the stack to the promise's allocation point.
> +   */
> +  getAllocationStack: DebuggerClient.requester({

If this is only available for promises, can we rename this to make that more explicit? Or do we hope in the future to be able to call this for any object?

::: toolkit/devtools/server/actors/object.js
@@ +566,5 @@
> +  }),
> +
> +  _getSourceOriginalLocation: function(stack) {
> +    return this.hooks.sources().getOriginalLocation(new GeneratedLocation(
> +      this.hooks.sources().createNonSourceMappedActor(stack.source),

What is `stack.source` here, a URL? What happens for eval sources?

We really do need to improve how sources are interacting with everything. What you theoretically should be doing is calling `getSourceActorByURL`. I want to populate `TabSources` on demand when the toolbox is opened anyway, we need it far too often to try to make it lazy somehow.

We're going to talk about it more at Whistler. Until then I would recommend just not supporting sourcemaps.

Comment 43

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8616099 [details] [diff] [review]
Part 10: Implement getAllocationStack method in ObjectClient WIP

The problem with using Task.jsm in script actors is that we cannot use JSMs in workers (because Components is not available, neither is Cu.import).

It should be possible to work around that by converting Task.jsm to a CommonJS module, and in fact, we have done something similar for Promise.jsm (converting Promise-backend.js into a CommonJS module). Until we do, however, Task.jsm is off-limit for the script actors.

In case you decide to port Task.jsm to workers, another thing to watch out for is that native DOM promises are not aware of nested event loops created by the debugger, so they will not behave like expected if you use them in the debugger on a worker thread. I've refactored Promise.jsm so that it *is* aware of nested event loops in workers, so you can use that instead.

Comment 44

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Expose Promise life time in object grip [1.0]

I seem to be getting the same date timestamp for:
start = Date.now()
p = new Promise()
end = Date.now()

I tried new Date() and I would still get the exact same timestamp for start and end. Looking for feedback on what to do.

Comment 45

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8616777 [details] [diff] [review]
Part 7: Expose Promise life time in object grip [1.0]

Review of attachment 8616777 [details] [diff] [review]:
-----------------------------------------------------------------

It's ok to have start and end be the same number of ms, we are just using them as an inclusive bound: `start <= allocTimestamp <= end`. Note that we are using `<=`, and not `<`.

Rather than taking `start` and `end` timestamps inside the functions passed into `testPromiseCreationTimestamp` and then sticking them on the promise as properties, I think it would be cleaner if `testPromiseCreationTimestamp` took them itself. This way we wouldn't be repeating that in both functions passed in, nor dealing with funky grip properties.

  function* testPromiseCreationTimestamp(client, form, makePromise) {
    ...

    let start = Date.now();
    let promise = makePromise();
    let end = Date.now();

    ...

    ok(start <= creationTimestamp);
    ok(creationTimestamp <= end);

    ...
  }

Comment 46

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to James Long (:jlongster) from comment #42)
> Comment on attachment 8616099 [details] [diff] [review]
> Part 10: Implement getAllocationStack method in ObjectClient WIP
> 
> Review of attachment 8616099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +2435,5 @@
> > +
> > +  /**
> > +   * Request the stack to the promise's allocation point.
> > +   */
> > +  getAllocationStack: DebuggerClient.requester({
> 
> If this is only available for promises, can we rename this to make that more
> explicit? Or do we hope in the future to be able to call this for any object?\

We could totally (and should) check Debugger.Object.prototype.allocationSite if the object is not a promise.

> ::: toolkit/devtools/server/actors/object.js
> @@ +566,5 @@
> > +  }),
> > +
> > +  _getSourceOriginalLocation: function(stack) {
> > +    return this.hooks.sources().getOriginalLocation(new GeneratedLocation(
> > +      this.hooks.sources().createNonSourceMappedActor(stack.source),
> 
> What is `stack.source` here, a URL? What happens for eval sources?

Stack is a SavedFrame stack, so source is the filename/URL string. For eval sources, that means it is something like `foo.js:234 > eval`.

> We really do need to improve how sources are interacting with everything.
> What you theoretically should be doing is calling `getSourceActorByURL`. I
> want to populate `TabSources` on demand when the toolbox is opened anyway,
> we need it far too often to try to make it lazy somehow.
> 
> We're going to talk about it more at Whistler. Until then I would recommend
> just not supporting sourcemaps.

I think the problem isn't actually source maps, the more that I think about it. We only have a URL and no D.Source, so even non-source-mapped sources are a problem if the TabSources isn't populated.

Gabriel, we're going to have to work around this by manually ensuring that the TabSources is populated while we are in the attached state ourselves. That means doing something like `dbg.findScripts().forEach(s => populateTabSources(s.source))` and adding a `dbg.onNewScript = s => populateTabSources(s.source)` hook on attach. Then on detach, removing the `onNewScript` hook. I think the `populateTabSources` hand waving should be a call to `TabSources.prototype.createSourceActors`, but I'm not 100% sure. I'll let you verify that. After the source actors have been created, then you can just go ahead and get the original, potentially-source-mapped locations no problem.

Comment 47

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Eddy Bruel [:ejpbruel] from comment #43)

Gabriel, in light of the information that Eddy provided, I recommend avoiding Task.jsm for now and rewriting with raw promises. Let's limit the number of rabbit holes we have to go down.

Comment 48

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 8: Expose Promise time to settle in object grip [1.0]

Added test for:

* Promise.resolve() and Promise.reject() should both have a time-to-settle of 0

* return new Promise(resolve => setTimeout(resolve, 100)) should have time-to-settle of about 100 ms

* A promise that is not settled should not have a time-to-settle

Comment 49

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8616837 [details] [diff] [review]
Part 8: Expose Promise time to settle in object grip [1.0]

Review of attachment 8616837 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: toolkit/devtools/server/actors/object.js
@@ +146,5 @@
>        PromiseDebugging.getPromiseLifetime(rawPromise);
>  
> +    try {
> +      promiseState.timeToSettle = PromiseDebugging.getTimeToSettle(rawPromise);
> +    } catch(e) {}

Should have mentioned this before (sorry), but we should comment why we are swallowing errors here. Something like "If the promise is not settled, getTimeToSettle will throw an error, and we avoid adding the timeToSettle property."

Comment 50

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Expose Promise life time in object grip [2.0]

Comment 51

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8616957 [details] [diff] [review]
Part 7: Expose Promise life time in object grip [2.0]

Review of attachment 8616957 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/tests/unit/test_promises_object_creationtimestamp.js
@@ +59,5 @@
> +  ok(grip, "Found our new promise.");
> +
> +  let creationTimestamp = grip.promiseState.creationTimestamp;
> +
> +  ok(start <= creationTimestamp <= end, "Expect promise creation timestamp " +

Unfortunately, you have to split this into two comparisons. Unlike python, JS doesn't desugar multiple comparisons "properly"; it's just left associative.

Take this for example:

  js> 3 > 2 > 1
  false

This is because it is parsed as `(3 > 2) > 1`, which is equivalent to `true > 1`, which is equivalent to `1 > 1`, which is false. Hooray for weakly typed languages!

Split it out into `start <= creationTimestamp && creationTimestamp <= end`.

Comment 52

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Expose Promise life time in object grip [3.0]

I split the checks to `start - 1 <= creationTimestamp && creationTimestamp <= end + 1` to avoid an intermittent failure with Date.now() getting an earlier end time. Doing various Math round/ceil/floor did not help either and should be avoided.

Comment 53

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 8: Expose Promise time to settle in object grip [2.0]

Added comment on why we catch for PromiseDebugging.getTimeToSettle

Comment 54

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Implement getDependentPromises method in ObjectClient [2.0]

Rebased

Comment 55

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f0967ab6c21

Comment 56

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 8: Expose Promise time to settle in object grip [3.0]

Switched to flooring the time to settle instead of rounding since we expect it to be 0.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37933f187e22

Comment 57

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f0c15d17566f
https://hg.mozilla.org/integration/fx-team/rev/ae4f4b3fd22f
https://hg.mozilla.org/integration/fx-team/rev/57ec499fafb5

Comment 58

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f0c15d17566f
https://hg.mozilla.org/mozilla-central/rev/ae4f4b3fd22f
https://hg.mozilla.org/mozilla-central/rev/57ec499fafb5

Comment 59

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 10: Implement getAllocationStack method in ObjectClient WIP2

Comment 60

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 11: Add test for asserting the list of Promise objects returned from the PromiseActor onPromiseSettled event handler

test_promises_Actor_onpromisesettled.js was removed in bug 1164564, but only the assertion that the settlement time is equal to 0 should've been removed. We found that check that to be racy, but test_promises_object_timetosettle-02.js should be sufficient in checking the approximate time to settle.

Comment 61

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 10: Implement getAllocationStack method in ObjectClient [1.0]

Comment 62

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 11: Add test for asserting the list of Promise objects returned from the PromiseActor onPromiseSettled event handler [1.0]

Comment 63

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 12: Fix eslint complaints in promise.js [1.0]

Comment 64

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8623104 [details] [diff] [review]
Part 10: Implement getAllocationStack method in ObjectClient [1.0]

Review of attachment 8623104 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

Comment 65

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8623106 [details] [diff] [review]
Part 11: Add test for asserting the list of Promise objects returned from the PromiseActor onPromiseSettled event handler [1.0]

Review of attachment 8623106 [details] [diff] [review]:
-----------------------------------------------------------------

Mmmmmm love me some tests

Comment 66

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9711613cb0c

Comment 67

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 10: Implement getAllocationStack method in ObjectClient [2.0]

Added a try catch in the case getSourceByURL fails.

This error was happening in the test in the try runs (See https://treeherder.mozilla.org/logviewer.html#?job_id=8664044&repo=try):
2 INFO Console message: [JavaScript Error: "error occurred while processing 'allocationStack: Error: getSourceByURL: could not find source for chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js
Stack: TabSources.prototype.getSourceActorByURL@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/utils/TabSources.js:237:11
ObjectActor.prototype._getSourceOriginalLocation@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/object.js:585:18
ObjectActor.prototype.onAllocationStack@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/object.js:561:22
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1598:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:569:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
EventLoop.prototype.enter@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:339:5
ThreadActor.prototype._pushThreadPause@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:531:5
ThreadActor.prototype._pauseAndRespond@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:732:7
ThreadActor.prototype.onDebuggerStatement@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:1791:9
makePromises@http://example.com/browser/browser/devtools/debugger/test/doc_promise-get-allocation-stack.html:21:9
this.call@chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:18:10
@chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:71:12
Promise*@chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:70:3
Line: 237, column: 11" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js" line: 1467}]
onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'allocationStack: Error: getSourceByURL: could not find source for chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js\nStack: TabSources.prototype.getSourceActorByURL@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/utils/TabSources.js:237:11\nObjectActor.prototype._getSourceOriginalLocation@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/object.js:585:18\nObjectActor.prototype.onAllocationStack@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/object.js:561:22\nDSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1598:15\nLocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:569:11\nmakeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14\nmakeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14\nEventLoop.prototype.enter@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:339:5\nThreadActor.prototype._pushThreadPause@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:531:5\nThreadActor.prototype._pauseAndRespond@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:732:7\nThreadActor.prototype.onDebuggerStatement@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:1791:9\nmakePromises@http://example.com/browser/browser/devtools/debugger/test/doc_promise-get-allocation-stack.html:21:9\nthis.call@chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:18:10\n@chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:71:12\nPromise*@chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:70:3\nLine: 237, column: 11"}
Stack: DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:968:1
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:569:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
EventLoop.prototype.enter@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:339:5
ThreadActor.prototype._pushThreadPause@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:531:5
ThreadActor.prototype._pauseAndRespond@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:732:7
ThreadActor.prototype.onDebuggerStatement@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:1791:9
makePromises@http://example.com/browser/browser/devtools/debugger/test/doc_promise-get-allocation-stack.html:21:9
this.call@chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:18:10
@chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:71:12
Promise*@chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:70:3
Line: 968, column: 1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8883a1cf791a

Comment 68

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 13: Add test for asserting the Promise allocation stack in chrome debugging [1.0]

Comment 69

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a02d50df6d3b

Comment 70

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8624538 [details] [diff] [review]
Part 10: Implement getAllocationStack method in ObjectClient [2.0]

Review of attachment 8624538 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: toolkit/devtools/server/actors/object.js
@@ +584,5 @@
> +  _getSourceOriginalLocation: function(stack) {
> +    let source;
> +    try {
> +      source = this.hooks.sources().getSourceActorByURL(stack.source);
> +    } catch(e) {}

Document this catchall with a comment.

Comment 71

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8624860 [details] [diff] [review]
Part 13: Add test for asserting the Promise allocation stack in chrome debugging [1.0]

Review of attachment 8624860 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/debugger/test/browser.ini
@@ +350,5 @@
>  skip-if = e10s && debug
>  [browser_dbg_promises-allocation-stack.js]
>  skip-if = e10a && debug
> +[browser_dbg_promises-chrome-allocation-stack.js]
> +skip-if = e10a && debug

"e10a"

Comment 72

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 10: Implement getAllocationStack method in ObjectClient [3.0]

Comment 73

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 11: Add test for asserting the list of Promise objects returned from the PromiseActor onPromiseSettled event handler [2.0]

Comment 74

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 12: Fix eslint complaints in promise.js [2.0]

Comment 75

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 13: Add test for asserting the Promise allocation stack in chrome debugging [2.0]

Added requestLongerTimeout

Comment 76

[Gabriel Luong [:gl] (ΦωΦ)]

Try up to part 12 https://treeherder.mozilla.org/#/jobs?repo=try&revision=37cf3a41f568

Try up to part 13 https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d595d39649e

Comment 77

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4af7a65a8178
https://hg.mozilla.org/integration/fx-team/rev/7b21fda089ff
https://hg.mozilla.org/integration/fx-team/rev/060770ae1170
https://hg.mozilla.org/integration/fx-team/rev/6cc71c83d198

Comment 78

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4af7a65a8178
https://hg.mozilla.org/mozilla-central/rev/7b21fda089ff
https://hg.mozilla.org/mozilla-central/rev/060770ae1170
https://hg.mozilla.org/mozilla-central/rev/6cc71c83d198