Closed Bug 1229769 Opened 4 years ago Closed 4 years ago

We should be able to use DOM promises in the worker debugger.

Categories

(Core :: DOM: Workers, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

The worker debugger uses a separate event queue for it's runnables. This allows us to suspend normal worker runnables when the debugger is paused, but still keep processing debugger runnables in a nested event loop.

The problem with this approach is that promises have their own event queue, known as a micro task queue. If we want to be able to suspend normal worker promises when the debugger is paused, but still keep processing debugger promises in a nested event loop, we need a separate micro task queue for promises as well.
Attached patch Proof of concept (obsolete) — Splinter Review
Here is a proof of concept that I believe should work. I'm putting this up for feedback rather than review because I'd like to make sure that the overall approach makes sense first.

In particular, I'm a bit worried about checking what thread we are on in each call to Promise::DispatchToMicroTask and Promise::PerformMicroTaskCheckpoint. How hot should we expect these code paths to be?

I'd also like to double check whether we're calling Promise::PerformMicroTaskCheckpoint appropriate time in WorkerPrivate::EnterDebuggerEventLoop. My reasoning was that since then handlers can schedule additional runnables, we need to perform a micro task checkpoint every time before checking for runnables to be processed.

If you have any other comments, or think this approach doesn't make sense, please let me know as well.
Attachment #8694727 - Flags: feedback?(khuey)
I think Boris and Till have been working on promise changes.  They may be interested in anything happening here.
I'm working on implementing promises in SpiderMonkey in bug 911216. I don't think that that has much bearing on this issue: the microtask queue management will always be up to the embedding.
I've done some experiments with this patch applied. When I replace the Promise.jsm promises in script.js (which is where most of the actors we use for worker debugging are defined) with DOM promises, the tests continue to work, but there is a significant delay of about 10 seconds.

It looks like DOM promises take a long time to be resolved, which must be due to the way I've implemented the debugger micro task queue. Kyle, any ideas as to what could be causing this delay?
Flags: needinfo?(khuey)
Comment on attachment 8694727 [details] [diff] [review]
Proof of concept

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

I sent email on September 29th with what I believe is a better way to implement this.

"The promise queue is currently effectively stored in TLS.  It hangs off of CycleCollectedJSRuntime (mPromiseMicroTaskQueue).  When we enter the debugger's nested event loop we should "set aside" the current queue of promises and when the stack is unwound we can restore it.  Then we don't need to change the promise code at all or tell where a promise lives.  This will work if there are never any content events that run while a debugger event loop is on the stack, which I believe is the case."

Did you try/think about this and reject it for some reason?

I don't know about the delay offhand.  But we could figure out (with rr or something) if we do want to take this approach.

::: dom/workers/WorkerPrivate.cpp
@@ +5436,2 @@
>      // Don't block with the periodic GC timer running.
> +    SetGCTimerMode(IdleTimer);

You should either justify or revert the changes to the GC timer scheduling here.  Why do you want to do it unconditionally?

::: dom/workers/WorkerScope.h
@@ +340,5 @@
>    virtual ~WorkerDebuggerGlobalScope();
>  };
>  
> +bool
> +IsWorkerGlobal(JSObject *object);

These are already declared in Workers.h  If you intend to move them they should actually move.
ni for previous comment.
Flags: needinfo?(khuey) → needinfo?(ejpbruel)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5)
> Comment on attachment 8694727 [details] [diff] [review]
> Proof of concept
> 
> Review of attachment 8694727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I sent email on September 29th with what I believe is a better way to
> implement this.
> 
> "The promise queue is currently effectively stored in TLS.  It hangs off of
> CycleCollectedJSRuntime (mPromiseMicroTaskQueue).  When we enter the
> debugger's nested event loop we should "set aside" the current queue of
> promises and when the stack is unwound we can restore it.  Then we don't
> need to change the promise code at all or tell where a promise lives.  This
> will work if there are never any content events that run while a debugger
> event loop is on the stack, which I believe is the case."
> 
> Did you try/think about this and reject it for some reason?

I read your suggestion as follows: if we can assume that all promises created inside the top-level event loop are content promises, and all promises created inside a nested event loop are debugger promises, then we can set aside the top-level queue when we enter the outermost nested event loop, and subsequently reuse the queue for debugger promises.

I don't think these assumptions hold, however. First of all, the debugger can create promises inside the top-level event loop (for instance, when a new source-mapped source is introduced, we asynchronously load the source map for that source), so not all promises created inside the top-level event loop are content promises. We definitely do not want to suspend debugger promises that were created inside the top-level event loop while we are inside a nested event loop.

