1306121 - Add an embedding API for emulating V8's stack format for ErrorObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

In SpiderNode we've run into a roadblock emulating V8's error stack formatting without modifying SpiderMonkey.

It would be nice to add an embedding API that allows the embedder to choose the V8 stack format instead of SpiderMonkey's.

Comment 1

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

Here is a sketch of what needs to happen:

* Add an `enum class StackFormat { SpiderMonkey, V8 };` to `jsfriendapi.h` in the `js` namespace

* Add a `js::StackFormat stackFormat_` member to `JSRuntime` that is `js::StackFormat::SpiderMonkey` by default in the JSRuntime ctor

* Add an internal function to get the parent runtime's stack format:

    js::StackFormat stackFormat() const {
        JSRuntime* rt = this;
        while (rt->parentRuntime)
            rt = rt->parentRuntime;
        return rt->stackFormat_;
    }

  This will let the format be shared between a whole tree of JSRuntimes (so workers inherit the format)

* Add `void js::SetStackFormat(JSRuntime* rt, js::StackFormat format)` and `js::StackFormat js::GetStackFormat(JSRuntime* rt)` to `jsfriendapi.{h,cpp}`. These should assert that there is no parentRuntime, so that we don't get disagreements between worker JSRuntimes and their parent main thread JSRuntime.

* Add a `StackFormat` parameter to `JS::BuildStackString` with a default value of `cx->stackFormat()` (cx inherits from rt)
  * Move existing implementation to a static `BuildSpiderMonkeyStackString` function
  * Create a static `BuildV8StackString` function
  * Have `JS::BuildStackString` call the appropriate implementation based on the `StackFormat` parameter

* Test! Add something to js/src/jsapi-tests/testSavedStacks.cpp that sets the V8 stack format, captures a simple stack (see other tests in this file for an example), stringifies the captured stack, and assert that it looks like V8's format.

Let me know if anything is unclear or you have questions or anything -- good luck!

Comment 2

[(no longer active)]

Seems pretty straightforward.  Thanks for the suggestions!

Comment 3

[(no longer active)]

I ended up diverging from your suggestions a bit:

* I added StackFormat and the setter/getter to jsapi.h, because that's where BuildStackString is declared, and if the enum class is defined in jsfriendapi.h, it can't be used in jsapi.h.
* I added a Default value to StackFormat, so that when calling BuildStackString() you can either pass Default which means use the runtime's stack format, or pass the desired format explicitly.  I added assertions to make sure that you can't pass Default to SetStackString().  All child runtimes will have their stackFrame_ set to Default, and parent runtimes have their stackFrame_ set to SpiderMonkey by default.
* Instead of providing separate functions for building the entire stack string, I only refactored the code to format one frame out.  The looping logic stays the same across both stack formatting modes.  The only difference is that async stack frames are ignored in the V8 emulation mode.

Comment 4

[(no longer active)]

and also:

* I ended up accepting a JSContext* rather than JSRuntime* in the embedder facing API as per #jsapi.

Comment 5

[(no longer active)]

Attachment: Add support for emulating V8 stack frame formatting to SpiderMonkey

Comment 6

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

Comment on attachment 8796740 [details] [diff] [review]
Add support for emulating V8 stack frame formatting to SpiderMonkey

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

r=me with raciness addressed (sorry for not thinking of that originally!)

Thanks!

::: js/src/vm/Runtime.h
@@ +1277,5 @@
> +        while (rt->parentRuntime) {
> +            MOZ_ASSERT(rt->stackFormat_ == js::StackFormat::Default);
> +            rt = rt->parentRuntime;
> +        }
> +        MOZ_ASSERT(rt->stackFormat_ != js::StackFormat::Default);

Upon reflection, I think accessing `stackFormat_` this way will be racy (in a 99% benign way, but still https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong) so we should either make the member mozilla::Atomic (I think we only need RelAcq ordering) or inherit the stack format from the parent upon JSRuntime creation and always access our own copy rather than the parent's copy. Either way works for me.

::: js/src/vm/SavedStacks.cpp
@@ +945,5 @@
>      js::StringBuffer sb(cx);
>  
> +    if (format == js::StackFormat::Default) {
> +        format = cx->stackFormat();
> +    }

Nit: spidermonkey style is to drop {} from single line consequents.

