Set an async stack when invoking promise handlers

RESOLVED FIXED in Firefox 39

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
We can now set async stacks from bug 1083359 when calling DOM Promise handlers.
(Assignee)

Comment 1

3 years ago
Created attachment 8574029 [details] [diff] [review]
The patch

Carrying over r+ from bug 1083359 comment 44 for the change.

Since this is the first use of async stacks, there might be adjustments needed in some tests that make assumptions about the stack structure, before this can land.

I think there is one instance where this (correctly) masks a stack from a nested event loop, and this triggers a bug in an xpcshell assertion stack capture.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8574029 - Flags: review+
(Assignee)

Comment 2

3 years ago
Comment on attachment 8574029 [details] [diff] [review]
The patch

In the meantime, asking more feedback on the actual code.

For example, this uses JS_NewStringCopyZ every time, maybe there is a better way?
Attachment #8574029 - Flags: feedback?(jimb)

Comment 3

3 years ago
Comment on attachment 8574029 [details] [diff] [review]
The patch

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

::: dom/promise/Promise.cpp
@@ +77,5 @@
>        return NS_OK;
>      }
>  
> +    JS::Rooted<JSObject*> asyncStack(cx, mPromise->mAllocationStack);
> +    JS::Rooted<JSString*> asyncCause(cx, JS_NewStringCopyZ(cx, "Promise"));

Short of calling JS_NewStringCopyZ at startup and stashing it somewhere rooted, I don't know of any way to avoid the string copy every time, outside the JS engine.

@@ +79,5 @@
>  
> +    JS::Rooted<JSObject*> asyncStack(cx, mPromise->mAllocationStack);
> +    JS::Rooted<JSString*> asyncCause(cx, JS_NewStringCopyZ(cx, "Promise"));
> +    if (!asyncCause)
> +      return NS_ERROR_OUT_OF_MEMORY;

That's going to leave an exception set on cx. Should we call JS_ClearPendingException before returning NS_ERROR_OUT_OF_MEMORY? I don't know how errors are propagated out here.
Attachment #8574029 - Flags: feedback?(jimb) → feedback+
(Assignee)

Comment 4

3 years ago
I've added JS_ClearPendingException and left the string creation in place in this tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d2bd0233b92
(Assignee)

Comment 5

3 years ago
And fixed one more test where we did actually get an async stack now:

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

Pushed to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/c5fc760d1401
https://hg.mozilla.org/mozilla-central/rev/c5fc760d1401
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Updated

3 years ago
Blocks: 1163139
You need to log in before you can comment on or make changes to this bug.