Implement postMessage for ServiceWorkers #12910

creativcoder · 2016-08-17T19:00:26Z

Fixes #12773
r? @jdm

Changes:

Implements postMessage on ServiceWorker object.
Removes unused channels from sw and their scopes.
Fixes a crash when calling scope.script_chan() in sw-scopes event handling.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #12773

There are tests for these changes at tests/html/service-worker

This change is

highfive · 2016-08-17T19:00:30Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs
@KiChjang: components/script/serviceworker_manager.rs, components/script/dom/serviceworkerregistration.rs, components/script/dom/serviceworker.rs, components/script/dom/workerglobalscope.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/dom/webidls/ServiceWorker.webidl, components/script/script_thread.rs, components/script/dom/bindings/structuredclone.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/serviceworkerglobalscope.rs

highfive · 2016-08-17T19:00:32Z

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-08-23T23:53:35Z

Great work! Note that the specification has steps for postMessage that include using a different interface than the MessageEvent one, but I'm happy to leave that as a followup.

-S-awaiting-review +S-needs-code-changes

Reviewed 12 of 13 files at r1.
Review status: 12 of 13 files reviewed at latest revision, 10 unresolved discussions.

components/script/dom/serviceworker.rs, line 35 [r1] (raw file):

    script_url: DOMRefCell<String>,
    state: Cell<ServiceWorkerState>,
    #[ignore_heap_size_of = "Defined in std"]

Defined in ipc-channel. now.

components/script/dom/serviceworker.rs, line 118 [r1] (raw file):

            let _ = sender.send(msg_vec);
        } else {
            warn!("Could not communicate message to ServiceWorkerGlobalScope");

We should consider buffering messages here and sending them when the sender is received.

components/script/dom/serviceworkerglobalscope.rs, line 55 [r1] (raw file):

}

// Required for run_with_memory_reporting

I'm not sure what this is referring to.

components/script/dom/serviceworkerglobalscope.rs, line 89 [r1] (raw file):

    #[ignore_heap_size_of = "Defined in std"]
    msg_port: Receiver<DOMMessage>,
    #[ignore_heap_size_of = "Defined in std"]

This should be removed.