@@ +975,5 @@
> +            switch (format) {
> +                case js::StackFormat::SpiderMonkey:
> +                    if (!FormatSpiderMonkeyStackFrame(cx, sb, frame, indent, skippedAsync)) {
> +                        return false;
> +                    }

Nit: drop {}

@@ +980,5 @@
> +                    break;
> +                case js::StackFormat::V8:
> +                    if (!FormatV8StackFrame(cx, sb, frame, indent, !nextFrame)) {
> +                        return false;
> +                    }

ditto

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55734588d50e
Add support for emulating V8 stack frame formatting to SpiderMonkey; r=fitzgen

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing SM test saved-stacks/async-principals.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b50d8e02f67

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=55734588d50e58e06485436d6bc98ddbc69b0c42
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=36930275&repo=mozilla-inbound

[task 2016-10-03T18:48:10.837558Z] TEST-PASS | js/src/jit-test/tests/saved-stacks/async.js | Success (code 0, args "")
[task 2016-10-03T18:48:10.843785Z] high.checkVisibleStack(stackD)
[task 2016-10-03T18:48:10.843863Z] low.checkVisibleStack(stackD)
[task 2016-10-03T18:48:10.843912Z] eval:18:5 Error: Assertion failed: got "a", expected "Async*a"
[task 2016-10-03T18:48:10.843935Z] Stack:
[task 2016-10-03T18:48:10.843958Z]   checkVisibleStack@eval:18:5
[task 2016-10-03T18:48:10.843975Z]   Async*e@eval:3:3
[task 2016-10-03T18:48:10.843991Z]   a@eval:2:3
[task 2016-10-03T18:48:10.844028Z] Exit code: 3
[task 2016-10-03T18:48:10.844084Z] FAIL - saved-stacks/async-principals.js
[task 2016-10-03T18:48:10.844208Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/saved-stacks/async-principals.js | eval:18:5 Error: Assertion failed: got "a", expected "Async*a" (code 3, args "--baseline-eager")
[task 2016-10-03T18:48:10.844266Z] INFO exit-status     : 3
[task 2016-10-03T18:48:10.844334Z] INFO timed-out       : False
[task 2016-10-03T18:48:10.844404Z] INFO stdout          > high.checkVisibleStack(stackD)
[task 2016-10-03T18:48:10.844454Z] INFO stdout          > low.checkVisibleStack(stackD)
[task 2016-10-03T18:48:10.844510Z] INFO stderr         2> eval:18:5 Error: Assertion failed: got "a", expected "Async*a"
[task 2016-10-03T18:48:10.844539Z] INFO stderr         2> Stack:
[task 2016-10-03T18:48:10.844571Z] INFO stderr         2> checkVisibleStack@eval:18:5
[task 2016-10-03T18:48:10.844598Z] INFO stderr         2> Async*e@eval:3:3
[task 2016-10-03T18:48:10.844624Z] INFO stderr         2> a@eval:2:3
[task 2016-10-03T18:48:10.852244Z] TEST-PASS | js/src/jit-test/tests/saved-stacks/async.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
[task 2016-10-03T18:48:10.863424Z] TEST-PASS | js/src/jit-test/tests/saved-stacks/async.js | Success (code 0, args "--baseline-eager")
[task 2016-10-03T18:48:10.867119Z] TEST-PASS | js/src/jit-test/tests/saved-stacks/async.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off")
[task 2016-10-03T18:48:10.870144Z] TEST-PASS | js/src/jit-test/tests/saved-stacks/async.js | Success (code 0, args "--no-baseline --no-ion")
[task 2016-10-03T18:48:10.882933Z] high.checkVisibleStack(stackD)
[task 2016-10-03T18:48:10.882998Z] low.checkVisibleStack(stackD)
[task 2016-10-03T18:48:10.883034Z] eval:18:5 Error: Assertion failed: got "a", expected "Async*a"
[task 2016-10-03T18:48:10.883055Z] Stack:
[task 2016-10-03T18:48:10.883085Z]   checkVisibleStack@eval:18:5
[task 2016-10-03T18:48:10.883109Z]   Async*e@eval:3:3
[task 2016-10-03T18:48:10.883132Z]   a@eval:2:3
[task 2016-10-03T18:48:10.883154Z] Exit code: 3
[task 2016-10-03T18:48:10.883183Z] FAIL - saved-stacks/async-principals.js
[task 2016-10-03T18:48:10.883234Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/saved-stacks/async-principals.js | eval:18:5 Error: Assertion failed: got "a", expected "Async*a" (code 3, args "")

Comment 9

[(no longer active)]

Besides a dumb error about gcc now knowing how to parse C++, this error was caused by assigning to skippedAsync too soon.  Previously we'd assign to it at the end of the loop when calling GetFirstSubsumedFrame() but moving that to before the format function calls made the value correspond to the next stack frame in FormatSpiderMonkeyStackFrame(), which was simple to fix.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1dd8b9b9915
Add support for emulating V8 stack frame formatting to SpiderMonkey; r=fitzgen

Comment 11

[Phil Ringnalda (:philor)]

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