Fix uses of JS<T> as a type on the stack #8026

eefriedman · 2015-10-15T02:14:54Z

JS<T> belongs on the heap, and only on the heap. This is a collection of fixes so that code uses either Root<T> or &T to pass around garbage-collected pointers.

Ideally, we could completely ban constructing a JS<T> outside of constructor functions, but we aren't quite there yet.

highfive · 2015-10-15T02:14:58Z

Warning

These commits modify unsafe code. Please review it carefully!

nox · 2015-10-15T12:30:25Z

Nice work!

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

Reviewed 7 of 7 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 11 of 11 files at r4, 7 of 7 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

^{components/script/dom/bindings/js.rs, line 232 [r1] (raw file):}
Why not bound T with HeapSizeOf and derive that impl?

^{components/script/dom/bindings/js.rs, line 305 [r1] (raw file):}
Why not bound T with HeapSizeOf and derive that impl?

^{components/script/dom/document.rs, line 304 [r4] (raw file):}
base.r() should work just fine.

^{components/script/dom/nodelist.rs, line 106 [r5] (raw file):}
last_visited.r() should work just fine.

Comments from the review on Reviewable.io

eefriedman · 2015-10-15T18:41:23Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

^{components/script/dom/bindings/js.rs, line 232 [r1] (raw file):}
The derive macro can't derive the impl because of the UnsafeCell. As for delegating... it seems kind of pointless.

^{components/script/dom/document.rs, line 304 [r4] (raw file):}
Ah... I didn't realize the RootedReference trait existed.

Comments from the review on Reviewable.io

bors-servo · 2015-10-15T19:41:56Z

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


        Stop implementing Copy for JS<T>.

A copy of a JS<T> doesn't have the rooting properties of the original, so it makes no sense for it to implement Copy.


        Don't return a JS<T> from bound_texture_for/

It's never correct to return a JS<T>. Maybe the lint should catch this?


        Don't use MutNullableHeap in HTMLCanvasElement.

MutNullableHeap is going away in favor of MutNullableHeapJS.


        Make get() and set() on MutNullableHeap use the correct types.

get() must always return a rooted value, because we have no way of ensuring the value won't be invalidated. set() takes an &T because it's convenient; there isn't any need to expose JS<T>.


        Fix remaining MutHeap methods not to expose JS<T>.


        Fix documentation for JS<T> and friends.


        Simplify extracting a reference from an Option<Root<T>>, per review c…

…omments.

eefriedman · 2015-10-15T22:27:58Z

Rebased, addressed review comments.

jdm · 2015-10-15T22:33:40Z

I'm curious if eefriedman@7a08b29 passes the rooting static analysis. I believe it shouldn't, since DOMRefCell isn't a must_root type.

eefriedman · 2015-10-15T22:54:04Z