components/script/dom/serviceworkerglobalscope.rs, line 242 [r1] (raw file):

                true
            }
            MixedMessage::PostMessage(data) => {

I'm tempted to call self.handle_script_event(ServiceWorkerScriptMsg::CommonWorker(WorkerScriptMsg::DOMMessage(data))) here instead with the reconstructed StructuredCloneData.

components/script/dom/workerglobalscope.rs, line 401 [r1] (raw file):

            return service_worker.script_chan();
        } else {
            panic!("need to implement a sender for SharedWorker/ServiceWorker")

The /ServiceWorker can be removed now.

components/script/dom/bindings/structuredclone.rs, line 50 [r1] (raw file):

    /// Converts a StructuredCloneData to Vec<u64>
    pub fn move_to_arraybuffer(self) -> Vec<u64> {

This should be a Vec<u8>. Let's make it return the DOMMessage type instead.

components/script/dom/bindings/structuredclone.rs, line 57 [r1] (raw file):

    /// Converts back to StructuredCloneData using a pointer and no of bytes
    pub fn make_structured_clone(data: *mut u64, nbytes: size_t) -> StructuredCloneData {

Let's make this accept the tuple struct mentioned previously, rather than raw pointers and numbers, so that we can't create StructureCloneData values out of arbitrary data buffers.

components/script_traits/script_msg.rs, line 163 [r1] (raw file):

/// Message that gets passed to service worker scope on postMessage
#[derive(Deserialize, Serialize, Debug)]
pub struct DOMMessage(pub Vec<u64>);

This should hold a Vec<u8> instead.

tests/html/service-worker/index.html, line 29 [r1] (raw file):

    // The delay is due to the sender being set in the service worker object after the scope starts.
    // This won't be needed when service workers are wrapped with Promises.

My buffering proposal avoids this requirement.

Comments from Reviewable

jdm · 2016-08-23T23:56:30Z

My mistake, those are steps for Client.postMessage - I meant these steps instead, which uses ExtendableMessageEvent.

Reviewed 1 of 13 files at r1.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

Comments from Reviewable

creativcoder · 2016-08-24T13:45:01Z

Yep, i will be swapping MessageEvent with ExtendableMessageEvent interface. I used it initially as my focus was to lay out the pathway for postMessage impl with existing things, and to make this PR thin.

Review status: all files reviewed at latest revision, 10 unresolved discussions.

components/script/dom/serviceworker.rs, line 118 [r1] (raw file):

Previously, jdm (Josh Matthews) wrote…

We should consider buffering messages here and sending them when the sender is received.

So you mean a `Vec` in `ServiceWorker` object for buffering ?

components/script/dom/serviceworkerglobalscope.rs, line 55 [r1] (raw file):

Previously, jdm (Josh Matthews) wrote…

I'm not sure what this is referring to.

I needed to add that `ServiceWorkerChan` which implements `ScriptChan` , needed when `scope.script_chan()` is called in `run_with_memory_reporting` in sw-scope event handling.

Comments from Reviewable

jdm · 2016-08-24T13:49:43Z

Review status: all files reviewed at latest revision, 10 unresolved discussions.

components/script/dom/serviceworker.rs, line 118 [r1] (raw file):

Previously, creativcoder (Rahul Sharma) wrote…

So you mean a Vec<DOMMessage> in ServiceWorker object for buffering ?

Correct.

components/script/dom/serviceworkerglobalscope.rs, line 55 [r1] (raw file):

Previously, creativcoder (Rahul Sharma) wrote…

I needed to add that ServiceWorkerChan which implements ScriptChan , needed when scope.script_chan() is called in run_with_memory_reporting in sw-scope event handling.

I don't think we need to leave a comment to explain that.

Comments from Reviewable

creativcoder · 2016-08-24T21:28:26Z

So @jdm i gave a re-thought on this PR, and now that i know that StructuredCloneData can be destructured and we can pass Vec<u8> around, i think we don't need to go from SWM -> constellation -> script path (that is an overhead). So i have made changes to the PR, by storing the Vec<u8>'s into SWM by keyed in by the ServiceWorker script_url. And then when we start scope; we send all of the messages for any ServiceWorker a given script_url from there itself. And this works for multiple postMessage calls as well without timeouts. Updated PR coming in a few minutes. Let me know what you think.

jdm · 2016-08-24T21:42:53Z

-S-awaiting-review +S-needs-code-changes

Reviewed 10 of 10 files at r2.
Review status: 12 of 13 files reviewed at latest revision, 6 unresolved discussions.

components/script/serviceworker_manager.rs, line 114 [r2] (raw file):

                // Send any backed up messages from postMessage calls
                if let Some(ref mut msg_vec) = self.msg_backup.get_mut(&scope_things.script_url) {
                    let _ = msg_vec.iter().map(|msg| {

for msg in msg_vec.drain() {, then there's no need to collect or clear.

components/script/serviceworker_manager.rs, line 115 [r2] (raw file):

                if let Some(ref mut msg_vec) = self.msg_backup.get_mut(&scope_things.script_url) {
                    let _ = msg_vec.iter().map(|msg| {
                        let data = StructuredCloneData::make_structured_clone(msg.clone());

This clone shouldn't be necessary with the above change.

components/script/serviceworker_manager.rs, line 120 [r2] (raw file):

                    msg_vec.clear();
                } else {
                    warn!("Failed to send DOMMessage from postMessage");

Isn't this a perfectly normal situation if there are no buffered messages?

components/script/serviceworker_manager.rs, line 165 [r2] (raw file):

            },
            ServiceWorkerMsg::BackupMessage(msg, url) => {
                if !self.msg_backup.contains_key(&url) {

Traditionally one would use the entry API for this instead.

components/script/dom/serviceworker.rs, line 114 [r2] (raw file):

        let msg_vec = data.move_to_arraybuffer();
        // Backup the message in service worker manager and send when sw scope starts
        let script_url = Url::parse(&(*self.script_url.borrow()).clone()).unwrap();

Url::parse(&*self.script_url.borrow())

components/script/dom/serviceworker.rs, line 115 [r2] (raw file):

        // Backup the message in service worker manager and send when sw scope starts
        let script_url = Url::parse(&(*self.script_url.borrow()).clone()).unwrap();
        let _ = self.global().r().constellation_chan().send(ScriptMsg::BackupMessage(msg_vec, script_url));

Hmm, so we always bounce postMessage through the constellation now? BackupMessage isn't a good description, in that case.

Comments from Reviewable

creativcoder · 2016-08-25T11:51:26Z

Review status: 12 of 13 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

components/script/serviceworker_manager.rs, line 120 [r2] (raw file):

Previously, jdm (Josh Matthews) wrote…

Isn't this a perfectly normal situation if there are no buffered messages?

Ah yes. Removed the else block.

components/script/serviceworker_manager.rs, line 165 [r2] (raw file):

Previously, jdm (Josh Matthews) wrote…

Traditionally one would use the entry API for this instead.

Cool, that made it much shorter :)

components/script/dom/serviceworker.rs, line 115 [r2] (raw file):

Previously, jdm (Josh Matthews) wrote…

Hmm, so we always bounce postMessage through the constellation now? BackupMessage isn't a good description, in that case.

So `ForwardDOMMessage` looks okay to you?

Comments from Reviewable

jdm · 2016-08-25T12:39:23Z

-S-awaiting-review +S-needs-code-changes

Reviewed 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/constellation/constellation.rs, line 988 [r3] (raw file):

            FromScriptMsg::ForwardDOMMessage(msg_vec, script_url) => {
                if let Some(ref mgr) = self.swmanager_chan {
                    let _ = mgr.send(ServiceWorkerMsg::BackupMessage(msg_vec, script_url));

This is still an odd name here.

components/script/dom/serviceworker.rs, line 115 [r2] (raw file):

Previously, creativcoder (Rahul Sharma) wrote…

So ForwardDOMMessage looks okay to you?

The name is fine, but I'm not sure that the behaviour is - we now unconditionally send the message to the constellation, which forwards it to the SW manager. The manager stores it in the hashtable, but any message received after the worker is activated for the first time won't be received, right?

Comments from Reviewable


        store senders instead of buffering messages

creativcoder · 2016-09-07T06:49:51Z

components/script/dom/serviceworker.rs

-                           script_url.as_str(),
-                           scope_url,
-                           skip_waiting)
-    }


Refactored this into a single method as this was just an unneccessary indirection.

jdm · 2016-09-12T14:36:32Z

-S-needs-rebase -S-awaiting-review +S-needs-code-changes

Reviewed 17 of 17 files at r6, 17 of 17 files at r7, 5 of 5 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

components/script/dom/serviceworkerregistration.rs, line 42 at r8 (raw file):

               scope: String,
               container: &Controllable) -> Root<ServiceWorkerRegistration> {
        let scope_url = Url::parse(&scope).unwrap();

Let's make scope a Url, and change ServiceWorkerContainer::Register to not turn its url into a string quite so soon.

components/script/dom/bindings/structuredclone.rs, line 70 at r7 (raw file):

    /// Reads a structured clone.
    ///
    /// Panics if `JS_ReadStructuredClone` fails.

This documentation should be reinstated.

components/script/dom/bindings/structuredclone.rs, line 61 at r8 (raw file):

    /// Reads a structured clone.
    pub fn read_clone( global: GlobalRef, data: *mut u64, nbytes: size_t, rval: MutableHandleValue) {

nit: extra space after (. Also, does this need to be public?

Comments from Reviewable


        make structured clone an enum

creativcoder · 2016-09-15T10:06:14Z

Review status: 16 of 19 files reviewed at latest revision, 5 unresolved discussions.

components/script/dom/bindings/structuredclone.rs, line 61 at r8 (raw file):

Previously, jdm (Josh Matthews) wrote…

nit: extra space after (. Also, does this need to be public?

No. Made it private.

Comments from Reviewable

jdm · 2016-09-15T13:34:20Z

@bors-servo: r+

Reviewed 3 of 3 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

bors-servo · 2016-09-15T13:34:21Z

📌 Commit 314dedb has been approved by jdm

bors-servo · 2016-09-15T13:34:23Z

⌛ Testing commit 314dedb with merge 0ec4ea4...


        Auto merge of #12910 - creativcoder:swsender, r=jdm

@jdm

Implement postMessage for ServiceWorkers  Fixes #12773 r? @jdm Changes: * Implements `postMessage` on `ServiceWorker` object. * Removes unused channels from sw and their scopes. * Fixes a crash when calling `scope.script_chan()` in sw-scopes event handling. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12773  - [X] There are tests for these changes at `tests/html/service-worker`   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12910)

bors-servo · 2016-09-15T14:38:31Z

💔 Test failed - mac-rel-wpt

jdm · 2016-09-15T14:49:57Z

@bors-servo: retry

#12654

creativcoder · 2016-09-15T14:51:14Z

Ran 6851 tests finished in 2514.0 seconds.
  • 6849 ran as expected. 1340 tests skipped.
  • 2 tests timed out unexpectedly

Tests with unexpected results:
  ▶ TIMEOUT [expected OK] /XMLHttpRequest/send-usp.worker
  │ 
  │ Stack trace for thread "ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }"
  │ stack backtrace:
  │    0:        0x10f25a1fe - backtrace::backtrace::trace::h0e60ef08c7c34e9f
  │    1:        0x10f25a4ec - backtrace::capture::Backtrace::new::h8bf319c36d8f5d1b
  │    2:        0x10da7ac6c - servo::install_crash_handler::handler::h7801cf2e27f7ace0
  │    3:     0x7fff99b3df19 - _sigtramp
  │    4:        0x10e86d12f - 13JSCompartment4wrapEP9JSContextN2JS13MutableHandleIP8JSObjectEENS2_6HandleIS5_E
  │    5:        0x10e84aacd - 13JSCompartment4wrapEP9JSContextN2JS13MutableHandleINS2_5ValueEEENS2_6HandleIP8JSObjectE
  │    6:        0x10e831934 - _Z12JS_WrapValueP9JSContextN2JS13MutableHandleINS1_5ValueEEE
  │    7:        0x10de7a8f8 - std::panicking::try::do_call::h2963350797f2457f
  │    8:        0x10f2e8f0a - __rust_maybe_catch_panic
  │    9:        0x10e1910b7 - script::dom::bindings::codegen::Bindings::MessageEventBinding::MessageEventBinding::get_data::h739fa18f8a6d80ec
  │ Stack trace for thread "ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }"
  │ stack backtrace:
  │    0:        0x10f25a1fe - backtrace::backtrace::trace::h0e60ef08c7c34e9f
  │    1:        0x10f25a4ec - backtrace::capture::Backtrace::new::h8bf319c36d8f5d1b
  │    2:        0x10da7ac6c - servo::install_crash_handler::handler::h7801cf2e27f7ace0
  │    3:     0x7fff99b3df19 - _sigtramp
  │    4:        0x10da7acf0 - servo::install_crash_handler::handler::h7801cf2e27f7ace0
  │    5:     0x7fff99b3df19 - _sigtramp
  │    6:        0x10e86d12f - 13JSCompartment4wrapEP9JSContextN2JS13MutableHandleIP8JSObjectEENS2_6HandleIS5_E
  │    7:        0x10e84aacd - 13JSCompartment4wrapEP9JSContextN2JS13MutableHandleINS2_5ValueEEENS2_6HandleIP8JSObjectE
  │    8:        0x10e831934 - _Z12JS_WrapValueP9JSContextN2JS13MutableHandleINS1_5ValueEEE
  │    9:        0x10de7a8f8 - std::panicking::try::do_call::h2963350797f2457f
  │   10:        0x10f2e8f0a - __rust_maybe_catch_panic
  └   11:        0x10e1910b7 - script::dom::bindings::codegen::Bindings::MessageEventBinding::MessageEventBinding::get_data::h739fa18f8a6d80ec

  ▶ TIMEOUT [expected OK] /webgl/conformance-1.0.3/conformance/textures/origin-clean-conformance.html

Possibly #12654

bors-servo · 2016-09-15T15:34:52Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows-dev are reusable. Rebuilding only mac-rel-wpt...

bors-servo · 2016-09-15T16:38:30Z

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

highfive assigned jdm Aug 17, 2016

highfive added the S-awaiting-review label Aug 17, 2016

creativcoder force-pushed the creativcoder:swsender branch 2 times, most recently from 310c4dd to 1f5b94e Aug 18, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Aug 23, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 24, 2016

creativcoder force-pushed the creativcoder:swsender branch 4 times, most recently from 851826d to fec8bca Aug 24, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Aug 24, 2016

creativcoder force-pushed the creativcoder:swsender branch from fec8bca to e7d49cc Aug 25, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 25, 2016

creativcoder force-pushed the creativcoder:swsender branch from e7d49cc to 1976f24 Aug 25, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Aug 25, 2016

store senders instead of buffering messages

9dcb734

creativcoder force-pushed the creativcoder:swsender branch from 7cd7827 to 7a21fb2 Sep 7, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 7, 2016

creativcoder reviewed Sep 7, 2016
View changes

components/script/dom/serviceworker.rs Outdated

script_url.as_str(),

scope_url,

skip_waiting)

}

This comment has been minimized.

Sign in to view

creativcoder Sep 7, 2016
Author Contributor

Refactored this into a single method as this was just an unneccessary indirection.

creativcoder force-pushed the creativcoder:swsender branch 2 times, most recently from 35f33d1 to ca255f2 Sep 7, 2016

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Sep 12, 2016

make structured clone an enum

Loading status checks…

314dedb

creativcoder force-pushed the creativcoder:swsender branch from ca255f2 to 314dedb Sep 15, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 15, 2016

highfive removed the S-awaiting-review label Sep 15, 2016

highfive added the S-awaiting-merge label Sep 15, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 15, 2016

bors-servo merged commit 314dedb into servo:master Sep 15, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

servo / servo

Implement postMessage for ServiceWorkers #12910

Implement postMessage for ServiceWorkers #12910

creativcoder commented Aug 17, 2016 •

edited

highfive commented Aug 17, 2016

highfive commented Aug 17, 2016

jdm commented Aug 23, 2016

jdm commented Aug 23, 2016

creativcoder commented Aug 24, 2016

jdm commented Aug 24, 2016

creativcoder commented Aug 24, 2016 •

edited

jdm commented Aug 24, 2016

creativcoder commented Aug 25, 2016

jdm commented Aug 25, 2016

This comment has been minimized.

jdm commented Sep 12, 2016

creativcoder commented Sep 15, 2016

jdm commented Sep 15, 2016

bors-servo commented Sep 15, 2016

bors-servo commented Sep 15, 2016

bors-servo commented Sep 15, 2016

jdm commented Sep 15, 2016

creativcoder commented Sep 15, 2016

bors-servo commented Sep 15, 2016

bors-servo commented Sep 15, 2016

servo / servo

Join GitHub today

Implement postMessage for ServiceWorkers #12910

Implement postMessage for ServiceWorkers #12910

Conversation

creativcoder commented Aug 17, 2016 • edited

highfive commented Aug 17, 2016

highfive commented Aug 17, 2016

jdm commented Aug 23, 2016

jdm commented Aug 23, 2016

creativcoder commented Aug 24, 2016

jdm commented Aug 24, 2016

creativcoder commented Aug 24, 2016 • edited

jdm commented Aug 24, 2016

creativcoder commented Aug 25, 2016

jdm commented Aug 25, 2016

This comment has been minimized.

creativcoder Sep 7, 2016

jdm commented Sep 12, 2016

creativcoder commented Sep 15, 2016

jdm commented Sep 15, 2016

bors-servo commented Sep 15, 2016

bors-servo commented Sep 15, 2016

bors-servo commented Sep 15, 2016

jdm commented Sep 15, 2016

creativcoder commented Sep 15, 2016

bors-servo commented Sep 15, 2016

bors-servo commented Sep 15, 2016

creativcoder commented Aug 17, 2016 •

edited

creativcoder commented Aug 24, 2016 •

edited