Skip to content
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

Provide a Sender for ServiceWorker object to communicate to its ServiceWorkerGlobalScope #12773

Closed
creativcoder opened this issue Aug 8, 2016 · 10 comments

Comments

@creativcoder
Copy link
Contributor

@creativcoder creativcoder commented Aug 8, 2016

Note: This is assigned to myself.

In order to implement the postMessage api for the ServiceWorker object, we need to have a Sender to its corresponding ServiceWorkerGlobalScope. From the current implementation of SW api's, we do not have a way to store a Sender<ServiceWorkerScriptMsg>, in ServiceWorker object as the creation of object and its global scope is decoupled with the global scope being created inside the ServiceWorkerManager at a later stage.

The issue has been described here discussing approaches.

Need suggestions from @jdm

@jdm jdm added the A-content/dom label Aug 8, 2016
@jdm
Copy link
Member

@jdm jdm commented Aug 8, 2016

It could be an IpcSender to the ServiceWorkerManager which could route the messages, I suppose.

@nox nox added the C-assigned label Aug 9, 2016
@jdm
Copy link
Member

@jdm jdm commented Aug 10, 2016

To work around the cyclic issues, one option might be to do something like the OpaqueScriptLayoutChannel we used to have: 49d244d#diff-ac20d4dfaff4522ec78bdd6f12f7445cL233 . There may also be a way to use traits in a similar fashion as the rest of that commit...

@jdm
Copy link
Member

@jdm jdm commented Aug 10, 2016

Although pub struct OpaqueScriptLayoutChannel(pub (Box<Any + Send>, Box<Any + Send>)); will prevent us from sending it over IPC channels, which may prevent us from using it :<

@jdm
Copy link
Member

@jdm jdm commented Aug 10, 2016

Another idea: if we have a set of messages that can be sent to the ServiceWorkerGlobalScope from the ServiceWorker by routing them through the Constellation, what if those messages are defined in script_traits instead of trying to reuse ServiceWorkerScriptMsg?

@creativcoder
Copy link
Contributor Author

@creativcoder creativcoder commented Aug 15, 2016

Using a seperate message in script_traits would be an option, but then again we will also need to send StructuredCloneData(which resides in script) to ServiceWorkerGlobalScope for postMessage impl. :|

@jdm
Copy link
Member

@jdm jdm commented Aug 15, 2016

Sending StructuredCloneData wouldn't make much sense; we could send Vec<u8> instead by porting https://dxr.mozilla.org/mozilla-central/rev/6e191a55c3d23e83e6a2e72e4e80c1dc21516493/dom/base/StructuredCloneHolder.cpp#357 .

@creativcoder
Copy link
Contributor Author

@creativcoder creativcoder commented Aug 16, 2016

So, do you mean something like below would work ? The vec is Vec<u64>, as StructuredCloneData is a *mut u64

fn PostMessage(&self, cx: *mut JSContext, message: HandleValue) -> ErrorResult {
        let data = try!(StructuredCloneData::write(cx, message));
        let mut msg_vec : Vec<u64> = Vec::new();
        let mut msg_ptr = msg_vec.as_mut_ptr();
        data.move_to_arraybuffer(msg_ptr);
        if let Some(ref sender) = *self.msg_sender.borrow() {
            let _ = sender.send(msg_vec);
        }
        Ok(())
    }

where move_to_arraybuffer is defined on StructuredCloneData as:

/// converts a StructuredCloneData to Vec<u64>
    pub fn move_to_arraybuffer(&self, dst: *mut u64) {
        unsafe {
            ptr::copy_nonoverlapping(self.data, dst, self.nbytes);
        }
    }

Correct me if i misunderstood the way of converting a StructuredClone to a Vec<u8>.

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Aug 16, 2016

You could extract the bytes with

slice::from_raw_parts(self.data, self.nbytes).to_vec()

but the idea was very much to send StructuredCloneData around; that's what it's for. If we want to use it in script_traits, we could define it there with an api like

pub unsafe fn new(data: *mut u64, nbytes: size_t) -> StructuredCloneData {}
pub unsafe fn extract(self) -> (*mut u64, size_t) {}
@jdm
Copy link
Member

@jdm jdm commented Aug 16, 2016

Pointers don't serialize over IPC channels, hence the Vec I propose. @creativcoder, that should work, but the Vec needs to reserve the right number of bytes and use u8 to ensure that nbytes is correct.

@creativcoder
Copy link
Contributor Author

@creativcoder creativcoder commented Aug 17, 2016

Thanks @Ms2ger and @jdm for the valuable feedback. My mentioned snippet was causing servo to crash (most likely due to copy_nonoverlapping), but using slice::from_raw_parts as @Ms2ger suggested makes it work and postMessage now works on ServiceWorker object. Making a PR soon for this. :)

@creativcoder creativcoder mentioned this issue Aug 17, 2016
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Sep 15, 2016
Implement postMessage for ServiceWorkers

<!-- Please describe your changes on the following line: -->

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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12773

<!-- Either: -->
- [X] There are tests for these changes at `tests/html/service-worker`

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/12910)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.