Closed Bug 1083851 Opened 10 years ago Closed 10 years ago

Capture some stack information in Promises

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(4 files, 1 obsolete file)

There's some info on this in https://etherpad.mozilla.org/promise-debugging but to smmarize the stack bit, the goal is to store the following: 1) Creation stack. This is a stack snapshot from when the promise is created. Two possible complications, both of which I'm going to ignore for the moment, I think: A) When doing then(null, null) maybe copy over the creation stack. B) When creating a Promise inside a then() callback, maybe chain up the creation stack to its parent's "then" stack. 2) Rejection stack. This is where Promise.reject() or the rejection callback handed out in the Promise ctor is called. A possible improvement would be to also set this from the thrown exception if rejecting due to an exception in a then callback, but that's blocked on bug 1038238. I filed bug 1083849 on this. 3) Resolution stack. We also talked about some sort of "causality stack" but for now we've decided that the concept we have for it in the etherpad is not very useful and that we should focus on some sort of generic "async stack" thing instead to serve that use case.
Attachment #8506205 - Flags: review?(nsm.nikhil)
Attachment #8506206 - Flags: review?(nsm.nikhil)
Attachment #8506208 - Flags: review?(nsm.nikhil)
Attachment #8506210 - Flags: review?(nsm.nikhil)
Comment on attachment 8506205 [details] [diff] [review] part 1. Capture allocation stacks at promise creation time Review of attachment 8506205 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/promise/Promise.cpp @@ +358,5 @@ > + if (!CaptureStack(cx, mAllocationStack)) { > + JS_ClearPendingException(cx); > + aRv.Throw(NS_ERROR_OUT_OF_MEMORY); > + return; > + } trailing white space
Comment on attachment 8506205 [details] [diff] [review] part 1. Capture allocation stacks at promise creation time Review of attachment 8506205 [details] [diff] [review]: ----------------------------------------------------------------- r+ but if we can optimize the common non-debugger case, that would be great. ::: dom/promise/Promise.cpp @@ +1199,5 @@ > +bool > +Promise::CaptureStack(JSContext* aCx, JS::Heap<JSObject*>& aTarget) > +{ > + JS::Rooted<JSObject*> stack(aCx); > + if (!JS::CaptureCurrentStack(aCx, &stack)) { How expensive an operation is this? Can we have it happen only when devtools has been opened? Similar to how the network panel in devtools only logs traffic when the page is reloaded while the panel is open.
Attachment #8506205 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8506210 [details] [diff] [review] part 4. Expose promise stacks on PromiseDebugging Review of attachment 8506210 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/promise/PromiseDebugging.cpp @@ +39,5 @@ > +/* static */ void > +PromiseDebugging::GetAllocationStack(GlobalObject&, Promise& aPromise, > + JS::MutableHandle<JSObject*> aStack) > +{ > + aStack.set(aPromise.mAllocationStack); I am not qualified to review this part, because I don't know how our compartments stuff works in this case. Does PromiseDebugging run under the same global as the window/worker the Promise is from? If PromiseDebugging is invoked from chrome code on a promise from content, will there be problems with security or GC or something? I'd like to know the answer, but someone else should review this. Same for the rejection and fulfillment stacks. ::: dom/webidl/PromiseDebugging.webidl @@ +22,5 @@ > + * Return the stack to the promise's allocation point. This can > + * return null if the promise was not created from script. > + */ > + static object? getAllocationStack(Promise<any> p); > + /** Nit: Newline before comment. @@ +28,5 @@ > + * rejection happened from script. This can return null if the > + * promise has not been rejected or was not rejected from script. > + */ > + static object? getRejectionStack(Promise<any> p); > + /** Nit: Ditto.
Attachment #8506210 - Flags: review?(nsm.nikhil)
> How expensive an operation is this? It depends on the stack depth. It's somewhere on the order of 1-4 microseconds on my laptop for stacks that are 0-20 frames deep. For comparison, allocating a promise without the stack trace bit is about 1 microsecond via Promise.resolve() and 2 microseconds via new Promise, on the same hardware. Nick has some ideas for speeding up the stack thing even more in cases when multiple stacks are taken at similar points in the code. > Can we have it happen only when devtools has been opened? We could, if we decide the performance hit is not something we're willing to take. The network panel experience may not be a good match for debugging promises, because network stuff mostly happens during pageload while promise debugging is likely to be later during the page lifetime... If we happen to have real-life non-synthetic testcases that heavily use promises, I woudl be very interested in trying them out with and without this patch to see how much of an issue this is in practice. > Does PromiseDebugging run under the same global as the window/worker the Promise is from? Not necessarily (and in fact most likely not). The binding code will make sure to wrap the thing put in the MutableHandle into the appropriate compartment. > If PromiseDebugging is invoked from chrome code on a promise from content That's the expected use case, fwiw. > will there be problems with security Shouldn't be: it gets Xrays in the normal way. > or GC or something? Also no, see hueyfix. ;) Thank you for letting me know you're not comfortable reviewing that part; I'll find another reviewer. I'll fix the newlines.
Attachment #8506591 - Flags: review?(peterv)
Attachment #8506210 - Attachment is obsolete: true
Attachment #8506591 - Flags: review?(peterv) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: