Eddy Bruel [:ejpbruel] Reporter
	Description • 8 years ago

This bug is part of the effort to implement a C++ interface for the debugger API. See the parent bug 1271641 for more context.

I'm filing this as a separate bug because at the time of this writing, some of the promise debugger tests still fail with SPIDERMONKEY_PROMISE enabled. This makes the required changes hard to test. Since SPIDERMONKEY_PROMISE is still disabled by default, I don't want to block landing bug 1271653 on this.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 1 • 8 years ago

Note that Till already did some of the work required for this bug in bug 911216.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 2 • 8 years ago

Till, I've been holding off on this bug because I didn't have a way to run the debugger tests with SpiderMonkey promises enabled that didn't lead to test failures (even without any of my patches applied), which made any changes I intended to do very hard to test.

What is the status on SpiderMonkey promises by now? Can I run the debugger tests with SpiderMonkey promises enabled without getting test failures? (perhaps while having a patch from you applied; we couldn't quite make that work last time I tried).

	Till Schneidereit [:till]
	Comment 3 • 8 years ago

I triggered the switch on SM Promise yesterday, and it seems like it has stuck, so this should be all ready to go now.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 4 • 8 years ago

Started looking into this again.

	Jim Blandy :jimb
	Comment 6 • 8 years ago

Comment on attachment 8781158 [details] [diff] [review]
Proposed API change.

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

Needs docs, but seems okay otherwise.

The preexisting tests are pretty meager; thanks for adding to them. It would be nice if the tests also checked the behavior of the accessors on non-promise objects, and on Debugger.Object.prototype.

::: js/src/jit-test/tests/debug/inspect-wrapped-promise.js
@@ +32,2 @@
>  // Depending on whether async stacks are activated, this can be null, which
>  // has typeof null.

Pre-existing, but I think this should be 'has typeof "object"'.

@@ +38,2 @@
>  // Depending on whether async stacks are activated, this can be null, which
>  // has typeof null.

Similarly.

@@ +58,3 @@
>  
> +assertEq(typeof promiseDO2.promiseTimeToResolution, "number");
> +assertEq(typeof promiseDO3.promiseTimeToResolution, "number");

Can't we assert that promiseDO1.promiseTimeToResolution throws here?

	Eddy Bruel [:ejpbruel] Reporter
	Comment 7 • 8 years ago

During an earlier conversation, we agreed to adhere to the rule that fallible getters should be prefixed with 'get', whereas infallible getters should not. The scriptedProxyTarget/Handler getters are fallible, but do not adhere to this rule. This patch aims to rectify that.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 8 • 8 years ago

Similar to the previous patch, but now for the isPromise getter, which is also fallible.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 9 • 8 years ago

This patch implements the API we discussed earlier. I've also updated the documentation, and addressed your request to extend the tests.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 10 • 8 years ago

(In reply to Eddy Bruel [:ejpbruel] from comment #7)
> Created attachment 8782445 [details] [diff] [review]
> Rename scriptedProxyTarget/Handler to getScriptedProxyTarget/Handler.
> 
> During an earlier conversation, we agreed to adhere to the rule that
> fallible getters should be prefixed with 'get', whereas infallible getters
> should not. The scriptedProxyTarget/Handler getters are fallible, but do not
> adhere to this rule. This patch aims to rectify that.

A while ago, I told myself I should try to be more precise with my wording. Here's a case where I should have done just that:

When I talk about a 'fallible getter', what I actually mean is a getter that needs to be static. A getter needs to be static if it can cause the this-pointer to be garbage collected. In that case, cannot use the implicit this pointer, so instead we pass the object to be operated on as an explicit Handle argument.

Fallible getters need to be static, because apparently reporting an error can cause the GC to run. But some non-fallible getters may need to be static too (recall DebuggerFrame::getReferent, which we had to make non-static because it caused the static rooting analysis to complain).

So the exact rule is: for getters that need to be static, use the 'get' prefix. For all other (i.e. trivial) getters, don't use a prefix.

I hope this clarifies things.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 11 • 8 years ago

Jim, does the DebuggerObject::getIsPromise getter actually need to be static? The reason it is static right now is because it is fallible: if the referent is a CCW, it tries to unwrap it. If unwrapping the referent fails, the getter reports an error and fails.

Do we want to be able to distinguish between an object that is not a promise, and a CCW that we can't tell is not a promise, because we are not allowed to unwrap it? Or could we just return false in both cases. If so, we could rewrite this getter to be infallible, and thus make it non-static.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 12 • 8 years ago

Minor patch update: forgot to add the SPIDERMONKEY_PROMISE feature macro around the definition of DebuggerObject::getIsPromise.

	Jim Blandy :jimb
	Comment 14 • 8 years ago

Comment on attachment 8782445 [details] [diff] [review]
Rename scriptedProxyTarget/Handler to getScriptedProxyTarget/Handler.

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

Does this patch belong on this bug? Those don't have anything to do with promises...

(I'll review this tomorrow!)

	Jim Blandy :jimb
	Comment 15 • 8 years ago

(In reply to Eddy Bruel [:ejpbruel] from comment #11)
> Jim, does the DebuggerObject::getIsPromise getter actually need to be
> static? The reason it is static right now is because it is fallible: if the
> referent is a CCW, it tries to unwrap it. If unwrapping the referent fails,
> the getter reports an error and fails.
> 
> Do we want to be able to distinguish between an object that is not a
> promise, and a CCW that we can't tell is not a promise, because we are not
> allowed to unwrap it? Or could we just return false in both cases. If so, we
> could rewrite this getter to be infallible, and thus make it non-static.

I think it's more helpful to the developer if an attempt to unwrap a CCW we can't see through throws an error. So I guess that means, unfortunately, that the interface needs to remain fallible.

	Jim Blandy :jimb
	Comment 16 • 8 years ago

Comment on attachment 8782447 [details] [diff] [review]
Split promiseState into promiseState/Value/Reason.

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

Looks great. Just one more test I wanted...

::: js/src/jit-test/tests/debug/inspect-wrapped-promise.js
@@ +9,5 @@
>  
> +g.promise1 = new Promise(() => {});
> +g.promise2 = Promise.resolve(42);
> +g.promise3 = Promise.reject(42);
> +g.promise4 = new Object();

There's just one more case I'd like to throw in here:

g.promise5 = Promise.prototype;

It's not of the same class, so Debugger should immediately recognize that it's not a promise, but I'd just like to be sure.

	Jim Blandy :jimb
	Comment 17 • 8 years ago

Comment on attachment 8782476 [details] [diff] [review]
Implement a C++ interface for DebuggerObject.promiseState.

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

I have to go, will finish this tonight. But now that I see the big picture, I think I may have made a mistake earlier in our discussion about isPromise:

::: js/src/vm/Debugger.cpp
@@ +9342,5 @@
> +                                JS::PromiseState& result)
> +{
> +    Rooted<PromiseObject*> promise(cx);
> +    if (!DebuggerObject::getPromise(cx, object, &promise))
> +        return false;

Oh... now I understand why you were hoping to make the isPromise predicate infallible.

What we really want is a function that means, "It is safe to call the promise accessors on this object," so we can follow the convention we established elsewhere that the C++ accessors should MOZ_ASSERT that the D.O's referent is of the correct type.

	Jim Blandy :jimb
	Comment 18 • 8 years ago

Comment on attachment 8782476 [details] [diff] [review]
Implement a C++ interface for DebuggerObject.promiseState.

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

Clearing review for now; please re-flag me once they've been revised per the previous comment.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 19 • 8 years ago

As discussed during our standup yesterday.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 20 • 8 years ago

I don't assert in DebuggerObject::promiseState() because DebuggerObject::promise() already does the assertion.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 21 • 8 years ago

These accessors still need to be fallible; we need to wrap the value before returning it, and wrapping a value is a fallible operation.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 23 • 8 years ago

Try push for the following patches:
- Rename scriptedProxyTarget/Handler to getScriptedProxyTarget/Handler.
- Rename isPromise to getIsPromise.

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

	Jim Blandy :jimb
	Comment 24 • 8 years ago

Oh, man, I wrote a whole serious evaluation of the different angles of the JS and C++ API, and as far as I can tell Bugzilla just swallowed the whole thing. I'll look this over, maybe you guessed everything I was going to say!

	Jim Blandy :jimb
	Comment 25 • 8 years ago

Comment on attachment 8784775 [details] [diff] [review]
Implement a C++ interface for DebuggerObject.promiseState.

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

::: js/src/vm/Debugger-inl.h
@@ +108,5 @@
> +
> +    JSObject* referent = this->referent();
> +    if (IsCrossCompartmentWrapper(referent)) {
> +        referent = CheckedUnwrap(referent);
> +        MOZ_ASSERT(referent);

It might be worth commenting that this assertion is expected to pass because isPromise performs the same check.

::: js/src/vm/Debugger.cpp
@@ +10009,5 @@
> +DebuggerObject::requirePromise(JSContext* cx, HandleDebuggerObject object)
> +{
> +   if (!object->isPromise()) {
> +      JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_NOT_EXPECTED_TYPE,
> +                           "Debugger", "Promise", object->getClass()->name);

If I'm reading correctly, this produces slightly less useful diagnostic information than the THIS_DEBUGOBJECT_PROMISE does. The latter distinguishes between being unable to traverse a wrapper and the final referent not being a promise, which seems like something that could save people a lot of trouble.

Could we make that distinction here? The isPromise function doesn't provide any way to make the distinction, so it's fine to just write out both checks here.

	Pulsebot
	Comment 26 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd260dd847e7
Rename scriptedProxyTarget/Handler to getScriptedProxyTarget/Handler. r=jimb

	Pulsebot
	Comment 27 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/921661a93532
Rename isPromise to getIsPromise. r=jimb

	Eddy Bruel [:ejpbruel] Reporter
	Comment 28 • 8 years ago

Try push for the following patches:
- Split promiseState into promiseState/Value/Reason. 
- Make the isPromise getter infallible.
- Implement a C++ interface for DebuggerObject.promiseState.
- Implement a C++ interface for DebuggerObject.promiseValue.
- Implement a C++ interface for DebuggerObject.promiseReason.

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

	Pulsebot
	Comment 29 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/477689b30c9e
Split promiseState into promiseState/Value/Reason. r=jimb

	Pulsebot
	Comment 30 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da5623128a3a
Make the isPromise getter infallible. r=jimb

	Pulsebot
	Comment 31 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a66d787e352
Implement a C++ interface for DebuggerObject.promiseState. r=jimb

	Pulsebot
	Comment 32 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2a5dc69ab0
Implement a C++ interface for DebuggerObject.promiseValue. r=jimb

	Pulsebot
	Comment 33 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3bb9308449
Implement a C++ interface for DebuggerObject.promiseReason. r=jimb

	Eddy Bruel [:ejpbruel] Reporter
	Comment 36 • 8 years ago

I noticed that instead of returning `undefined`, DebuggerObject.promiseResolutionSite throws a TypeError if the promise does not have the correct state.

In my opinion, we should do the same thing for DebuggerObject.promiseValue/Reason, which currently return `undefined` if the promise does not have the correct state. Since `undefined` can also be a legal value if the promise *does* have the correct state, throwing an error here should be slightly less error prone.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 38 • 8 years ago

That's it for now. Going to give you a chance to catch up with reviews first.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 39 • 8 years ago

Backed these out for devtools failures (e.g. browser_dbg_promises-allocation-stack.js) and xpcshell failures (test_promise_state-01.js):

https://hg.mozilla.org/integration/mozilla-inbound/rev/16d864c58b3ebb0bfc5804515c54feda8800191b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0e64e292b7ae1993e361cdc1ffd4ddace852960
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6655b488dfda039c129e8d20ecc561eb5262ae7
https://hg.mozilla.org/integration/mozilla-inbound/rev/8734d1dcca7b35b65b442bab2860d77347253d93
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb0e6ad4dd2179e6a0c6eaf450d1c82814cc6559

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=477689b30c9e934bd11db95bbc28e7978a0dcaed
Please check the test failures, there are several devtools mochitests failing, also at least one xpcshell test: https://treeherder.mozilla.org/logviewer.html#?job_id=34768812&repo=mozilla-inbound

TIMEOUT | devtools/server/tests/unit/test_promise_state-01.js

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 40 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bd260dd847e7
https://hg.mozilla.org/mozilla-central/rev/921661a93532

	Eddy Bruel [:ejpbruel] Reporter
	Comment 41 • 8 years ago

New try push for the following patches
- Split promiseState into promiseState/Value/Reason. 
- Make the isPromise getter infallible.
- Implement a C++ interface for DebuggerObject.promiseState.
- Implement a C++ interface for DebuggerObject.promiseValue.
- Implement a C++ interface for DebuggerObject.promiseReason.

with previous issues addressed, and mochitest-dt and xpcshell tests included:

	Eddy Bruel [:ejpbruel] Reporter
	Comment 42 • 8 years ago

Fixed some bugs in this patch.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 43 • 8 years ago

And this one too.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 45 • 8 years ago

Implementing a C++ interface for DebuggerObject.promiseDependentProperties turns out to be somewhat tricky.

DebuggerObject.promiseDependentProperties returns an array of reaction objects, each of which has the following properties:
- promise
  The promise this reaction resolves.
- resolve
  The `resolve` callback content code provided.
- reject
  The `reject` callback content code provided.
- fulfillHandler
  The internal handler that fulfills this promise.
- rejectHandler
  The internal handler that rejects this promise.
- incumbentGlobal
  An object from the global that was incumbent when the reaction was created.

I can think of two ways to turn this into a C++ interface. The most obvious approach is to convert each of these objects into a struct with the same properties, and have the C++ function append instances of these structs to a GCVector that is provided as an output parameter.

I somewhat dislike this approach, because it burdens embedders with making sure that these structs are traced properly; this is something we've managed to avoid with all other methods/accessors so far. Another approach would be to 'flatten the array'. That is, implement a separate C++ function for each of these properties, each of which fills a GCVector with instance of the appropriate type.

To be honest, I don't really like this approach either. It seemed reasonable to do this for promiseState, which only returned an object with three properties. In contrast promiseDependentProperties returns an array of objects, each of which has six properties. Splitting this up feels like it would make the interface needlessly complex. In addition, it doesn't match well with how Promises are implemented (promise->dependentPromises is designed to return an array of result objects).

Jim, can you think of any better ideas? What approach would you take?

	Eddy Bruel [:ejpbruel] Reporter
	Comment 46 • 8 years ago

Forgot to include a link to the try push in comment 41:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b116aee3a041

	Pulsebot
	Comment 47 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/115f3bbe634f
Split promiseState into promiseState/Value/Reason. r=jimb

	Pulsebot
	Comment 48 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4c0101e717
Make the isPromise getter infallible. r=jimb

	Pulsebot
	Comment 49 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/035cd4aa6771
Implement a C++ interface for DebuggerObject.promiseState. r=jimb

	Pulsebot
	Comment 50 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b231da97c3b6
Implement a C++ interface for DebuggerObject.promiseValue. r=jimb

	Pulsebot
	Comment 51 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b3de279ec8
Implement a C++ interface for DebuggerObject.promiseReason. r=jimb

	Jim Blandy :jimb
	Comment 52 • 8 years ago

(In reply to Eddy Bruel [:ejpbruel] from comment #45)
> I somewhat dislike this approach, because it burdens embedders with making
> sure that these structs are traced properly; this is something we've managed
> to avoid with all other methods/accessors so far.

Aside from the ergonomic question, returning a struct seems like a pretty nice API. And I think there's a way to make the ergonomics actually not too bad.

SpiderMonkey's Rooted and Handle templates have been generalized to support any type that has a `trace` method. So if your "PromiseDependentProperties" type (or whatever is best to call it) uses Heap<JSObject*> and Heap<Value> to point to the values, and has a `trace` method, then the embedder can simply use Rooted<PromiseDependentProperties>, and that will work.

Alternatively, you could make the struct use PersistentRooted to point at the values. However, that would mean the embedding could never have a GC'd type that owns a PromiseDependentProperties; PersistentRooted is a genuine *root*, and if a root is ever owned by a GC'd type, that's a leak.

It seems better to make PromiseDependentProperties implement `trace`, and then let the embedder decide whether to use it with Rooted, PersistentRooted, or even Heap!

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 53 • 8 years ago

Backed out for asserting in bug1091757.js of spidermonkey root analysis:

https://hg.mozilla.org/integration/mozilla-inbound/rev/692b9d7255001eec493b88cc0795f67527c9baec
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3a459c7824dcc580da1ce68e4055b05ae775f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce63d8624048426d1974c97da0a027f9125f3ddd
https://hg.mozilla.org/integration/mozilla-inbound/rev/90f93d95f75ae6349a33345952160ac1dedb5645
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7bed3ba115ac6376fdab9d4b48bfde6f4e5f68

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

TEST-PASS | js/src/jit-test/tests/basic/bug1035287-track-allocation-sites-recursion.js | Success (code 3, args "")
TEST-PASS | js/src/jit-test/tests/basic/bug1100623.js | Success (code 3, args "")
TEST-PASS | js/src/jit-test/tests/basic/bug1091757.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off")
TEST-PASS | js/src/jit-test/tests/basic/bug1091757.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")
/home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:9:5 Error: Assertion failed: got false, expected true
Stack:
  @/home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:9:5
Exit code: 3
FAIL - basic/bug1091757.js
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/basic/bug1091757.js | /home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:9:5 Error: Assertion failed: got false, expected true (code 3, args "--baseline-eager")
INFO exit-status     : 3
INFO timed-out       : False
INFO stderr         2> /home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:9:5 Error: Assertion failed: got false, expected true
INFO stderr         2> Stack:
INFO stderr         2> @/home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:9:5

	Jim Blandy :jimb
	Comment 54 • 8 years ago

Comment on attachment 8785267 [details] [diff] [review]
Implement a C++ interface for DebuggerObject.promiseResolutionSite.

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

The title of this patch is incorrect, I think.

	Jim Blandy :jimb
	Comment 55 • 8 years ago

(In reply to Eddy Bruel [:ejpbruel] from comment #36)
> I noticed that instead of returning `undefined`,
> DebuggerObject.promiseResolutionSite throws a TypeError if the promise does
> not have the correct state.
> 
> In my opinion, we should do the same thing for
> DebuggerObject.promiseValue/Reason, which currently return `undefined` if
> the promise does not have the correct state. Since `undefined` can also be a
> legal value if the promise *does* have the correct state, throwing an error
> here should be slightly less error prone.

I guess this is a pre-existing issue, so being consistent with the other accessors is probably best, and it should throw.

But in principle, accessors that throw is not The JavaScript Way, and Debugger generally tries to follow The JavaScript Way.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 56 • 8 years ago

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] from comment #53)
> Backed out for asserting in bug1091757.js of spidermonkey root analysis:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 692b9d7255001eec493b88cc0795f67527c9baec
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 1c3a459c7824dcc580da1ce68e4055b05ae775f8
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> ce63d8624048426d1974c97da0a027f9125f3ddd
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 90f93d95f75ae6349a33345952160ac1dedb5645
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> eb7bed3ba115ac6376fdab9d4b48bfde6f4e5f68
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=115f3bbe634f0c2764b07485fdb1808e92b83444
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=34865342&repo=mozilla-
> inbound
> 
> TEST-PASS |
> js/src/jit-test/tests/basic/bug1035287-track-allocation-sites-recursion.js |
> Success (code 3, args "")
> TEST-PASS | js/src/jit-test/tests/basic/bug1100623.js | Success (code 3,
> args "")
> TEST-PASS | js/src/jit-test/tests/basic/bug1091757.js | Success (code 0,
> args "--ion-eager --ion-offthread-compile=off")
> TEST-PASS | js/src/jit-test/tests/basic/bug1091757.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")
> /home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:9:
> 5 Error: Assertion failed: got false, expected true
> Stack:
>  
> @/home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:
> 9:5
> Exit code: 3
> FAIL - basic/bug1091757.js
> TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/basic/bug1091757.js |
> /home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:9:
> 5 Error: Assertion failed: got false, expected true (code 3, args
> "--baseline-eager")
> INFO exit-status     : 3
> INFO timed-out       : False
> INFO stderr         2>
> /home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:9:
> 5 Error: Assertion failed: got false, expected true
> INFO stderr         2> Stack:
> INFO stderr         2>
> @/home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:
> 9:5

This error makes zero sense. The assertion failure isn't an actual rooting hazard, and in fact the same patch passed without any rooting hazards on try just before I landed it.

What is actually failing is an assertion in this test:
https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/basic/bug1100623.js?q=path%3Abug1100623.js&redirect_type=single#10

This test has absolutely nothing to do with this patch, which makes some minor changes to the debugger API surface. This test doesn't even touch the debugger API, so it shouldn't be affected at all.

My understanding is that Shu landed a major patch just before mine, that *could* explain the failures we're seeing here. However, it does not explain why my patch caused these failures to be triggered. Shu, do you have any idea what might be going on here?

	Eddy Bruel [:ejpbruel] Reporter
	Comment 57 • 8 years ago

I tried to run the rooting hazard analyser for SpiderMonkey on my machine, in an attempt to reproduce the assertion failure locally (this took some time, since I had to re-install my Linux VM; I switched back to OSX after my Linux VM crashed catastrophically and I lost 2 days of work in the progress).

I used the process described here:
https://dxr.mozilla.org/mozilla-central/source/js/src/devtools/rootAnalysis/README.md

But running this command:
GECKO_DIR=$SRCDIR $SRCDIR/taskcluster/scripts/builder/build-haz-linux.sh $(pwd) --dep

Give me the following error:

Error: Cannot find module 'promise'
    at Function.Module._resolveFilename (module.js:326:15)
    at Function.Module._load (module.js:277:25)
    at Module.require (module.js:354:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/home/ejpbruel/Projects/node_modules/taskcluster-vcs/node_modules/superagent-promise/index.js:6:31)

This looks like its either:
- The taskcluster script having a dependency the README.md didn't tell me about
- A versioning problem with one of the tools used by the taskcluster script (probably node).
- A bug in the taskcluster script.

I'm still trying to figure out what's going on, but so far, I haven't been able to make it work

	Eddy Bruel [:ejpbruel] Reporter
	Comment 58 • 8 years ago

(In reply to Eddy Bruel [:ejpbruel] from comment #57)
> I tried to run the rooting hazard analyser for SpiderMonkey on my machine,
> in an attempt to reproduce the assertion failure locally (this took some
> time, since I had to re-install my Linux VM; I switched back to OSX after my
> Linux VM crashed catastrophically and I lost 2 days of work in the progress).
> 
> I used the process described here:
> https://dxr.mozilla.org/mozilla-central/source/js/src/devtools/rootAnalysis/
> README.md
> 
> But running this command:
> GECKO_DIR=$SRCDIR $SRCDIR/taskcluster/scripts/builder/build-haz-linux.sh
> $(pwd) --dep
> 
> Give me the following error:
> 
> Error: Cannot find module 'promise'
>     at Function.Module._resolveFilename (module.js:326:15)
>     at Function.Module._load (module.js:277:25)
>     at Module.require (module.js:354:17)
>     at require (internal/module.js:12:17)
>     at Object.<anonymous>
> (/home/ejpbruel/Projects/node_modules/taskcluster-vcs/node_modules/
> superagent-promise/index.js:6:31)
> 
> This looks like its either:
> - The taskcluster script having a dependency the README.md didn't tell me
> about
> - A versioning problem with one of the tools used by the taskcluster script
> (probably node).
> - A bug in the taskcluster script.
> 
> I'm still trying to figure out what's going on, but so far, I haven't been
> able to make it work

Made some more progress. There was indeed an implicit dependency that I had to install by hand: 'npm install promise' did the trick for me. This made the build script run, but now it fails in the configuration stage:

DEBUG: /home/ejpbruel/Projects/gecko-dev/work/gcc/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.3/../../../../x86_64-unknown-linux-gnu/bin/ld: /usr/lib/x86_64-linux-gnu/crti.o: unrecognized relocation (0x2a) in section `.init'
DEBUG: /home/ejpbruel/Projects/gecko-dev/work/gcc/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.3/../../../../x86_64-unknown-linux-gnu/bin/ld: final link failed: Bad value

(I made sure my CFLAGS and CXXFLAGS are set as mentioned in the README.md file)

A quick search on Google suggest that this might be a libc bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=808205.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 59 • 8 years ago

(In reply to Eddy Bruel [:ejpbruel] from comment #56)
> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] from comment #53)
> > Backed out for asserting in bug1091757.js of spidermonkey root analysis:
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > 692b9d7255001eec493b88cc0795f67527c9baec
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > 1c3a459c7824dcc580da1ce68e4055b05ae775f8
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > ce63d8624048426d1974c97da0a027f9125f3ddd
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > 90f93d95f75ae6349a33345952160ac1dedb5645
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > eb7bed3ba115ac6376fdab9d4b48bfde6f4e5f68
> > 
> > Push with failures:
> > https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> > inbound&revision=115f3bbe634f0c2764b07485fdb1808e92b83444
> > Failure log:
> > https://treeherder.mozilla.org/logviewer.html#?job_id=34865342&repo=mozilla-
> > inbound
> > 
> > TEST-PASS |
> > js/src/jit-test/tests/basic/bug1035287-track-allocation-sites-recursion.js |
> > Success (code 3, args "")
> > TEST-PASS | js/src/jit-test/tests/basic/bug1100623.js | Success (code 3,
> > args "")
> > TEST-PASS | js/src/jit-test/tests/basic/bug1091757.js | Success (code 0,
> > args "--ion-eager --ion-offthread-compile=off")
> > TEST-PASS | js/src/jit-test/tests/basic/bug1091757.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")
> > /home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:9:
> > 5 Error: Assertion failed: got false, expected true
> > Stack:
> >  
> > @/home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:
> > 9:5
> > Exit code: 3
> > FAIL - basic/bug1091757.js
> > TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/basic/bug1091757.js |
> > /home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:9:
> > 5 Error: Assertion failed: got false, expected true (code 3, args
> > "--baseline-eager")
> > INFO exit-status     : 3
> > INFO timed-out       : False
> > INFO stderr         2>
> > /home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:9:
> > 5 Error: Assertion failed: got false, expected true
> > INFO stderr         2> Stack:
> > INFO stderr         2>
> > @/home/worker/workspace/build/src/js/src/jit-test/tests/basic/bug1091757.js:
> > 9:5
> 
> This error makes zero sense. The assertion failure isn't an actual rooting
> hazard, and in fact the same patch passed without any rooting hazards on try
> just before I landed it.
> 
> What is actually failing is an assertion in this test:
> https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/basic/
> bug1100623.js?q=path%3Abug1100623.js&redirect_type=single#10
> 
> This test has absolutely nothing to do with this patch, which makes some
> minor changes to the debugger API surface. This test doesn't even touch the
> debugger API, so it shouldn't be affected at all.
> 
> My understanding is that Shu landed a major patch just before mine, that
> *could* explain the failures we're seeing here. However, it does not explain
> why my patch caused these failures to be triggered. Shu, do you have any
> idea what might be going on here?

I included the wrong link to the test in this comment, sorry:
https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/basic/bug1091757.js

	Eddy Bruel [:ejpbruel] Reporter
	Comment 60 • 8 years ago

The next patch will need some additional GC typedefs. I've factored these changes out into a separate patch to make them easier for you to review.

	Eddy Bruel [:ejpbruel] Reporter
	Comment 61 • 8 years ago

I was wrong earlier. DebuggerObject.promiseDependentPromises does *not* return an array of reaction objects. Instead, it walks over the array of reaction objects stored in the promise, and returns the promise property of each one. The result is an array of JSObjects.

That makes this patch much simpler than I thought it would be: instead of a GCVector to a struct, we can just return a GCVector of DebuggerObjects wrapping each of the promises I mentioned before.

	Pulsebot
	Comment 62 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3276dc9d5b1
Split promiseState into promiseState/Value/Reason. r=jimb

	Jim Blandy :jimb
	Comment 63 • 8 years ago

(In reply to Eddy Bruel [:ejpbruel] from comment #36)
> I noticed that instead of returning `undefined`,
> DebuggerObject.promiseResolutionSite throws a TypeError if the promise does
> not have the correct state.
> 
> In my opinion, we should do the same thing for
> DebuggerObject.promiseValue/Reason, which currently return `undefined` if
> the promise does not have the correct state. Since `undefined` can also be a
> legal value if the promise *does* have the correct state, throwing an error
> here should be slightly less error prone.

I would generally agree that always throwing for any use of an accessor that doesn't make sense for the referent type, or the referent's current state, is the better plan. But that's not The JavaScript Way, as I understand it. This is why every other accessor in the API returns `undefined` when it's not appropriate.

So, the promise part of Debugger.Object's API behaves the way I wish APIs would behave in general, but not in a way consistent with other JavaScript, nor with the rest of Debugger.Object.

For the time being, the right thing is probably for the new promise methods and accessors to be consistent with the old promise stuff. But really, the promise stuff shouldn't have been landed that way in the first place.

	Jim Blandy :jimb
	Comment 64 • 8 years ago

dup of comment 55, sorry

	Jim Blandy :jimb
	Comment 65 • 8 years ago

Comment on attachment 8785834 [details] [diff] [review]
Implement a C++ interface for DebuggerObject.promiseAllocationSite.

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

::: js/src/vm/Debugger.cpp
@@ +9567,5 @@
>  }
> +
> +/* static */ bool
> +DebuggerObject::getPromiseAllocationSite(JSContext* cx, HandleDebuggerObject object,
> +                                         MutableHandleObject result)

Could we be using MutableHandleDebuggerObject in these method signatures?

	Jim Blandy :jimb
	Comment 66 • 8 years ago

Comment on attachment 8785834 [details] [diff] [review]
Implement a C++ interface for DebuggerObject.promiseAllocationSite.

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

::: js/src/vm/Debugger.cpp
@@ +9567,5 @@
>  }
> +
> +/* static */ bool
> +DebuggerObject::getPromiseAllocationSite(JSContext* cx, HandleDebuggerObject object,
> +                                         MutableHandleObject result)

Oh, duh, we're returning the SavedFrame objects directly (which is the right thing!), so this would be a MutableHandleSavedFrame.

	Jim Blandy :jimb
	Comment 67 • 8 years ago

Comment on attachment 8785835 [details] [diff] [review]
Implement a C++ interface for DebuggerObject.promiseResolutionSite.

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

::: js/src/vm/Debugger.h
@@ +1264,5 @@
>                                                MutableHandleValue result);
>      static MOZ_MUST_USE bool getPromiseAllocationSite(JSContext* cx, HandleDebuggerObject object,
>                                                        MutableHandleObject result);
> +    static MOZ_MUST_USE bool getPromiseResolutionSite(JSContext* cx, HandleDebuggerObject object,
> +                                                      MutableHandleObject result);

As before, consider MutableHandleSavedFrame here.

	Jim Blandy :jimb
	Comment 68 • 8 years ago

Comment on attachment 8785836 [details] [diff] [review]
Implement a C++ interface for DebuggerObject.promiseID.

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

::: js/src/vm/Debugger.cpp
@@ +8735,5 @@
>  
> +    if (!DebuggerObject::requirePromise(cx, object))
> +        return false;
> +
> +    args.rval().setNumber(double(object->promiseID()));

Pre-existing issue, but let's change this to static_cast<double>(object->promiseID()).

@@ +9408,5 @@
>      return promise()->timeToResolution();
>  }
>  
> +double
> +DebuggerObject::promiseID() const

The right return type for this is probably uint64_t, since that's what PromiseObject::getID returns.

	Jim Blandy :jimb
	Comment 69 • 8 years ago

Comment on attachment 8786300 [details] [diff] [review]
Add some GC typedefs.

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

Sorry, I didn't notice the naming conflict the first time.

::: js/src/gc/Rooting.h
@@ +74,5 @@
>  
> +typedef JS::GCVector<DebuggerObject*>   DebuggerObjectVector;
> +typedef JS::GCVector<DebuggerFrame*>    DebuggerFrameVector;
> +typedef JS::GCVector<JSFunction*>       FunctionVector;
> +typedef JS::GCVector<JSObject*>         ObjectVector;

It worries me a bit that js/src/jit/IonCode.h and js/src/jit/Mir.h already have a definition for ObjectVector that is different. Adding this typedef makes it too easy for them to end up with a vector with the wrong alloc policy and never know.

It looks like ObjectVector is pretty much the whole reason you wrote the patch, but I think we should actually leave it out until we can get the JITs using a different name.

At the very least, we should get a JIT person's approval on this patch.

@@ +78,5 @@
> +typedef JS::GCVector<JSObject*>         ObjectVector;
> +typedef JS::GCVector<PropertyName*>     PropertyNameVector;
> +typedef JS::GCVector<Shape*>            ShapeVector;
> +typedef JS::GCVector<JSString*>         StringVector;
> +typedef JS::GCVector<Value>             ValueVector;

This is already declared in NamespaceImports.h and jsapi.h. Could we clean up those definitions?

	Jim Blandy :jimb
	Comment 70 • 8 years ago

"This" in the last line of the prior comment refers specifically to ValueVector, not to the whole bunch of typedefs.

	Jim Blandy :jimb
	Comment 71 • 8 years ago

Jan, what's your take on having a more generic ObjectVector typedef for the broader code base, and perhaps renaming js::jit::ObjectVector?

	Jim Blandy :jimb
	Comment 72 • 8 years ago

Comment on attachment 8786301 [details] [diff] [review]
Implement a C++ interface for DebuggerObject.promiseDependentPromises.

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

::: js/src/builtin/Promise.cpp
@@ +803,5 @@
>   * containing the required data for both cases. So we just walk that list
>   * and extract the dependent promises from all reaction records.
>   */
>  bool
> +PromiseObject::dependentPromises(JSContext* cx, MutableHandle<ObjectVector> result)

Let's just leave the names alone. This isn't an obvious improvement, and it's noise in the patch.

@@ +846,2 @@
>              return false;
> +        result[i].set(&val.toObject());

Is it for certain that this property always exists and is an object? If so, would it be better for the argument to be MutableHandle<GCVector<PromiseObject*>>?

	Ryan VanderMeulen [:RyanVM]
	Comment 73 • 8 years ago
bugherder

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

	Jan de Mooij [:jandem]
	Comment 74 • 8 years ago

(In reply to Jim Blandy :jimb from comment #71)
> Jan, what's your take on having a more generic ObjectVector typedef for the
> broader code base, and perhaps renaming js::jit::ObjectVector?

Maybe rename to js::jit::MObjectVector (same M* prefix we use for MIR instructions)? Also, the typedef in IonCode.h can probably be removed.

	Pulsebot
	Comment 75 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ab78fba118
Make the isPromise getter infallible. r=jimb

	Pulsebot
	Comment 76 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f05cea45abc
Implement a C++ interface for DebuggerObject.promiseState. r=jimb

	Pulsebot
	Comment 77 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22e24aa12077
Implement a C++ interface for DebuggerObject.promiseValue. r=jimb

	Pulsebot
	Comment 78 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d687cd6446a
Implement a C++ interface for DebuggerObject.promiseReason. r=jimb

	Ryan VanderMeulen [:RyanVM]
	Comment 79 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d4ab78fba118
https://hg.mozilla.org/mozilla-central/rev/6f05cea45abc
https://hg.mozilla.org/mozilla-central/rev/22e24aa12077
https://hg.mozilla.org/mozilla-central/rev/6d687cd6446a

	Eddy Bruel [:ejpbruel] Reporter
	Comment 80 • 8 years ago

Try push for the following patches:
- Implement a C++ interface for DebuggerObject.promiseLifetime
- Implement a C++ interface for DebuggerObject.promiseTimeToResolution.
- DebuggerObject.promiseValue/Reason should throw if promise is not fulfilled/rejected.

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

	Pulsebot
	Comment 81 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c01cff2648a
Implement a C++ interface for DebuggerObject.promiseLifetime. r=jimb

	Pulsebot
	Comment 82 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de45e910de91
Implement a C++ interface for DebuggerObject.promiseTimeToResolution. r=jimb

	Pulsebot
	Comment 83 • 8 years ago

Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4928fc74e5
DebuggerObject.promiseValue/Reason should throw if promise is not fulfilled/rejected. r=jimb

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 84 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9c01cff2648a
https://hg.mozilla.org/mozilla-central/rev/de45e910de91
https://hg.mozilla.org/mozilla-central/rev/fc4928fc74e5

	Sylvestre Ledru [:Sylvestre]
	Comment 85 • 6 years ago

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information