Implement job queue for ServiceWorkerRegistration #13574

creativcoder · 2016-10-04T14:12:42Z

Fixes #13424
As of now this PR (Work in Progress), will try to implement (partially) on algorithms namely : Schedule_Job, Run_Job, Register_Job.

An issue that needs to be addressed:
Resolving the promise value after queueing it as a dom_manipulation_task by wrapping it in a runnable RunJobHandler (which holds a TrustedPromise) and trying to call resolve_native results in a crash. stack trace

@jdm I am not sure what's causing the crash ?

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-10-04T14:12:46Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/serviceworkerregistration.rs, components/script/dom/serviceworkerjob.rs, components/script/dom/mod.rs, components/script/dom/bindings/trace.rs, components/script/script_thread.rs, components/script/dom/serviceworkercontainer.rs, components/script/dom/webidls/ServiceWorkerContainer.webidl

highfive · 2016-10-04T14:12:47Z

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!

jdm · 2016-10-04T14:19:34Z

You likely need a JSAutoCompartment in place before calling resolve_native.

bors-servo · 2016-10-07T14:09:16Z

☔ The latest upstream changes (presumably #13596) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2016-10-14T21:03:57Z

@creativcoder Were you looking to have these changes reviewed at all, or just asking for help with the crash?

creativcoder · 2016-10-14T21:30:31Z

@jdm I am at grandma's place and we barely have internet connection here to (sync+build) since the last rebase. I'll continue with the implementation when i reach college (hopefully by 20th) and notify you of code review. :)

jdm

Nice work!

jdm · 2016-11-01T18:24:55Z

components/script/dom/serviceworkerjob.rs

@@ -0,0 +1,202 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


I mainly try to keep implementations of interfaces in script/dom/, so this file belongs in script/ instead.

jdm · 2016-11-01T18:24:55Z

components/script/dom/bindings/trace.rs

@@ -328,6 +329,8 @@ no_jsmanaged_fields!(UntrustedNodeAddress);
 no_jsmanaged_fields!(LengthOrPercentageOrAuto);
 no_jsmanaged_fields!(RGBA);
 no_jsmanaged_fields!(EuclidLength<Unit, T>);
+no_jsmanaged_fields!(Job);
+no_jsmanaged_fields!(JobQueue);


This looks scary. Why not derive JSTraceable instead?

jdm · 2016-11-01T18:24:55Z

components/script/dom/promise.rs

@@ -200,6 +200,16 @@ impl Promise {
    }

    #[allow(unsafe_code)]
