New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement unhandledrejection event #20755

Merged
merged 20 commits into from Oct 23, 2018

Conversation

Projects
None yet
@CYBAI
Collaborator

CYBAI commented May 6, 2018


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #15412
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented May 6, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/promiserejectionevent.rs, components/script/dom/webidls/PromiseRejectionEvent.webidl, components/script/dom/globalscope.rs, components/script/dom/promisedebugging.rs and 2 more
  • @fitzgen: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/promiserejectionevent.rs, components/script/dom/webidls/PromiseRejectionEvent.webidl, components/script/dom/globalscope.rs, components/script/dom/promisedebugging.rs and 2 more
  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/promiserejectionevent.rs, components/script/dom/webidls/PromiseRejectionEvent.webidl, components/script/dom/globalscope.rs, components/script/dom/promisedebugging.rs and 2 more
@highfive

This comment has been minimized.

highfive commented May 6, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
EventCancelable::Cancelable,
// FIXME(CYBAI): Use real rejected `promise` and `reason`
Promise::new(&global.upcast::<GlobalScope>()),
HandleValue::null()

This comment has been minimized.

@CYBAI

CYBAI May 6, 2018

Collaborator

@jdm , @nox :

I'm stuck by how to convert a HandleObject into a Rc<Promise> and Value into HandleValue here.

This comment has been minimized.

@jdm

jdm May 7, 2018

Member

We can use Promise::new_with_js_promise here if we make the method public.

This comment has been minimized.

@CYBAI

CYBAI May 7, 2018

Collaborator

Yes, new_with_js_promise is what I should use and just make it as a public method.

Previously, I also tried this method but the assert inside new_with_js_promise complained about that the passed HandleObject is not Promise. But now it works fine! I think I used wrong HandleObject before 😓