Second, although it's true that no content events should be able to run while we are inside a nested event loop, it's still possible to evaluate code in the client's context (for instance using the webconsole). Such code is allowed to do things like Promise.resolve().then(function () { ... }), so not all promises created inside a nested event loop are debugger promises.

Third, there seems to be a second, tacit assumption here, which is that all debugger promises have been resolved when we leave the outermost nested event loop. We could copy those promises back to the original queue, but that would again violate the assumption that all promises on the top-level queue are content promises.

With this in mind, do you still disagree with my approach? Or have I missed anything in your approach that would address these issues?
Flags: needinfo?(ejpbruel)
Needinfo for comment 7
Flags: needinfo?(khuey)
(In reply to Eddy Bruel [:ejpbruel] from comment #7)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5)
> > Comment on attachment 8694727 [details] [diff] [review]
> > Proof of concept
> > 
> > Review of attachment 8694727 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I sent email on September 29th with what I believe is a better way to
> > implement this.
> > 
> > "The promise queue is currently effectively stored in TLS.  It hangs off of
> > CycleCollectedJSRuntime (mPromiseMicroTaskQueue).  When we enter the
> > debugger's nested event loop we should "set aside" the current queue of
> > promises and when the stack is unwound we can restore it.  Then we don't
> > need to change the promise code at all or tell where a promise lives.  This
> > will work if there are never any content events that run while a debugger
> > event loop is on the stack, which I believe is the case."
> > 
> > Did you try/think about this and reject it for some reason?
> 
> I read your suggestion as follows: if we can assume that all promises
> created inside the top-level event loop are content promises, and all
> promises created inside a nested event loop are debugger promises, then we
> can set aside the top-level queue when we enter the outermost nested event
> loop, and subsequently reuse the queue for debugger promises.

Yes.

> I don't think these assumptions hold, however. First of all, the debugger
> can create promises inside the top-level event loop (for instance, when a
> new source-mapped source is introduced, we asynchronously load the source
> map for that source), so not all promises created inside the top-level event
> loop are content promises. We definitely do not want to suspend debugger
> promises that were created inside the top-level event loop while we are
> inside a nested event loop.

That's only a problem if the debugger interrupts the debugger, I think.  So that may not be an issue.

> Second, although it's true that no content events should be able to run
> while we are inside a nested event loop, it's still possible to evaluate
> code in the client's context (for instance using the webconsole). Such code
> is allowed to do things like Promise.resolve().then(function () { ... }), so
> not all promises created inside a nested event loop are debugger promises.

That's more of a problem.  What semantics do we actually want there?  What happens on the main thread if you, from the debugger, trigger execution of some code that causes new promises to be created and immediately resolved?  What if they're created and resolved sometime later (say, from an async XHR response)?

> Third, there seems to be a second, tacit assumption here, which is that all
> debugger promises have been resolved when we leave the outermost nested
> event loop. We could copy those promises back to the original queue, but
> that would again violate the assumption that all promises on the top-level
> queue are content promises.

Yeah, that is an implicit assumption.  Is that not going to be true?
Flags: needinfo?(khuey) → needinfo?(ejpbruel)
(Based on your comment it does look like we need something more like what you propose and not what I propose, but I want to make sure I understand the desired semantics here.)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9)
> (In reply to Eddy Bruel [:ejpbruel] from comment #7)
> > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5)
> > > Comment on attachment 8694727 [details] [diff] [review]
> > > Proof of concept
> > > 
> > > Review of attachment 8694727 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I sent email on September 29th with what I believe is a better way to
> > > implement this.
> > > 
> > > "The promise queue is currently effectively stored in TLS.  It hangs off of
> > > CycleCollectedJSRuntime (mPromiseMicroTaskQueue).  When we enter the
> > > debugger's nested event loop we should "set aside" the current queue of
> > > promises and when the stack is unwound we can restore it.  Then we don't
> > > need to change the promise code at all or tell where a promise lives.  This
> > > will work if there are never any content events that run while a debugger
> > > event loop is on the stack, which I believe is the case."
> > > 
> > > Did you try/think about this and reject it for some reason?
> > 
> > I read your suggestion as follows: if we can assume that all promises
> > created inside the top-level event loop are content promises, and all
> > promises created inside a nested event loop are debugger promises, then we
> > can set aside the top-level queue when we enter the outermost nested event
> > loop, and subsequently reuse the queue for debugger promises.
> 
> Yes.
> 
> > I don't think these assumptions hold, however. First of all, the debugger
> > can create promises inside the top-level event loop (for instance, when a
> > new source-mapped source is introduced, we asynchronously load the source
> > map for that source), so not all promises created inside the top-level event
> > loop are content promises. We definitely do not want to suspend debugger
> > promises that were created inside the top-level event loop while we are
> > inside a nested event loop.
> 
> That's only a problem if the debugger interrupts the debugger, I think.  So
> that may not be an issue.
>