+    pub fn is_settled(&self) -> bool {
+        let mut _state = PromiseState::Pending;
+        unsafe { _state = GetPromiseState(self.promise_obj()); }


let state = unsafe { GetPromiseState(self.promise_object()) };

jdm · 2016-11-01T18:24:55Z

components/script/dom/serviceworkerjob.rs

+    fn eq(&self, other: &Self) -> bool {
+        let same_job = self.job_type == other.job_type;
+        if same_job {
+            return match self.job_type {


No need for return.

jdm · 2016-11-01T18:24:55Z

components/script/dom/serviceworkerjob.rs

+    #[allow(unrooted_must_root)]
+    // https://w3c.github.io/ServiceWorker/#run-job-algorithm
+    pub fn run_job(&mut self, run_job_handler: Box<RunJobHandler>, script_thread: &ScriptThread) {
+        let scope_url = run_job_handler.scope_url.clone();


Why is this clone necessary?

jdm · 2016-11-01T18:24:55Z

components/script/dom/serviceworkercontainer.rs

+        let container = ServiceWorkerContainer::new_inherited();
+        let client = Client::new(&global.as_window());
+        container.client.set(Some(&*client));
+        reflect_dom_object(box container, global, Wrap)


I propose we create the client first, and pass it as an argument to new_inherited instead. That should let us use JS<Client> instead.

jdm · 2016-11-01T18:24:55Z

components/script/dom/serviceworkercontainer.rs

    }
 }

 pub trait Controllable {
    fn set_controller(&self, active_worker: &ServiceWorker);
 }
-
+// TODO remove this trait usage


Any more details about this?

Setting the controller attribute to an active worker needs to happen at a much later stage (not during creation of registration), probably during activate algorithm. Since we are passing the client (which can be used to set the controller) with schedule_job, we may not need this trait then. We can set it from job.client.set_controller(sw).

jdm · 2016-11-01T18:24:55Z

components/script/dom/serviceworkerjob.rs

 #[must_root]
 pub struct Job {
    pub job_type: JobType,
    pub scope_url: Url,
    pub script_url: Url,
    pub promise: Rc<Promise>,
    pub equivalent_jobs: Vec<Job>,
+    // client can be a window client, worker client so `Client` will be an enum in future
+    pub client: Trusted<Client>,


It's odd to use Trusted and not TrustedPromise here. Why can't we use JS<Client>?

jdm · 2016-11-01T18:24:55Z

components/script/dom/serviceworkerjob.rs

@@ -111,12 +136,14 @@ impl JobQueue {
                           reg: &ServiceWorkerRegistration,
                           script_thread: &ScriptThread) {
        let promise = job.promise.clone();
-        // Initialize to an empty vec
-        self.0.insert(job.scope_url.clone(), vec![]);
+        if !self.0.contains_key(&job.scope_url) {


If we use the Entry API here instead, we can get clearer code I think.

jdm · 2016-11-01T18:24:55Z

components/script/dom/urlhelper.rs

+            let host = url.host_str().unwrap();
+            host == "127.0.0.0/8" || host == "::1/128"
+        // Step 5
+        } else  { url.scheme() == "file" }


nit: extra space before {, and move the check onto its own line.

creativcoder · 2016-11-16T13:09:04Z

Ran 4511 tests finished in 1393.0 seconds.
  • 4510 ran as expected. 1300 tests skipped.
  • 1 tests had unexpected subtest results

Tests with unexpected results:
  ▶ Unexpected subtest result in /_mozilla/mozilla/service-workers/service-worker-registration.html:
  │ FAIL [expected PASS] Test: Active Service Worker ScriptURL property
  │   → assert_equals: expected "http://web-platform.test:8000/_mozilla/mozilla/service-workers/resources/sw.js" but got "http://web-platform.test:8000/_mozilla/mozilla/service-workers/sw.js"
  │ 
  └ @http://web-platform.test:8000/_mozilla/mozilla/service-workers/service-worker-registration.html:43:5

Ah, so it took off resources from the script_url property. Lemme have a look.

creativcoder · 2016-11-17T16:54:08Z

@jdm Ah, yes. So in the wpt tests, https://github.com/servo/servo/blob/master/tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html#L18 is registered before https://github.com/servo/servo/blob/master/tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html#L47 . And both have the same scope url (being an empty string). So the update algorithm just returns us, previously registered one.
The ideal behaviour is to update the script with the newer one, which is the part in the second half of update algorithm (just filed an issue #14263 for this next PR). So as of now, I have switched the order of the two tests, to make them pass.


        Initial work on job queues for service workers

jdm · 2016-11-21T20:40:32Z

@bors-servo: r+

bors-servo · 2016-11-21T20:40:33Z

📌 Commit 114c491 has been approved by jdm

bors-servo · 2016-11-21T20:52:03Z

⌛ Testing commit 114c491 with merge 0ea0ade...


        Auto merge of #13574 - creativcoder:job-iface, r=jdm

@jdm

Implement job queue for ServiceWorkerRegistration  Fixes #13424 As of now this PR (Work in Progress), will try to implement (partially) on algorithms namely : [Schedule_Job](https://w3c.github.io/ServiceWorker/#schedule-job-algorithm), [Run_Job](https://w3c.github.io/ServiceWorker/#run-job-algorithm), [Register_Job](https://w3c.github.io/ServiceWorker/#register-algorithm). An issue that needs to be addressed: Resolving the promise value after queueing it as a `dom_manipulation_task` by wrapping it in a runnable `RunJobHandler` (which holds a `TrustedPromise`) and trying to call `resolve_native` results in a crash. [stack trace](http://paste.ofcode.org/zdjJ9npBgX6JsyWQkJjnYk) @jdm I am not sure what's causing the crash ? ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13574)

bors-servo · 2016-11-21T21:51:57Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

PeterW-LWL · 2016-11-23T15:47:38Z

components/script/dom/urlhelper.rs

+    // https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy
+    pub fn is_origin_trustworthy(url: &ServoUrl) -> bool {
+        // Step 3
+        if url.scheme() == "http" || url.scheme() == "wss" {


shouldn't this be https instead of http?

Yes, except that we don't support HTTPS in our automated tests yet. Certainly a FIXME comment here would be an improvement.

Noted. I'll add that comment


        Auto merge of #26111 - CYBAI:fix-origin-trustworthy, r=<try>

Update checking origin trustworthy align to spec While reading the [spec of ` Is origin potentially trustworthy? `](https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy), I found our second step is wrong; then, I found Josh said we didn't check it with `https` because we didn't support https in tests yet (#13574 (comment)). IIRC, we've supported https wpt tests now so it might be fine to change it to align with spec. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #26111 - CYBAI:fix-origin-trustworthy, r=<try>

Update checking origin trustworthy align to spec While reading the [spec of ` Is origin potentially trustworthy? `](https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy), I found our second step is wrong; then, I found Josh said we didn't check it with `https` because we didn't support https in tests yet (#13574 (comment)). IIRC, we've supported https wpt tests now so it might be fine to change it to align with spec. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #26111 - CYBAI:fix-origin-trustworthy, r=<try>

Update checking origin trustworthy align to spec While reading the [spec of ` Is origin potentially trustworthy? `](https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy), I found our second step is wrong; then, I found Josh said we didn't check it with `https` because we didn't support https in tests yet (#13574 (comment)). IIRC, we've supported https wpt tests now so it might be fine to change it to align with spec. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #26111 - CYBAI:fix-origin-trustworthy, r=jdm

Update checking origin trustworthy align to spec While reading the [spec of ` Is origin potentially trustworthy? `](https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy), I found our second step is wrong; then, I found Josh said we didn't check it with `https` because we didn't support https in tests yet (#13574 (comment)). IIRC, we've supported https wpt tests now so it might be fine to change it to align with spec. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #26111 - CYBAI:fix-origin-trustworthy, r=<try>

Update checking origin trustworthy align to spec While reading the [spec of ` Is origin potentially trustworthy? `](https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy), I found our second step is wrong; then, I found Josh said we didn't check it with `https` because we didn't support https in tests yet (#13574 (comment)). IIRC, we've supported https wpt tests now so it might be fine to change it to align with spec. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #26111 - CYBAI:fix-origin-trustworthy, r=jdm

Update checking origin trustworthy align to spec While reading the [spec of ` Is origin potentially trustworthy? `](https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy), I found our second step is wrong; then, I found Josh said we didn't check it with `https` because we didn't support https in tests yet (#13574 (comment)). IIRC, we've supported https wpt tests now so it might be fine to change it to align with spec. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #26111 - CYBAI:fix-origin-trustworthy, r=jdm

Update checking origin trustworthy align to spec While reading the [spec of ` Is origin potentially trustworthy? `](https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy), I found our second step is wrong; then, I found Josh said we didn't check it with `https` because we didn't support https in tests yet (#13574 (comment)). IIRC, we've supported https wpt tests now so it might be fine to change it to align with spec. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #26111 - CYBAI:fix-origin-trustworthy, r=jdm

Update checking origin trustworthy align to spec While reading the [spec of ` Is origin potentially trustworthy? `](https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy), I found our second step is wrong; then, I found Josh said we didn't check it with `https` because we didn't support https in tests yet (#13574 (comment)). IIRC, we've supported https wpt tests now so it might be fine to change it to align with spec. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #26111 - CYBAI:fix-origin-trustworthy, r=jdm

Update checking origin trustworthy align to spec While reading the [spec of ` Is origin potentially trustworthy? `](https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy), I found our second step is wrong; then, I found Josh said we didn't check it with `https` because we didn't support https in tests yet (#13574 (comment)). IIRC, we've supported https wpt tests now so it might be fine to change it to align with spec. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

highfive assigned wafflespeanut Oct 4, 2016

highfive added the S-awaiting-review label Oct 4, 2016

wafflespeanut assigned jdm and unassigned wafflespeanut Oct 5, 2016

highfive added the S-needs-rebase label Oct 7, 2016

creativcoder force-pushed the creativcoder:job-iface branch from 04c9ee0 to 02812bf Oct 10, 2016

jdm added S-awaiting-answer and removed S-awaiting-review labels Oct 14, 2016

creativcoder force-pushed the creativcoder:job-iface branch from 02812bf to 34cd67c Oct 20, 2016

highfive added the S-awaiting-review label Oct 20, 2016

creativcoder force-pushed the creativcoder:job-iface branch 5 times, most recently from 6f63596 to 9616714 Oct 20, 2016

jdm removed S-awaiting-answer S-needs-rebase labels Oct 24, 2016

creativcoder force-pushed the creativcoder:job-iface branch from aff5c89 to 90579e1 Oct 24, 2016

jdm added the S-needs-rebase label Nov 1, 2016

jdm requested changes Nov 1, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 1, 2016

creativcoder force-pushed the creativcoder:job-iface branch from 90579e1 to b6f706d Nov 6, 2016

highfive removed the S-needs-code-changes label Nov 6, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 16, 2016

creativcoder force-pushed the creativcoder:job-iface branch from 0621549 to f43af21 Nov 17, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Nov 17, 2016

creativcoder mentioned this pull request Nov 17, 2016

Implement Update, Install algorithms for Service Workers api #14263

Open

Initial work on job queues for service workers

Loading status checks…

114c491

creativcoder force-pushed the creativcoder:job-iface branch from f43af21 to 114c491 Nov 21, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 21, 2016

bors-servo merged commit 114c491 into servo:master Nov 21, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 21, 2016

PeterW-LWL reviewed Nov 23, 2016

View changes

CYBAI mentioned this pull request Apr 4, 2020

Update checking origin trustworthy align to spec #26111

Merged

2 of 4 tasks complete

servo / servo

Implement job queue for ServiceWorkerRegistration #13574

Implement job queue for ServiceWorkerRegistration #13574

creativcoder commented Oct 4, 2016 •

edited

highfive commented Oct 4, 2016

highfive commented Oct 4, 2016

jdm commented Oct 4, 2016

bors-servo commented Oct 7, 2016

jdm commented Oct 14, 2016

creativcoder commented Oct 14, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

creativcoder commented Nov 16, 2016

creativcoder commented Nov 17, 2016 •

edited

jdm commented Nov 21, 2016

bors-servo commented Nov 21, 2016

bors-servo commented Nov 21, 2016

bors-servo commented Nov 21, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

		@@ -0,0 +1,202 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

servo / servo

Join GitHub today

Implement job queue for ServiceWorkerRegistration #13574

Implement job queue for ServiceWorkerRegistration #13574

Conversation

creativcoder commented Oct 4, 2016 • edited

highfive commented Oct 4, 2016

highfive commented Oct 4, 2016

jdm commented Oct 4, 2016

bors-servo commented Oct 7, 2016

jdm commented Oct 14, 2016

creativcoder commented Oct 14, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

creativcoder commented Nov 16, 2016

creativcoder commented Nov 17, 2016 • edited

jdm commented Nov 21, 2016

bors-servo commented Nov 21, 2016

bors-servo commented Nov 21, 2016

bors-servo commented Nov 21, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

creativcoder commented Oct 4, 2016 •

edited

creativcoder commented Nov 17, 2016 •

edited