Use a structured clone holder to store rooted clones #21218

gterzian · 2018-07-20T05:08:58Z

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

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

This change is

gterzian · 2018-07-20T05:10:07Z

@jdm I'm wondering if this addresses the problem you've identified in #21164 (it is actually crashing now, I think because of a problem with the raw pointer stuff)

gterzian · 2018-07-20T08:24:59Z

Wonders! It's not crashing anymore... problem was doing Rc::from_raw(closure as *const Rc<StructuredCloneHolder>);, which off-course had be be changed into Rc::from_raw(closure as *const StructuredCloneHolder);

The structured cloning of a blob test is also still passing, so I guess this one is ready for review...

KiChjang

So, the StructuredCloneHolder right now would only grow in size, since the caller of StructuredClone::read doesn't actually pop the element off of the holder. I imagine that this is the intended behavior instead?

KiChjang · 2018-07-23T17:13:44Z

components/script/dom/bindings/structuredclone.rs

    if tag == StructuredCloneTags::DomBlob as u32 {
-        return read_blob(cx, r)
+        let (rooted_blob, js_object) = read_blob(cx, r);
+        holder.blobs.borrow_mut().push(rooted_blob);


We need to be very careful here, because if holder.blob is being borrowed mutably or immutably elsewhere, this will panic. Specifically, there may be a chance that some code elsewhere borrows it immutably for reading, and then during the read, it performs yet another StructuredCloneRead, which would then run into a panic.

Should we use try_borrow_mut and do some error handling at this point? What to do if that returns an error? Should we abort the reading and return a ptr::null_mut()?

there may be a chance that some code elsewhere borrows it immutably for reading, and then during the read, it performs yet another StructuredCloneRead

The way I look at this, is that JS_ReadStructuredClone is basically a function call, and the 'callbacks' are a bit of a misnomer since they are simply called at the end of this function call, in case the engine encounters a StructuredCloneTags that doesn't match any of the tags it can handle itself, it then calls the read callback to see if it can handle it.

If my understanding is correct, it means it would be impossible to during the read, perform yet another StructuredCloneRead. Basically, one call to JS_ReadStructuredClone would need to return, which includes having called the 'callbacks', before another one could be executed.

In any case, I think it might still a good idea to use try_borrow_mut and return a ptr::null_mut() if that fails, 'just to be sure'.

What do you think?

That sounds good to me.

KiChjang · 2018-07-23T23:09:23Z

components/script/dom/globalscope.rs

@@ -78,6 +79,10 @@ pub struct GlobalScope {
    crypto: MutNullableDom<Crypto>,
    next_worker_id: Cell<WorkerId>,

+    /// A place to keep the clones read into this realm rooted.
+    #[ignore_malloc_size_of = "Rc<T> is hard"]
+    structured_clone_holder: Rc<StructuredCloneHolder>,


I'm actually more interested to know why this has to be an Rc<StructuredCloneHolder> here instead of just DomRefCell<Vec<DomRoot<Blob>>>. In the read_callback function under structuredclone.rs, you can get a reference to the global by simply calling GlobalScope::from_context(cx), so passing in a raw Rc pointer seems unnecessary to me.

My answer to 'why aRc' would be: 'it was easy to get a raw pointer out of it and cast it to *mut raw::c_void, and then build one back from the raw pointer on the other side'.

Would that be doable with a DomRefCell? (I see that a RefCell has a as_ptr method, and it doesn't seem to be available on the DomRefCell variant.).

Assuming we can get a raw pointer using as_ptr, how would we build it back on the read end from this pointer? I don't see the equivalent of Rc::from_raw on RefCell...

Any ideas?

Going back to what I suggested -- you already have a *mut JSContext, so you can make use of that to get a GlobalScope out of it, and in your read_callback function, you can simply call methods on GlobalScope to enqueue the DomRoot<Blob> into its data holder; there's no need to pass in a raw Rc pointer just to get a reference to the data holder.

Awesome, I'll give it a try! (sorry I actually missed your suggestion above on the first read...)

gterzian · 2018-07-24T03:02:18Z

So, the StructuredCloneHolder right now would only grow in size, since the caller of StructuredClone::read doesn't actually pop the element off of the holder. I imagine that this is the intended behavior instead?

It depends on the problem we're trying to solve, which is not entirely clear to me by the way.

I am under the impression that the problem is that previously a DomRoot<Blob> would be created from the cloned data, and the reflector would be returned from the 'read_callback' to the JS engine while the rooted blob would go out of scope. So my proposed solution was to keep this rooted blob alive for longer, by pushing it to this Vec that would live for as long as the GlobalScope would.

Yes it would only grow in size, and I am not sure when would be a good 'time' to potential pop it off and let it drop. My intuition would be 'when the JSobject is garbage collected', so perhaps via an impl of Drop on Blob?

gterzian · 2018-07-24T10:45:29Z

since the caller of StructuredClone::read doesn't actually pop the element off of the holder

Do you mean that we could just pop it off and drop it once the read operation has completed? I was under the impression that we might need to keep the Blob rooted while the JsObject might still be used by the JS code after the read operation has returned...

gterzian · 2018-07-24T12:03:35Z

@KiChjang Please take another look, I guess the last question is whether we want to pop the blob off of the vec at some point or not...

bors-servo · 2018-07-29T16:59:35Z

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

KiChjang · 2018-07-31T21:41:42Z

@gterzian So, we definitely need to pop the Dom<Blob> off at some point, but figuring out when is tricky, since not all of the use cases actually consume a JS Blob.

jdm · 2018-07-31T22:39:51Z

Huh, I left a detailed code review on this OR, but I can't find any trace of it now.

gterzian · 2018-08-01T08:57:55Z

@KiChjang You're right, and using this new DOMTracker would prevent using a Dom<Transferable> I think.

How about adding a StructureCloneHolder to the global, that would, well, just have one attribute of a DOMTracker for each type for which we implement structured cloning/transfering? It would the tags in the StructuredCloneTags in some way...

@jdm That's a real bummer, sorry to hear you lost that work...

KiChjang · 2018-08-02T21:21:58Z

@gterzian I'm not sure what you mean by DOMTracker? Perhaps you have code that exists in your local repo and didn't push to your fork?

jdm · 2018-08-02T21:29:03Z

components/script/dom/bindings/structuredclone.rs

@@ -111,7 +111,12 @@ unsafe fn read_blob(cx: *mut JSContext,
    let type_str = structured_reader.read_str();
    let target_global = GlobalScope::from_context(cx);
    let blob = Blob::new(&target_global, BlobImpl::new_from_bytes(blob_buffer), type_str);
-    return blob.reflector().get_jsobject().get()
+    let js_object = blob.reflector().get_jsobject().get();
+    if let Some(mut holder) = target_global.structured_clone_holder() {


As pointed out, this change means we leak any blob that is sent through the structured clone algorithm because it is now always reachable from the global. I propose we return to your original design, with an important modification: there's no need to use Rc; we should be able to create the holder on the stack in read_clone, since the holder does not need to exist beyond that stack frame when the cloned values have been read into the rooted rval value.

Ok, thanks, I will look into it...

jdm · 2018-08-02T22:08:25Z

Yes, that is an accurate summary :)

gterzian · 2018-08-06T13:44:55Z

@jdm ready for review

KiChjang

LGTM

jdm · 2018-08-07T14:25:12Z

components/script/dom/bindings/structuredclone.rs

@@ -104,14 +104,21 @@ impl StructuredCloneReader {
 }

 unsafe fn read_blob(cx: *mut JSContext,
-                    r: *mut JSStructuredCloneReader)
+                    r: *mut JSStructuredCloneReader,
+                    closure: *mut raw::c_void)


Let's make this take &mut StructuredCloneHolder instead. Then we don't need with_structured_clone_holder and can use holder.blob = Some(blob) directly.

jdm · 2018-08-07T14:25:12Z

components/script/dom/bindings/structuredclone.rs

                                   -> *mut JSObject {
    assert!(tag < StructuredCloneTags::Max as u32, "tag should be lower than StructuredCloneTags::Max");
    assert!(tag > StructuredCloneTags::Min as u32, "tag should be higher than StructuredCloneTags::Min");
    if tag == StructuredCloneTags::DomBlob as u32 {
-        return read_blob(cx, r)
+        return read_blob(cx, r, closure)


We can pass &mut *(closure as *mut StructuredCloneHolder) instead.


        use a structured clone holder to store rooted clones

gterzian · 2018-08-07T15:42:12Z

@jdm Ok, comments addressed...

jdm · 2018-08-07T16:41:43Z

@bors-servo r+
Looks good! Thanks for fixing this!

bors-servo · 2018-08-07T16:41:44Z

📌 Commit a5d7cd1 has been approved by jdm

bors-servo · 2018-08-07T16:49:27Z

⌛ Testing commit a5d7cd1 with merge 2ceb8dc...


        Auto merge of #21218 - gterzian:root_cloned_blobs, r=jdm

Use a structured clone holder to store rooted clones  ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #21164 (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/21218)

bors-servo · 2018-08-07T17:53:15Z

☀️ Test successful - android, 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, windows-msvc-dev
Approved by: jdm
Pushing 2ceb8dc to master...

gterzian changed the title ~~Use a structured clone holder to store rooted clones~~ [WIP] Use a structured clone holder to store rooted clones Jul 20, 2018

gterzian force-pushed the gterzian:root_cloned_blobs branch 2 times, most recently from 2a6ad46 to f5b1c03 Jul 20, 2018

highfive added the S-awaiting-review label Jul 20, 2018

gterzian changed the title ~~[WIP] Use a structured clone holder to store rooted clones~~ Use a structured clone holder to store rooted clones Jul 20, 2018

gterzian force-pushed the gterzian:root_cloned_blobs branch from f5b1c03 to 3ce004c Jul 20, 2018

gterzian mentioned this pull request Jul 22, 2018

Implement MessagePort and MessageChannel #16622

Closed

KiChjang reviewed Jul 23, 2018

View changes

jdm self-assigned this Jul 23, 2018

KiChjang reviewed Jul 23, 2018

View changes

gterzian force-pushed the gterzian:root_cloned_blobs branch 2 times, most recently from 70dfe55 to 931e632 Jul 24, 2018

gterzian force-pushed the gterzian:root_cloned_blobs branch 3 times, most recently from 44a8c14 to cce12db Jul 24, 2018

gterzian mentioned this pull request Jul 26, 2018

Implement document load cancellation #21111

Merged

0 of 5 tasks complete

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 27, 2018

highfive added the S-needs-rebase label Jul 29, 2018

jdm requested changes Aug 2, 2018

View changes

gterzian force-pushed the gterzian:root_cloned_blobs branch from cce12db to 55e27c7 Aug 6, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 6, 2018

gterzian force-pushed the gterzian:root_cloned_blobs branch 2 times, most recently from 2a91e35 to 5040142 Aug 6, 2018

gterzian force-pushed the gterzian:root_cloned_blobs branch 3 times, most recently from 4621900 to 155df0c Aug 6, 2018

KiChjang approved these changes Aug 6, 2018

View changes

jdm requested changes Aug 7, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Aug 7, 2018

use a structured clone holder to store rooted clones

Loading status checks…

a5d7cd1

gterzian force-pushed the gterzian:root_cloned_blobs branch from 155df0c to a5d7cd1 Aug 7, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 7, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 7, 2018

bors-servo merged commit a5d7cd1 into servo:master Aug 7, 2018
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

highfive removed the S-awaiting-merge label Aug 7, 2018

gterzian deleted the gterzian:root_cloned_blobs branch Aug 7, 2018

servo / servo

Use a structured clone holder to store rooted clones #21218

Use a structured clone holder to store rooted clones #21218

gterzian commented Jul 20, 2018 •

edited by SimonSapin

gterzian commented Jul 20, 2018 •

edited

gterzian commented Jul 20, 2018 •

edited

KiChjang 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.

gterzian commented Jul 24, 2018 •

edited

gterzian commented Jul 24, 2018

gterzian commented Jul 24, 2018

bors-servo commented Jul 29, 2018

KiChjang commented Jul 31, 2018

jdm commented Jul 31, 2018

gterzian commented Aug 1, 2018 •

edited

KiChjang commented Aug 2, 2018

This comment has been minimized.

This comment has been minimized.

jdm commented Aug 2, 2018

gterzian commented Aug 6, 2018

KiChjang left a comment

This comment has been minimized.

This comment has been minimized.

gterzian commented Aug 7, 2018

jdm commented Aug 7, 2018

bors-servo commented Aug 7, 2018

bors-servo commented Aug 7, 2018

bors-servo commented Aug 7, 2018

servo / servo

Join GitHub today

Use a structured clone holder to store rooted clones #21218

Use a structured clone holder to store rooted clones #21218

Conversation

gterzian commented Jul 20, 2018 • edited by SimonSapin

gterzian commented Jul 20, 2018 • edited

gterzian commented Jul 20, 2018 • edited

KiChjang left a comment

This comment has been minimized.

KiChjang Jul 23, 2018

This comment has been minimized.

gterzian Jul 24, 2018 • edited

This comment has been minimized.

KiChjang Jul 24, 2018

This comment has been minimized.

KiChjang Jul 23, 2018 • edited

This comment has been minimized.

gterzian Jul 24, 2018 • edited

This comment has been minimized.

KiChjang Jul 24, 2018

This comment has been minimized.

gterzian Jul 24, 2018 • edited

gterzian commented Jul 24, 2018 • edited

gterzian commented Jul 24, 2018

gterzian commented Jul 24, 2018

bors-servo commented Jul 29, 2018

KiChjang commented Jul 31, 2018

jdm commented Jul 31, 2018

gterzian commented Aug 1, 2018 • edited

KiChjang commented Aug 2, 2018

This comment has been minimized.

jdm Aug 2, 2018

This comment has been minimized.

gterzian Aug 2, 2018

jdm commented Aug 2, 2018

gterzian commented Aug 6, 2018

KiChjang left a comment

This comment has been minimized.

jdm Aug 7, 2018

This comment has been minimized.

jdm Aug 7, 2018

gterzian commented Aug 7, 2018

jdm commented Aug 7, 2018

bors-servo commented Aug 7, 2018

bors-servo commented Aug 7, 2018

bors-servo commented Aug 7, 2018

gterzian commented Jul 20, 2018 •

edited by SimonSapin

gterzian commented Jul 20, 2018 •

edited

gterzian commented Jul 20, 2018 •

edited

gterzian Jul 24, 2018 •

edited

KiChjang Jul 23, 2018 •

edited

gterzian Jul 24, 2018 •

edited

gterzian Jul 24, 2018 •

edited

gterzian commented Jul 24, 2018 •

edited

gterzian commented Aug 1, 2018 •

edited