After thinking it through for a while, I think you are right. What we care about is not where Promises are created, but where then-handlers are scheduled. The micro task queue guarantees that any then-handlers that are scheduled during an event will be processed before the next event is processed.

It's definitely possible for the debugger to schedule then-handlers from the top level event loop. This happens, for instance, when a new source-mapped source is introduced while the debugger is not paused, and we need to load the source map for this source. So the assumption that all then-handlers created from the top level event loop are content then-handlers does not hold.

However, there is a weaker assumption which I think does hold, namely that there are no pending debugger then-handlers on the micro task queue when we enter a nested event loop. Since all then-handlers are processed before the next event, the only way this could happen is if we entered a nested event loop from within a then-handler, which I don't think we ever do.

This weaker assumption might be enough for the solution you proposed. However, that still leaves the other issues I brought up. See my comments below.

> > Second, although it's true that no content events should be able to run
> > while we are inside a nested event loop, it's still possible to evaluate
> > code in the client's context (for instance using the webconsole). Such code
> > is allowed to do things like Promise.resolve().then(function () { ... }), so
> > not all promises created inside a nested event loop are debugger promises.
> 
> That's more of a problem.  What semantics do we actually want there?  What
> happens on the main thread if you, from the debugger, trigger execution of
> some code that causes new promises to be created and immediately resolved? 
> What if they're created and resolved sometime later (say, from an async XHR
> response)?

The semantics we want is that any content code evaluated from the webconsole is executed immediately. However, any content then-handlers scheduled as a result should not be executed until the debugger resumes/we leave the nested event loop.

This breaks the assumption that all then-handlers scheduled inside a nested event loop are debugger then-handlers.

> 
> > Third, there seems to be a second, tacit assumption here, which is that all
> > debugger promises have been resolved when we leave the outermost nested
> > event loop. We could copy those promises back to the original queue, but
> > that would again violate the assumption that all promises on the top-level
> > queue are content promises.
> 
> Yeah, that is an implicit assumption.  Is that not going to be true?

The only way this could happen is if we exited a nested event loop while there are still then-handlers on the micro task queue. Unfortunately, unlike entering a nested event loop, it looks like we do in fact exit nested event loops from within then-handlers, so I don't think this assumption will hold.
Flags: needinfo?(ejpbruel)
Needinfo for comment 11
Flags: needinfo?(khuey)
(In reply to Eddy Bruel [:ejpbruel] from comment #11)
> This weaker assumption might be enough for the solution you proposed.
> However, that still leaves the other issues I brought up. See my comments
> below.

Right.

> The semantics we want is that any content code evaluated from the webconsole
> is executed immediately. However, any content then-handlers scheduled as a
> result should not be executed until the debugger resumes/we leave the nested
> event loop.
> 
> This breaks the assumption that all then-handlers scheduled inside a nested
> event loop are debugger then-handlers.

Yes, it does.  And that sinks my plan, I think.

> The only way this could happen is if we exited a nested event loop while
> there are still then-handlers on the micro task queue. Unfortunately, unlike
> entering a nested event loop, it looks like we do in fact exit nested event
> loops from within then-handlers, so I don't think this assumption will hold.

So now that I think about it a bit more, the question here doesn't even make sense.  "Exiting" a nested event loop is done by setting a flag, and then when the current event finishes running, the "enter" nested event loop call sees it and doesn't run any more events, returning to the caller.  So we will definitely have to finish processing the current event and its promise handlers before the nested event loop is actually "exited".

Anyways, we can't do the simple thing I suggested because of the "debugger invokes content code" case.
Flags: needinfo?(khuey)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #13)
> Anyways, we can't do the simple thing I suggested because of the "debugger
> invokes content code" case.

Although, how does that work on the main thread today?  The main thread doesn't have this distinction between regular and debugger microtask queues ...
Flags: needinfo?(ejpbruel)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #14)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #13)
> > Anyways, we can't do the simple thing I suggested because of the "debugger
> > invokes content code" case.
> 
> Although, how does that work on the main thread today?  The main thread
> doesn't have this distinction between regular and debugger microtask queues
> ...

That's right, on the main thread we don't have separate queues for the debugger. We therefore explicitly opt-out of those events that could cause client code to run, using the same synchronous event loop mechanism that is used for XHR. Unfortunately, there are many cases where we fail to do this at the moment, with Promise microtasks being one example. There is a meta bug open for these issues: bug 1074448.

The difficulties with suppressing events on the main thread was one of the main reasons that I designed the worker debugger API to have a separate queue for debugger runnables. This reverses the problem to explicitly opting-in to those events that we want to handle while the debugger is paused, which is comparatively much easier to deal with.
Flags: needinfo?(ejpbruel)
I started working on a new version of this patch, but ran into some issues during testing. In particular, when running a debugger promise task from the main event loop, we need to make sure to enter the debugger's compartment first.
Depends on: 1241841
Marking this as P3. This is not a user facing change, and is essentially just cleaning up the implementation.
Priority: -- → P3
Attached patch Patch to be reviewed (obsolete) — Splinter Review
As per our discussion in the bug, the simpler solution you proposed is not viable, so I cleaned up my original patch.

FWIW, I figured out what was causing the delay in the earlier version of this patch: I forgot to explicitly perform a microtask checkpoint after processing each debugger runnable in the top-level event loop (this is not necessary for normal runnables, since the CycleCollectedJSRuntime takes care of it there).
Attachment #8694727 - Attachment is obsolete: true
Attachment #8694727 - Flags: feedback?(khuey)
Attachment #8722901 - Flags: review?(khuey)
Depends on: 1252818
Can I get a review for this?
Flags: needinfo?(khuey)
Comment on attachment 8722901 [details] [diff] [review]
Patch to be reviewed

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

Moving in the right direction, but I'd much rather push the complexity into CycleCollectedJSRuntime here.  See comments.

::: dom/promise/Promise.cpp
@@ +912,5 @@
>    MaybeSomething(JS::NullHandleValue, &Promise::MaybeReject);
>  }
>  
>  bool
>  Promise::PerformMicroTaskCheckpoint()

I don't like the code duplication here.  You could definitely factor PerformMicroTaskCheckpoint and PerformWorkerDebuggerMicroTaskCheckpoint into one function that takes the queue to operate on as an argument.