Yes, this branch passes the lint. Why were you expecting it to trigger? (The current version of the must_root lint doesn't really reason about type parameters: it essentially just assumes Generic<Foo> is must_root if Foo is must_root.)

jdm · 2015-10-15T23:00:11Z

Mmm, I forgot that the work in #4336 only blacklisted some particular types, rather than fully supporting generic types :(

nox · 2015-10-16T13:41:30Z

@bors-servo r+

-S-needs-rebase

Reviewed 11 of 11 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

bors-servo · 2015-10-16T13:41:32Z

📌 Commit 9de42c8 has been approved by nox

bors-servo · 2015-10-16T14:06:00Z

⌛ Testing commit 9de42c8 with merge 7c7dbde...


        Auto merge of #8026 - eefriedman:js-rooting, r=nox

Fix uses of JS<T> as a type on the stack `JS<T>` belongs on the heap, and only on the heap. This is a collection of fixes so that code uses either `Root<T>` or `&T` to pass around garbage-collected pointers. Ideally, we could completely ban constructing a `JS<T>` outside of constructor functions, but we aren't quite there yet.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8026)

bors-servo · 2015-10-16T14:49:12Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

bholley · 2015-10-16T17:46:49Z

Can someone explain to me why &T is safe but JS is not?

nox · 2015-10-16T17:47:47Z

Because the only way to obtain &T is from Root<T> or a similar type.

bholley · 2015-10-16T17:58:50Z

I see. But what happens if the GC moves the T you're referencing with the &T? It seems like we'd need to be passing around &Root<T> for that to be safe.

jdm · 2015-10-16T18:51:40Z

The T will never be moved, since it's a Rust-allocated object instead of a JS reflector.

bholley · 2015-10-16T19:30:27Z

I see - so what happens when the GC wants to move the JS object allocated in the same buffer? Or are we not doing that anymore?

jdm · 2015-10-16T19:46:03Z

The future of fused/magic DOM objects is still undecided.

bholley · 2015-10-16T19:48:07Z

Ok - but what is the current behavior? Does the GC move the reflectors, or is moving GC disabled?

jdm · 2015-10-16T20:07:16Z

Moving GC should be enabled; the only GC feature I'm aware of that's disabled is incremental GC.

bholley · 2015-10-16T20:34:48Z

Oh - so the thing that isn't enabled is fused objects? I thought those were there from the start, but I suppose I'm probably wrong.

jdm · 2015-10-16T23:25:50Z

Right, we've always had separate reflectors and Rust DOM objects.

highfive added the S-awaiting-review label Oct 15, 2015

nox added A-content/dom I-safety S-needs-code-changes and removed S-awaiting-review labels Oct 15, 2015

nox self-assigned this Oct 15, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 15, 2015

eefriedman added 7 commits Oct 14, 2015

Stop implementing Copy for JS<T>.

88a1cbb

A copy of a JS<T> doesn't have the rooting properties of the original, so it makes no sense for it to implement Copy.

Don't return a JS<T> from bound_texture_for/

2d3c3ec

It's never correct to return a JS<T>. Maybe the lint should catch this?

Don't use MutNullableHeap in HTMLCanvasElement.

7a08b29

MutNullableHeap is going away in favor of MutNullableHeapJS.

Make get() and set() on MutNullableHeap use the correct types.

57584e7

get() must always return a rooted value, because we have no way of ensuring the value won't be invalidated. set() takes an &T because it's convenient; there isn't any need to expose JS<T>.

Fix remaining MutHeap methods not to expose JS<T>.

5bdf6bb

Fix documentation for JS<T> and friends.

5713867

Simplify extracting a reference from an Option<Root<T>>, per review c…

Loading status checks…

9de42c8

…omments.

eefriedman force-pushed the eefriedman:js-rooting branch from 11c7cdf to 9de42c8 Oct 15, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 15, 2015

nox removed the S-needs-rebase label Oct 16, 2015

highfive removed the S-awaiting-review label Oct 16, 2015

highfive added the S-awaiting-merge label Oct 16, 2015

bors-servo merged commit 9de42c8 into servo:master Oct 16, 2015
1 check passed

1 check passed

homu Test successful
Details

This was referenced Oct 16, 2015

Introduce trait Castable #8041

Merged

Remove HAS_DIRTY_SIBLINGS. #8042

Merged

eefriedman mentioned this pull request Oct 27, 2015

MutHeap/MutNullableHeap should return safe, rooted values #6684

Closed

servo / servo

Fix uses of JS<T> as a type on the stack #8026

Fix uses of JS<T> as a type on the stack #8026

eefriedman commented Oct 15, 2015

highfive commented Oct 15, 2015

nox commented Oct 15, 2015

eefriedman commented Oct 15, 2015

bors-servo commented Oct 15, 2015

eefriedman commented Oct 15, 2015

jdm commented Oct 15, 2015

eefriedman commented Oct 15, 2015

jdm commented Oct 15, 2015

nox commented Oct 16, 2015

bors-servo commented Oct 16, 2015

bors-servo commented Oct 16, 2015

bors-servo commented Oct 16, 2015

bholley commented Oct 16, 2015

nox commented Oct 16, 2015

bholley commented Oct 16, 2015

jdm commented Oct 16, 2015

bholley commented Oct 16, 2015

jdm commented Oct 16, 2015

bholley commented Oct 16, 2015

jdm commented Oct 16, 2015

bholley commented Oct 16, 2015

jdm commented Oct 16, 2015

servo / servo

Join GitHub today

Fix uses of JS<T> as a type on the stack #8026

Fix uses of JS<T> as a type on the stack #8026

Conversation

eefriedman commented Oct 15, 2015

highfive commented Oct 15, 2015

nox commented Oct 15, 2015

eefriedman commented Oct 15, 2015

bors-servo commented Oct 15, 2015

eefriedman commented Oct 15, 2015

jdm commented Oct 15, 2015

eefriedman commented Oct 15, 2015

jdm commented Oct 15, 2015

nox commented Oct 16, 2015

bors-servo commented Oct 16, 2015

bors-servo commented Oct 16, 2015

bors-servo commented Oct 16, 2015

bholley commented Oct 16, 2015

nox commented Oct 16, 2015

bholley commented Oct 16, 2015

jdm commented Oct 16, 2015

bholley commented Oct 16, 2015

jdm commented Oct 16, 2015

bholley commented Oct 16, 2015

jdm commented Oct 16, 2015

bholley commented Oct 16, 2015

jdm commented Oct 16, 2015