Show resolved Hide resolved components/script/dom/bindings/codegen/CodegenRust.py
}
impl PromiseRejectionEvent {
fn new_inherited(global: &GlobalScope) -> PromiseRejectionEvent {

This comment has been minimized.

@emilio

emilio May 6, 2018

Member

nit: Self? Here and in other return values?

let result = GetPromiseResult(promise);
rooted!(in(cx) let object = result.to_object());
let error_report = JS_ErrorFromException(cx, object.handle());
println!("{:?}, {:?}", result, error_report);

This comment has been minimized.

@jdm

jdm May 7, 2018

Member

I would expect this to look like:

rooted!(in(cx) let result = GetPromiseResult(promise));

We can then use result.handle() to object a HandleValue. I don't think there's any reason to expect an exception object here.

This comment has been minimized.

@CYBAI

CYBAI May 7, 2018

Collaborator

@jdm Yeah! With using rooted macro, it works that I can cast the type of result to be HandleValue! Thanks a lot!

About the exception object, we don't need it for unhandlerejection so just removed it!

EventCancelable::Cancelable,
// FIXME(CYBAI): Use real rejected `promise` and `reason`
Promise::new(&global.upcast::<GlobalScope>()),
HandleValue::null()

This comment has been minimized.

@jdm

jdm May 7, 2018

Member

We can use Promise::new_with_js_promise here if we make the method public.

global.set_uncaught_rejections(promise);
unsafe {
PromiseDebugging::flush_uncaught_rejections_internal(global);

This comment has been minimized.

@CYBAI

CYBAI May 12, 2018

Collaborator

@jdm cc @nox

In Gecko, flush uncaught rejections internal will only be triggered in FlushSync.

So, I'd like to confirm if I'm correct about doing the similar implementation in Servo like following:

  • Make the received uncaught rejection be queued into script thread with task which should be the step 4 in notify-about-rejected-promises spec instead of running the flush_uncaught_rejections_internal directly when AddUncaughtRejection executed

This comment has been minimized.

@CYBAI

CYBAI May 19, 2018

Collaborator

@jdm ping for asking question, thanks!

This comment has been minimized.

@CYBAI

CYBAI May 22, 2018

Collaborator

I will try to use task macro to handle Queue a task for unhandled rejection.

This comment has been minimized.

@jdm

jdm May 28, 2018

Member

Sorry for not responding. Yes, queuing a task using the task macro is the right way to implement step 4. However, that task should only be queued once per run of the event loop, and I believe AddUncaughtRejection can run an arbitrary number of times per run of the event loop. You'll want to invoke the notify about rejected promises algorithm as part of the perform a microtask checkpoint algorithm.

}
}
pub fn new_uninitialized(global: &GlobalScope) -> DomRoot<Self> {

This comment has been minimized.

@jdm

jdm May 29, 2018

Member

Let's remove this method and add the initial values as arguments to new_inherited.

pub struct PromiseRejectionEvent {
event: Event,
#[ignore_malloc_size_of = "Rc"]
promise: DomRefCell<Rc<Promise>>,

This comment has been minimized.

@jdm

jdm May 29, 2018

Member

We shouldn't need the DomRefCell if we pass the initial promise value to new_inherited.

This comment has been minimized.

@CYBAI

CYBAI May 30, 2018

Collaborator

I will try to not use the DomRefCell here!

};
dictionary PromiseRejectionEventInit : EventInit {
Promise<any> promise;

This comment has been minimized.

@jdm

jdm May 29, 2018

Member

Let's add /* required */ here, since it's in the spec but we don't support it yet.

@@ -202,6 +222,22 @@ impl GlobalScope {
GlobalScope::from_object(obj)
}
pub fn set_uncaught_rejections(&self, rejection: HandleObject) {

This comment has been minimized.

@jdm

jdm May 29, 2018

Member

Let's call this add_uncaught_rejection.

This comment has been minimized.

@CYBAI

CYBAI May 30, 2018

Collaborator

Indeed, using add as prefix for this function makes more sense! Thanks!

&self.uncaught_rejections
}
pub fn set_consumed_rejections(&self, rejection: HandleObject) {

This comment has been minimized.

@jdm

jdm May 29, 2018

Member

Let's call this add_consumed_rejection.

}
#[allow(unsafe_code)]
pub unsafe fn flush_uncaught_rejections_internal(global: DomRoot<GlobalScope>) {

This comment has been minimized.

@jdm

jdm May 29, 2018

Member

Let's move this function into script_runtime.rs; there's no need for the promisedebugging.rs file to exist. We should add a comment pointing at https://html.spec.whatwg.org/multipage/webappapis.html#notify-about-rejected-promises and call this function notify_about_rejected_promises. We will need to call it from MicrotaskQueue::checkpoint in the TODO in step 8.

This comment has been minimized.

@CYBAI

CYBAI Jun 3, 2018

Collaborator

@jdm cc @KiChjang

For this comment, I still doesn't implement it yet due to below question:

I tried to figure out how to call notify_about_rejected_promises inside MicrotaskQueue::checkpoint.
Due to GlobalScope as an argument in notify_about_rejected_promises, we'll need to pass a GlobalScope from MicrotaskQueue::checkpoint but it seems that we don't have a way to get GlobalScope in MicrotaskQueue. So, maybe we can only pass a GlobalScope to it.

Ex.

pub fn checkpoint<F>(&self, global: GlobalScope, target_provider: F)
    where F: Fn(PipelineId) -> Option<DomRoot<GlobalScope>>
{

However, it will have an issue that we'll call MicrotaskQueue::checkpoint in two places:

One is in ScriptThread:

fn perform_a_microtask_checkpoint(&self) {
self.microtask_queue.checkpoint(|id| self.documents.borrow().find_global(id))
}

and the other is in GlobalScope:

/// Perform a microtask checkpoint.
pub fn perform_a_microtask_checkpoint(&self) {
self.microtask_queue.checkpoint(|_| Some(DomRoot::from_ref(self)));
}

We must be able to get the globalscope inside GlobalScope module but we don't have globalscope under every ScriptThread.

How can we have GlobalScope for both caller places and pass it to notify_about_rejected_promises?
Do you have any advises?

This comment has been minimized.

@jdm

jdm Jun 3, 2018

Member

While investigating this question, I discovered and filed #20908. Since step 3 says we need to notify about rejected promises for each global associated with the script thread, I propose that the checkpoint method should accept a Vec<DomRoot<GlobalScope>> argument and iterate over it to notify all of the contained globals. For ScriptThread, this can be created by iterating over all known documents and getting their global. For GlobalScope, it can be a vector consisting of the self object.

This comment has been minimized.

@CYBAI

CYBAI Jun 3, 2018

Collaborator

Passing an vector of GlobalScope sounds reasonable to me and I will try it.

But, my only concern to the solution is that, in script thread, if there's no any global in it, then it won't trigger notify_about_rejected_promises method from checkpoint method, is this reasonable?
Or, that said, after having a solution for it like #20908 mentioned; then, we will have corresponding global in each checkpoint.

This comment has been minimized.

@jdm

jdm Jun 3, 2018

Member

I can't think of a way to have a script thread with no globals while still having rejected promises that require notification.

This comment has been minimized.

@CYBAI

CYBAI Jun 4, 2018

Collaborator

Indeed. While having rejected promises, it should have its own globals.
I will try to use the vector way to implement it, thanks!

Show resolved Hide resolved components/script/script_runtime.rs
match state {
PromiseRejectionHandlingState::Unhandled => {
let global = GlobalScope::from_context(cx);
PromiseDebugging::AddUncaughtRejection(global, promise);

This comment has been minimized.

@jdm

jdm May 29, 2018

Member

We can call global.add_uncaught_rejection directly instead of using PromiseDebugging.

pub unsafe fn flush_uncaught_rejections_internal(global: DomRoot<GlobalScope>) {
let cx = global.get_cx();
global.get_uncaught_rejections().borrow().iter().for_each(|promise: &Box<Heap<*mut JSObject>>| {

This comment has been minimized.

@jdm

jdm May 29, 2018

Member

For the task from step 4, we need to create a RootedVec to hold the list of promises before we clear the original list. We can then use that vector in the task, rather than global.get_uncaught_rejections().

This comment has been minimized.

@CYBAI

CYBAI Jun 3, 2018

Collaborator

I will try to address this comment when I call notify_about_rejected_promises inside MicrotaskQueue::checkpoint successfully.

@jdm

This looks really close!

reason: HandleValue
) -> DomRoot<Self> {
let ev = reflect_dom_object(
Box::new(PromiseRejectionEvent::new_inherited(promise.clone())),

This comment has been minimized.

@jdm

jdm Jun 6, 2018

Member

nit: no need for the clone.

#[allow(unsafe_code, unrooted_must_root)]
/// https://html.spec.whatwg.org/multipage/#notify-about-rejected-promises
pub unsafe fn notify_about_rejected_promises(global: DomRoot<GlobalScope>) {

This comment has been minimized.

@jdm

jdm Jun 6, 2018

Member

Let's take &GlobalScope as an argument instead. And instead of making this function unsafe, we should use unsafe blocks in the implementation.

atom!("unhandledrejection"),
EventBubbles::DoesNotBubble,
EventCancelable::Cancelable,
promise.duplicate(),

This comment has been minimized.

@jdm

jdm Jun 6, 2018

Member

Can we just pass promise instead?

|id| self.documents.borrow().find_global(id),
self.documents.borrow()
.iter()
.filter_map(|(id, _doc)| self.documents.borrow().find_global(id))

This comment has been minimized.

@jdm

jdm Jun 6, 2018

Member

What if we use map and return document.global() instead?

This comment has been minimized.

@CYBAI

CYBAI Jun 7, 2018

Collaborator

Oh! Sounds good idea! I will try it! Thanks!

@jdm

This comment has been minimized.

Member

jdm commented Jun 6, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jun 6, 2018

⌛️ Trying commit eb7d954 with merge 1dae6a0...

bors-servo added a commit that referenced this pull request Jun 6, 2018

Auto merge of #20755 - CYBAI:unhandled-rejection, r=<try>
[WIP] Implement unhandledrejection event

r? @jdm and @nox

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix part of #15412
- [ ] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20755)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jun 6, 2018

💔 Test failed - linux-rel-wpt

let target = Trusted::new(global.upcast::<EventTarget>());
global.as_window().dom_manipulation_task_source().queue(

This comment has been minimized.

@jdm

jdm Jun 6, 2018

Member

A bunch of worker tests panicked because the global isn't a window. We're going to need to expose dom_manipulation_task_source() on GlobalScope and ensure that worker globals can use it as well. That should just mean moving this to GlobalScope and creating it here to pass to GlobalScope::new_inherited.

This comment has been minimized.

@CYBAI

CYBAI Jun 10, 2018

Collaborator

Fixed in ed10310.

With making the first argument as a Box<ScriptChan>, I tried to make a method of dom_manipulation_task_source in GlobalScope and call dom_manipulation_task_source method from each own thread which is similar to the solution of NetworkingTaskSource.

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 18, 2018

⌛️ Trying commit 24f1afd with merge a371f16...

bors-servo added a commit that referenced this pull request Oct 18, 2018

Auto merge of #20755 - CYBAI:unhandled-rejection, r=<try>
Implement unhandledrejection event

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #15412
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20755)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

@CYBAI CYBAI removed the S-tests-failed label Oct 19, 2018

@jdm

This comment has been minimized.

Member

jdm commented Oct 22, 2018

@bors-servo r+

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 22, 2018

📌 Commit 24f1afd has been approved by jdm

@CYBAI

This comment has been minimized.

Collaborator

CYBAI commented Oct 23, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

😪 I'm awake I'm awake

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

⌛️ Testing commit 24f1afd with merge 759fee8...

bors-servo added a commit that referenced this pull request Oct 23, 2018

Auto merge of #20755 - CYBAI:unhandled-rejection, r=jdm
Implement unhandledrejection event

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #15412
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20755)
<!-- Reviewable:end -->
@SimonSapin

This comment has been minimized.

Member

SimonSapin commented Oct 23, 2018

This is still shown as "pending" in Homu’s queue although Buildbot doesn’t show any running jobs. Let’s try to get things unstuck.

@servo-wpt-sync

This comment has been minimized.

Collaborator

servo-wpt-sync commented Oct 23, 2018

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#13681.

@CYBAI

This comment has been minimized.

Collaborator

CYBAI commented Oct 23, 2018

Hmm.. looks like homu still pend this PR 🤔

@jdm

This comment has been minimized.

Member

jdm commented Oct 23, 2018

@bors-servo retry

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

⌛️ Testing commit 24f1afd with merge 30d9962...

bors-servo added a commit that referenced this pull request Oct 23, 2018

Auto merge of #20755 - CYBAI:unhandled-rejection, r=jdm
Implement unhandledrejection event

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #15412
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20755)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

@bors-servo bors-servo merged commit 24f1afd into servo:master Oct 23, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@CYBAI CYBAI deleted the CYBAI:unhandled-rejection branch Oct 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment