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

Implement transferable/cloneable objects #15021

Closed
jdm opened this issue Jan 13, 2017 · 16 comments
Closed

Implement transferable/cloneable objects #15021

jdm opened this issue Jan 13, 2017 · 16 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jan 13, 2017

Spec: https://html.spec.whatwg.org/multipage/infrastructure.html#transferable-objects, https://html.spec.whatwg.org/multipage/infrastructure.html#cloneable-objects

We currently do not pass pointers to JSStructuredCloneCallbacks when calling JS_WriteStructuredClone and JS_ReadStructuredClone. These callbacks control what to do when DOM objects are encountered in the structured clone algorithm. We'll need to duplicate what Gecko does to support this in Servo.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 13, 2017

This support is necessary for the transferable arguments to postMessage, which allows sending complex DOM objects between the main thread and workers.

@fflorent
Copy link
Contributor

@fflorent fflorent commented Jan 15, 2017

Hi @jdm! What are the requirements to be able to take this issue?

Florent

@jdm
Copy link
Member Author

@jdm jdm commented Jan 15, 2017

Ability to read Gecko's C++ code (as linked in the original comment) and translate that into equivalent Rust code, mainly!

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jan 16, 2017

This would be a nice thing to have for MessagePort as well. We currently have some code that deals with structured clones in components/script/dom/bindings/structuredclone.rs.

@gterzian
Copy link
Member

@gterzian gterzian commented Jan 17, 2017

Is someone already working on this one? If not, I would be happy to give it a try...

@jdm
Copy link
Member Author

@jdm jdm commented Jan 17, 2017

Let's give @fflorent the right of first refusal here.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jan 17, 2017

I'd also highly encourage the person to work on this discuss the implementation plan here first before attempting.

@fflorent
Copy link
Contributor

@fflorent fflorent commented Jan 17, 2017

Sure take it! :)

Florent

@gterzian
Copy link
Member

@gterzian gterzian commented Jan 18, 2017

@fflorent thanks!

@KiChjang @jdm I'll take some time to look into this, and will come back to discuss the implementation first :)

@gterzian
Copy link
Member

@gterzian gterzian commented Feb 5, 2017

Hi there, I've started looking into this, and the first questions I have are:

The stuff found in https://github.com/servo/rust-mozjs/, are those just stubs or actual implementations as well? For example I see Servo is using JS_WriteStructuredClone but I can't find the actual implementations, either in rust_mozjs or Servo.

Same thing for JSStructuredCloneCallbacks, are those actually implemented somewhere?

Looking at the Firefox code, I can see the actual callbacks implementation, so I'm wondering, does this issue require implementing those callbacks? If so, would that be done in rust_mozjs? Or are those already implemented and do they just need to be glued into Servo?

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 5, 2017

@gterzian rust-mozjs is a repository for Rust bindings to the JavaScript engine that we use called SpiderMonkey. It is essentially the js directory in Gecko.

@gterzian
Copy link
Member

@gterzian gterzian commented Feb 5, 2017

@KiChjang thanks for the info. Why is it that those JSStructuredCloneCallbacks are found within rust_mozjs, while in gecko they are in the dom folder, not js?

EDIT: I think I get it, the stuff in js are the basic types, the stuff in dom the actual implementations.

@gterzian
Copy link
Member

@gterzian gterzian commented Feb 5, 2017

Ok so here is what I can make out of the Gecko code:

  • There is this dom/base/StructuredCloneHolder.
  • Sometimes directly, sometimes through a lot of indirection, this StructuredCloneHolder ends up calling the various JS_* operations in js/public/StructuredClone.h
  • StructuredCloneHolder also defines a bunch of StructuredCloneHolder::sCallbacks, which are basically implementations of JSStructuredCloneCallbacks found in js/public/StructuredClone
  • When calling the various JS_* operations, it passes around those StructuredCloneHolder::sCallbacks
  • When those callbacks end up being called, they in turn call handler operations back in dom/base/StructuredCloneHolder. For example StructuredCloneCallbacksRead will call CustomReadHandler, and return the value back to the caller of the callback...
  • There are also a bunch of StructuredCloneTags, which are used in the handler operations to handle various types of cloned values being read/written.

So basically, like Gecko, we need to:

So my question now is, am I getting it somewhat right? And also, am I right to think that, while we can implement all the callbacks, we should probably just start with one or two handler operations? I believe you mentioned two potential use cases("transferable arguments to postMessage" and "MessagePost"), so we should probably just do those for now?

@jdm
Copy link
Member Author

@jdm jdm commented Feb 5, 2017

Yep, that sounds like a correct summary of how all the pieces fit together. I think implementing stubs for all of the callbacks makes the most sense; we will presumably need some kind of delegation like https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/dom/base/StructuredCloneHolder.cpp#62-65, and then we can focus on supporting a single small use case like using postMessage to clone a Blob object when communicating with a worker.

@gterzian
Copy link
Member

@gterzian gterzian commented Feb 6, 2017

@jdm awesome, thanks! I'll get started then...

@gterzian
Copy link
Member

@gterzian gterzian commented Feb 12, 2017

first pass at #15519, if you could please take a look...

bors-servo added a commit that referenced this issue Mar 20, 2017
…r=jdm

implement structured clone callbacks - support Blob cloning

<!-- Please describe your changes on the following line: -->
1. Implement stubs for structured clone callbacks.
2. Support Blob cloning.

Partial implementation of https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

---
<!-- 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
- [ ] These changes fix #15021 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/15519)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Mar 22, 2017
…r=jdm

implement structured clone callbacks - support Blob cloning

<!-- Please describe your changes on the following line: -->
1. Implement stubs for structured clone callbacks.
2. Support Blob cloning.

Partial implementation of https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

---
<!-- 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
- [ ] These changes fix #15021 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/15519)
<!-- 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.

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