Implement unhandledrejection event #20755

CYBAI · 2018-05-06T13:10:58Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Notify global object about rejected promises #15412
There are tests for these changes

This change is

highfive · 2018-05-06T13:11:02Z

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 · 2018-05-06T13:11:04Z

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!

CYBAI · 2018-05-06T13:13:00Z

components/script/dom/promisedebugging.rs

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


@jdm , @nox :

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

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

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 😓

components/script/dom/bindings/codegen/CodegenRust.py

components/script/dom/promiserejectionevent.rs

jdm · 2018-05-07T02:00:00Z

components/script/dom/promisedebugging.rs

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


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.

@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!

jdm · 2018-05-07T02:02:25Z

components/script/dom/promisedebugging.rs

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


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

CYBAI · 2018-05-12T17:03:49Z

components/script/dom/promisedebugging.rs

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


@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

@jdm ping for asking question, thanks!

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

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.

jdm · 2018-05-29T16:23:40Z

components/script/dom/promiserejectionevent.rs

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


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

jdm · 2018-05-29T16:24:14Z

components/script/dom/promiserejectionevent.rs

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


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

I will try to not use the DomRefCell here!

jdm · 2018-05-29T16:26:24Z

components/script/dom/webidls/PromiseRejectionEvent.webidl

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


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

jdm · 2018-05-29T16:27:57Z

components/script/dom/globalscope.rs

@@ -202,6 +222,22 @@ impl GlobalScope {
        GlobalScope::from_object(obj)
    }

