Closed
Bug 1278562
Opened 8 years ago
Closed 5 years ago
Implement a C++ interface for the promise properties of Debugger.Object instances.
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
WONTFIX
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: ejpbruel, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(16 files, 4 obsolete files)
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.
Updated•8 years ago
|
Severity: normal → enhancement
Updated•8 years ago
|
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Reporter | ||
Comment 1•8 years ago
|
||
Note that Till already did some of the work required for this bug in bug 911216.
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).
Flags: needinfo?(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.
Flags: needinfo?(till)
Reporter | ||
Comment 4•8 years ago
|
||
Started looking into this again.
Reporter | ||
Comment 5•8 years ago
|
||
Attachment #8781158 -
Flags: feedback?(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?
Attachment #8781158 -
Flags: feedback?(jimb) → feedback+
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.
Attachment #8782445 -
Flags: review?(jimb)
Reporter | ||
Comment 8•8 years ago
|
||
Similar to the previous patch, but now for the isPromise getter, which is also fallible.
Attachment #8782446 -
Flags: review?(jimb)
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.
Attachment #8782447 -
Flags: review?(jimb)
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.
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.
Flags: needinfo?(jimb)
Reporter | ||
Comment 12•8 years ago
|
||
Minor patch update: forgot to add the SPIDERMONKEY_PROMISE feature macro around the definition of DebuggerObject::getIsPromise.
Attachment #8782446 -
Attachment is obsolete: true
Attachment #8782446 -
Flags: review?(jimb)
Attachment #8782470 -
Flags: review?(jimb)
Reporter | ||
Comment 13•8 years ago
|
||
Attachment #8782476 -
Flags: review?(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!)
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.
Flags: needinfo?(jimb)
Updated•8 years ago
|
Attachment #8782445 -
Flags: review?(jimb) → review+
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.
Attachment #8782447 -
Flags: review?(jimb) → review+
Updated•8 years ago
|
Attachment #8782470 -
Flags: review?(jimb) → review+
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.
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.
Attachment #8782476 -
Flags: review?(jimb)
Reporter | ||
Comment 19•8 years ago
|
||
As discussed during our standup yesterday.
Attachment #8782476 -
Attachment is obsolete: true
Attachment #8784774 -
Flags: review?(jimb)
Reporter | ||
Comment 20•8 years ago
|
||
I don't assert in DebuggerObject::promiseState() because DebuggerObject::promise() already does the assertion.
Attachment #8784775 -
Flags: review?(jimb)
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.
Attachment #8784779 -
Flags: review?(jimb)
Reporter | ||
Comment 22•8 years ago
|
||
Attachment #8784781 -
Flags: review?(jimb)
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
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!
Updated•8 years ago
|
Attachment #8784774 -
Flags: review?(jimb) → review+
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.
Attachment #8784775 -
Flags: review?(jimb) → review+
Updated•8 years ago
|
Attachment #8784779 -
Flags: review?(jimb) → review+
Updated•8 years ago
|
Attachment #8784781 -
Flags: review?(jimb) → review+
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
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
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
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
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
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
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
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
Reporter | ||
Comment 34•8 years ago
|
||
Attachment #8785266 -
Flags: review?(jimb)
Reporter | ||
Comment 35•8 years ago
|
||
Attachment #8785267 -
Flags: review?(jimb)
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.
Attachment #8785270 -
Flags: review?(jimb)
Reporter | ||
Comment 37•8 years ago
|
||
Attachment #8785271 -
Flags: review?(jimb)
Reporter | ||
Comment 38•8 years ago
|
||
That's it for now. Going to give you a chance to catch up with reviews first.
Attachment #8785272 -
Flags: review?(jimb)
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
Flags: needinfo?(ejpbruel)
Comment 40•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bd260dd847e7
https://hg.mozilla.org/mozilla-central/rev/921661a93532
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Reporter | ||
Updated•8 years ago
|
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:
Reporter | ||
Comment 42•8 years ago
|
||
Fixed some bugs in this patch.
Attachment #8785271 -
Attachment is obsolete: true
Attachment #8785271 -
Flags: review?(jimb)
Attachment #8785834 -
Flags: review?(jimb)
Reporter | ||
Comment 43•8 years ago
|
||
And this one too.
Attachment #8785272 -
Attachment is obsolete: true
Attachment #8785272 -
Flags: review?(jimb)
Attachment #8785835 -
Flags: review?(jimb)
Reporter | ||
Comment 44•8 years ago
|
||
Attachment #8785836 -
Flags: review?(jimb)
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?
Flags: needinfo?(jimb)
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
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
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
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
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
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
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!
Flags: needinfo?(jimb)
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
Flags: needinfo?(ejpbruel)
Updated•8 years ago
|
Attachment #8785266 -
Flags: review?(jimb) → review+
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.
Attachment #8785267 -
Flags: review?(jimb) → review+
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.
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?
Flags: needinfo?(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
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.
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
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.
Attachment #8786300 -
Flags: review?(jimb)
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.
Attachment #8786301 -
Flags: review?(jimb)
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
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.
Updated•8 years ago
|
Attachment #8785270 -
Flags: review?(jimb) → review+
Comment 64•8 years ago
|
||
dup of comment 55, sorry
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?
Attachment #8785834 -
Flags: review?(jimb) → review+
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.
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.
Attachment #8785835 -
Flags: review?(jimb) → review+
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.
Attachment #8785836 -
Flags: review?(jimb) → review+
Updated•8 years ago
|
Attachment #8786300 -
Flags: review?(jimb) → review+
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?
Attachment #8786300 -
Flags: review+ → review-
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.
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?
Flags: needinfo?(jdemooij)
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*>>?
Attachment #8786301 -
Flags: review?(jimb) → review+
Comment 73•8 years ago
|
||
bugherder |
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.
Flags: needinfo?(jdemooij)
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
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
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
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
Comment 79•8 years ago
|
||
bugherder |
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
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
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
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
Comment 84•8 years ago
|
||
bugherder |
Updated•8 years ago
|
Whiteboard: [devtools-html]
Reporter | ||
Updated•8 years ago
|
Assignee: ejpbruel → nobody
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
Priority: P2 → P3
Comment 86•6 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?
Flags: needinfo?(sdetar)
Comment 87•5 years ago
|
||
Jason, any thoughts on if we can close this bug?
Flags: needinfo?(sdetar) → needinfo?(jorendorff)
Comment 88•5 years ago
|
||
It's work that was never completed. I don't care if it's open or not.
Since there is a bot that pesters people about bugs in this state, I will mark WONTFIX. Easy enough to reopen it (or just file a duplicate) if anyone ever wants to work on this.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 5 years ago
Flags: needinfo?(jorendorff)
Keywords: leave-open
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•