1142577 - Use JS::AutoSetAsyncStackForNewCalls for setTimeout and setInterval

Status: RESOLVED DUPLICATE of bug 1148593

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

Description

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

This way, we will get nice stacks that span asynchronous operations.

Comment 1

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

Attachment: Use JS::AutoSetAsyncStackForNewCalls for setTimeout and setInterval; r=?

Comment 2

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

Just realized that this doesn't do much yet because only DOM exceptions use SavedFrame stacks at the moment. Gotta fix bug 1038238 before this is really useful.

Comment 3

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

Attachment: Use JS::AutoSetAsyncStackForNewCalls for setTimeout and setInterval

Comment 4

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

Attachment: Part 0: Use JS::AutoSetAsyncStackForNewCalls for setTimeout and setInterval

Comment 5

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

Attachment: Part 1: Add a test for async stacks across setTimeout

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8577662 [details] [diff] [review]
Part 1: Add a test for async stacks across setTimeout

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

Test this cross-global too, to make sure there are no compartment mismatches or anything.

Comment 7

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

Attachment: Part 1: Add a test for async stacks across setTimeout

Comment 8

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

Added a version that uses setTimeout and a callback from different compartments.

Comment 9

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

Attachment: Part 0: Use JS::AutoSetAsyncStackForNewCalls for setTimeout and setInterval

Comment 10

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

Attachment: Part 1: Add a test for async stacks across setTimeout

Comment 11

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

Rebased, carrying over r+.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17a19e13654c

Comment 12

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

Attachment: Part 0: Use JS::AutoSetAsyncStackForNewCalls for setTimeout and setInterval

Comment 13

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

Attachment: Part 1: Add a test for async stacks across setTimeout

Comment 14

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

Rebased again and fixed a bug with interaction between the CC and the GC and a lack of mozilla::HoldJSObjects calls.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0b24ce8735f

Comment 15

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

Looks like I just need to update a Promise + stacks test that is using setTimeout and getting more stack frames than it used to.

Comment 16

[:Paolo Amadini]

Does implementing this for setTimeout also include setImmediate as a result?

Comment 17

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

(In reply to :Paolo Amadini from comment #16)
> Does implementing this for setTimeout also include setImmediate as a result?

These patches don't.

Also, it seems setImmediate is only implemented for WorkerDebuggerGlobalScope -- not for general windows. This matches my recollection that we weren't going to implement setImmediate in Gecko for some reason or other.

Comment 18

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #17)
> (In reply to :Paolo Amadini from comment #16)
> > Does implementing this for setTimeout also include setImmediate as a result?
> 
> These patches don't.
> 
> Also, it seems setImmediate is only implemented for
> WorkerDebuggerGlobalScope -- not for general windows. This matches my
> recollection that we weren't going to implement setImmediate in Gecko for
> some reason or other.

WorkerDebuggerGlobalScope::setImmediate is a really special case. We can't use the WorkerGlobalScope version of setTimeout and setInterval in the worker debugger, because those functions are not aware of the debugger event queue. Consequently, they won't work inside nested event loops.

We need some way to schedule a runnable on the debugger's event queue, mainly so we can use Promise.jsm promises in the worker debugger, but we don't need full timer support. Which is why setImmediate is defined on WorkerDebuggerGlobalScope, and nowhere else.

Hope that helps.

Comment 19

[Tom Tromey :tromey]

FWIW this is fixed more generically by the patch in bug 1148593.