+    pub fn set_uncaught_rejections(&self, rejection: HandleObject) {


Let's call this add_uncaught_rejection.

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

jdm · 2018-05-29T16:28:06Z

components/script/dom/globalscope.rs

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


Let's call this add_consumed_rejection.

jdm · 2018-05-29T16:29:30Z

components/script/dom/promisedebugging.rs

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


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.

@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:

servo/components/script/script_thread.rs

Lines 2653 to 2655 in d23bc4f

fn perform_a_microtask_checkpoint(&self) {

self.microtask_queue.checkpoint(|id| self.documents.borrow().find_global(id))

}

and the other is in GlobalScope:

servo/components/script/dom/globalscope.rs

Lines 521 to 524 in d23bc4f

/// 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?

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.

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.

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

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

components/script/script_runtime.rs

jdm · 2018-05-29T16:33:23Z

components/script/script_runtime.rs

+        match state {
+            PromiseRejectionHandlingState::Unhandled => {
+                let global = GlobalScope::from_context(cx);
+                PromiseDebugging::AddUncaughtRejection(global, promise);


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

jdm · 2018-05-29T16:37:08Z

components/script/dom/promisedebugging.rs

+    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>>| {


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().

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

jdm

This looks really close!

jdm · 2018-06-06T18:46:58Z

components/script/dom/promiserejectionevent.rs

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


nit: no need for the clone.

jdm · 2018-06-06T18:48:32Z

components/script/script_runtime.rs

+
+#[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>) {


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

jdm · 2018-06-06T18:55:33Z

components/script/script_runtime.rs

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


Can we just pass promise instead?

jdm · 2018-06-06T18:58:00Z

components/script/script_thread.rs

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


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

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

bors-servo · 2018-10-18T20:23:52Z

⌛ Trying commit 24f1afd with merge a371f16...

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20755)

bors-servo · 2018-10-18T21:21:19Z

☀️ Test successful - android, android-mac, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, status-taskcluster, windows-msvc-dev
State: approved= try=True

jdm · 2018-10-22T23:03:43Z

@bors-servo r+

bors-servo · 2018-10-22T23:03:44Z

📌 Commit 24f1afd has been approved by jdm

CYBAI · 2018-10-23T03:31:01Z

@bors-servo ping

bors-servo · 2018-10-23T03:31:02Z

😪 I'm awake I'm awake

bors-servo · 2018-10-23T03:48:26Z

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

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20755)

SimonSapin · 2018-10-23T09:29:01Z

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 · 2018-10-23T09:29:35Z

Opened new PR for upstreamable changes.

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

CYBAI · 2018-10-23T12:27:03Z

Hmm.. looks like homu still pend this PR 🤔

jdm · 2018-10-23T13:57:59Z

@bors-servo retry

bors-servo · 2018-10-23T14:56:44Z

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

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20755)

bors-servo · 2018-10-23T16:09:30Z

☀️ Test successful - android, android-mac, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, status-taskcluster, windows-msvc-dev
Approved by: jdm
Pushing 30d9962 to master...

highfive assigned jdm May 6, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 6, 2018

CYBAI commented May 6, 2018

View reviewed changes

emilio reviewed May 6, 2018

View reviewed changes

components/script/dom/bindings/codegen/CodegenRust.py Show resolved Hide resolved

components/script/dom/promiserejectionevent.rs Outdated Show resolved Hide resolved

jdm reviewed May 7, 2018

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 7, 2018

CYBAI force-pushed the unhandled-rejection branch from 344823d to 583beaa Compare May 7, 2018 14:30

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels May 7, 2018

CYBAI force-pushed the unhandled-rejection branch from 583beaa to 3071ff5 Compare May 12, 2018 16:54

CYBAI commented May 12, 2018

View reviewed changes

CYBAI force-pushed the unhandled-rejection branch from 3071ff5 to aa0254d Compare May 20, 2018 06:49

jdm requested changes May 29, 2018

View reviewed changes

CYBAI force-pushed the unhandled-rejection branch from aa0254d to 0c9042a Compare June 3, 2018 16:56

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jun 3, 2018

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 4, 2018

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jun 5, 2018

CYBAI force-pushed the unhandled-rejection branch 2 times, most recently from 5a46ecf to eb7d954 Compare June 5, 2018 16:41

jdm requested changes Jun 6, 2018

View reviewed changes

CYBAI removed the S-tests-failed The changes caused existing tests to fail. label Oct 19, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 22, 2018

SimonSapin closed this Oct 23, 2018

SimonSapin reopened this Oct 23, 2018

servo-wpt-sync mentioned this pull request Oct 23, 2018

Implement unhandledrejection event web-platform-tests/wpt#13681

Merged

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

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 23, 2018

This was referenced Oct 23, 2018

Support building for Magic Leap. #21985

Merged

Updating performance implementation and putting more measurements in #20459

Merged

[WIP] Named window getter #21869

Closed

CYBAI deleted the unhandled-rejection branch October 23, 2018 16:12

CYBAI mentioned this pull request Oct 23, 2018

Implement rejectionhandled event #22006

Closed

ferjm mentioned this pull request Oct 24, 2018

Intermittent TIMEOUT in /2dcontext/imagebitmap/canvas-createImageBitmap-video-resize.html #22010

Closed

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

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

Implement unhandledrejection event #20755

Implement unhandledrejection event #20755

Conversation

CYBAI commented May 6, 2018 • edited by nox

highfive commented May 6, 2018

highfive commented May 6, 2018

CYBAI May 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CYBAI May 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Oct 18, 2018

bors-servo commented Oct 18, 2018

jdm commented Oct 22, 2018

bors-servo commented Oct 22, 2018

CYBAI commented Oct 23, 2018

bors-servo commented Oct 23, 2018

bors-servo commented Oct 23, 2018

SimonSapin commented Oct 23, 2018

servo-wpt-sync commented Oct 23, 2018

CYBAI commented Oct 23, 2018 • edited

jdm commented Oct 23, 2018

bors-servo commented Oct 23, 2018

bors-servo commented Oct 23, 2018

CYBAI commented May 6, 2018 •

edited by nox

CYBAI May 6, 2018 •

edited

CYBAI May 7, 2018 •

edited

CYBAI commented Oct 23, 2018 •

edited