:Paolo Amadini Assignee
	Description • 10 years ago

We can now set async stacks from bug 1083359 when calling DOM Promise handlers.

	:Paolo Amadini Assignee
	Comment 1 • 10 years ago

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.

	:Paolo Amadini Assignee
	Comment 2 • 10 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?

	Jim Blandy :jimb
	Comment 3 • 10 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.

	:Paolo Amadini Assignee
	Comment 4 • 10 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

	:Paolo Amadini Assignee
	Comment 5 • 10 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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 6 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/c5fc760d1401