Mark ready and finished promise rejections as handled
Categories
(Core :: DOM: Animation, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox76 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(3 files)
|
Assignee | |
Comment 1•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3503797c5a6811be2c167ea067d2ed6fc7f55055
Arai-san, does this look even close to correct?
https://hg.mozilla.org/try/rev/54bc69142ded40d5a5e5b8fef909bfd3fe1dab3e
It's the first time I've touched the JS API.
|
||
Comment 2•5 years ago
|
||
IMO, public JSAPI that receives JSContext + GC things (JSObject) should do:
- assert GC isn't performed (
AssertHeapIsIdle()call) - assert it's in the correct thread (
CHECK_THREAD(cx)call) - assert the GC thing is in the same compartment as JSContext (
cx->check(...)call) - handle cross-compartment wrapper (unwrap it, and enter the realm)
Example code that does the above:
https://searchfox.org/mozilla-central/rev/f36cb2af46edd2659f446b7acdb2154e230ee445/js/src/jsapi.cpp#3887-3906
Especially the last one is important.
otherwise as<PromiseObject>() cast fails and it results in sec-bug.
I've filed bug 1622329, in case promiseObj() can return non-PromiseObject.
if it's always PromiseObject, the assertion in the current patch should be sufficient.
About others above, Given cx->check isn't no-op on nightly opt build [1][2],
it's better adding it only to public JSAPI, and keep the private function as is.
we usually prepare 2 functions, public one (JS namespace) and private one (js namespace),
public one for extra check and type conversion, and private one for actual implementation.
// in jsapi.h
JS::Foo([parameters with public type]) {
// some extra check, and type conversion
js::Foo([parameters with private type])
}
// in internal file
js::Foo([parameters with private type]) {
// actual implementation here
}
Here, public API receives with JS::HandleObject, and private function can receive JS::Handle<PromiseObject*>.
js::SetSettledPromiseIsHandled can be moved to js/src/builtin/Promise-inl.h (doesn't exist yet), so that it can still be an inline function.
Then, about SetSettledPromiseIsHandled method, I think it should be DOM promise's method instead of Animation.
So that it already has global and corresponding JSContext, and no need to allocate AutoJSAPI again.
https://searchfox.org/mozilla-central/rev/f36cb2af46edd2659f446b7acdb2154e230ee445/dom/promise/Promise.h#317-318
[1] https://searchfox.org/mozilla-central/rev/f36cb2af46edd2659f446b7acdb2154e230ee445/js/src/vm/JSContext-inl.h#206-213
[2] https://searchfox.org/mozilla-central/rev/f36cb2af46edd2659f446b7acdb2154e230ee445/js/src/util/DiagnosticAssertions.h#16-18
|
|
Assignee | |
Comment 3•5 years ago
|
||
Arai-san, thank you so much!
Unfortunately today I didn't have time to incorporate this feedback but it is very helpful. Hopefully I will get to it later in the week. Thank you again!
|
|
Assignee | |
Comment 4•5 years ago
|
||
|
|
Assignee | |
Comment 5•5 years ago
|
||
|
|
Assignee | |
Comment 6•5 years ago
|
||
Depends on D67102
|
|
Assignee | |
Comment 7•5 years ago
|
||
As per spec change: https://github.com/w3c/csswg-drafts/commit/7bcad3219c09b788316d0aa4676cb962b6188162
Depends on D67103
|
||
Comment 11•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/7c1a4d74cc7a
https://hg.mozilla.org/mozilla-central/rev/14521d59a4cf
https://hg.mozilla.org/mozilla-central/rev/bd0f43a21b36
Description
•