@@ +949,5 @@
> +void
> +Promise::PerformWorkerMicroTaskCheckpoint()
> +{
> +  MOZ_ASSERT(!NS_IsMainThread(), "Wrong thread!");
> +  MOZ_ASSERT(DebuggerEventLoopLevel() > 0);

This doesn't actually compile.

@@ +984,5 @@
> +void
> +Promise::PerformWorkerDebuggerMicroTaskCheckpoint()
> +{
> +  MOZ_ASSERT(!NS_IsMainThread(), "Wrong thread!");
> +  MOZ_ASSERT(DebuggerEventLoopLevel() > 0);

Nor does this.

@@ +2478,2 @@
>  
> +  std::queue<nsCOMPtr<nsIRunnable>>* microtaskQueue = nullptr;

All of this logic belongs in CCJSR, I think.  You could add a function, "DispatchToMicrotask" or something, and override it on WorkerJSRuntime to handle the complexity.
Attachment #8722901 - Flags: review?(khuey) → review-
Also, it looks to me like this patch breaks the SPIDERMONKEY_PROMISE case.  Please don't do that.
I'm not against the idea, but there are a couple of reasons I implemented things the way I did. Before I start writing another patch, let's make sure we're on the same page here:

I did not coalesce PerformWorkerMicroTaskCheckpoint and PerformWorkerDebuggerMicrotaskCheckpoint into a single function that takes the queue to operate on as argument, because the former does not operate on a single queue at all: it operates on the debugger queue until it becomes empty, and *then* operates on the worker queue until that becomes empty.

To do what you suggest, we would have to change Perform*MicroTaskCheckpoint to Perform*Microtask, which takes a queue as argument, and then processes only a single microtask from the given queue. The actual logic for performing a microtask *checkpoint* (i.e. processing microtasks until the microtask queue(s) is/are empty) would then have to move to the cycle collected runtime.

If that's what you want to do, then there is a second issue. The cycle collected runtime is not the only place where we perform microtask checkpoints. We also do so here:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?from=WorkerPrivate.cpp#6047

If the code to perform a microtask checkpoint moves to the cycle collected runtime, this would would have to call into the CycleCollectedRuntime.

Is this how you want me to set things up?
Flags: needinfo?(khuey)
Boris, could you clarify comment 21 for me? I haven't seen the term SPIDERMONKEY_PROMISE before, so I get the feeling I'm missing something.
Flags: needinfo?(bzbarsky)
SPIDERMONKEY_PROMISE is the compile-time switch for enabling bug 911216.  It's not on yet, and the spidermonkey parts are not done, but the DOM/xpconnect parts _are_ done.  And this patch is not changing the code that would get used if that switch is flipped.
Flags: needinfo?(bzbarsky)
After thinking about this some more, I'm ok with you not making those changes (and I'll refactor this once I figure out exactly what I want to do).  But we still need to not break SPIDERMONKEY_PROMISE.
Flags: needinfo?(khuey)
Attached patch Patch to be reviewed (obsolete) — Splinter Review
Here's a new patch that should not break SPIDERMONKEY_PROMISE.

I simply moved DispatchToMicroTask from Promise to CycleCollectedJSRuntime, and reimplemented EnqueuePromiseJobCallback to use that method as well.

I also managed to do away with testing what thread we are on in DispatchToMicroTask. Instead, I implemented it as a virtual method that is overridden on the worker's runtime.

This is fairly close to khuey's suggestion to move most of the complexity to CycleCollectedJSRuntime. The only exception is that microtask handling still happens on dom::Promise.
Attachment #8722901 - Attachment is obsolete: true
Attachment #8731818 - Flags: review?(khuey)
Comment on attachment 8731818 [details] [diff] [review]
Patch to be reviewed

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

Almost there!

::: dom/promise/Promise.cpp
@@ +956,5 @@
> +
> +  for (;;) {
> +    // On worker threads, if we are not inside a debugger event loop, we first
> +    // use the debugger promise micro task queue. If the debugger promise micro
> +    // task queue is empty, we use the main promise micro task queue instead.

In this comment, make it clear that the "if we are not inside a debugger event loop" is why this function was called, and not a condition we need to check.

@@ +984,5 @@
> +void
> +Promise::PerformWorkerDebuggerMicroTaskCheckpoint()
> +{
> +  MOZ_ASSERT(!NS_IsMainThread(), "Wrong thread!");
> +  MOZ_ASSERT(DebuggerEventLoopLevel() > 0);

It seems odd that the DebuggerEventLoopLevel assertion is the same in both functions.  Why is that?

@@ +990,5 @@
> +  CycleCollectedJSRuntime* runtime = CycleCollectedJSRuntime::Get();
> +
> +  for (;;) {
> +    // On worker threads, if we are inside a debugger event loop, we always use
> +    // the debugger promise micro task queue.

Similarly here.

::: dom/workers/WorkerPrivate.cpp
@@ +4505,5 @@
>        static_cast<nsIRunnable*>(runnable)->Run();
>        runnable->Release();
>  
> +      // Flush the promise queue.
> +      Promise::PerformWorkerMicroTaskCheckpoint();

Why isn't this a WorkerDebuggerMicroTaskCheckpoint?
Attachment #8731818 - Flags: review?(khuey)
New patch with comments by khuey addressed.
Attachment #8731818 - Attachment is obsolete: true
Attachment #8733266 - Flags: review?(khuey)
Comment on attachment 8733266 [details] [diff] [review]
Patch to be reviewed

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

::: dom/promise/Promise.cpp
@@ +990,5 @@
> +  CycleCollectedJSRuntime* runtime = CycleCollectedJSRuntime::Get();
> +
> +  for (;;) {
> +    // We are on a worker thread, and inside a debugger event loop, so use the
> +    // debugger promise micro task queue.

so use only the debugger ...
Attachment #8733266 - Flags: review?(khuey) → review+
Try run for this patch, with final comment by khuey addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=36e50880daf9
Turns out that we don't only perform a debugger micro task check point inside a nested event loop, but also after processing a debugger runnable in the main run loop (per comment 18). I updated the assertion and comments accordingly. Here's a new try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4be984047c3
https://hg.mozilla.org/mozilla-central/rev/1052435cba5f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Is there a reason you didn't expose the _Promise interface in WorkerDebugger #ifdef SPIDERMONKEY_PROMISE?
Flags: needinfo?(ejpbruel)
(In reply to Boris Zbarsky [:bz] from comment #34)
> Is there a reason you didn't expose the _Promise interface in WorkerDebugger
> #ifdef SPIDERMONKEY_PROMISE?

No, that looks like an oversight.
Flags: needinfo?(ejpbruel)
Attachment #8735820 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.