Implement Deref<Target=T> for JS<T> where T: Reflectable #8060

nox · 2015-10-17T20:29:27Z

We can only borrow JS<T> from rooted things, so it's safe to deref it.
The only types that provide mutable JS<T> things are MutHeap<JS<T>> and
MutNullableHeap<JS<T>>, which don't actually expose that they contain
JS<T> values.


        Implement Deref<Target=T> for JS<T> where T: Reflectable

We can only borrow JS<T> from rooted things, so it's safe to deref it. The only types that provide mutable JS<T> things are MutHeap<JS<T>> and MutNullableHeap<JS<T>>, which don't actually expose that they contain JS<T> values.


        Return a reference in BrowserContext::active_document()


        Return a reference in BrowserContext::frame_element()

highfive · 2015-10-17T20:29:31Z

Warning

These commits modify unsafe code. Please review it carefully!

eefriedman · 2015-10-18T01:04:47Z

The only types that provide mutable JS things are MutHeap<JS<T>> and
MutNullableHeap<JS<T>>

That isn't completely true. For example,

servo/components/script/script_task.rs

Line 409 in e31ad01

mouse_over_targets: DOMRefCell<Vec<JS<Node>>>,

(mouse_over_targets: DOMRefCell<Vec<JS<Node>>>,).

I'd be much more comfortable with this if we unified JS<T> and Root<T> along the lines of http://manishearth.github.io/blog/2015/09/01/designing-a-gc-in-rust/ .

nox · 2015-10-18T01:05:52Z

The case you point out isn't unsafe in terms of Rust, in the sense that only a panic will happen if we try to drop it while borrowed, AFAIK.

eefriedman · 2015-10-18T01:23:51Z

The way Deref makes the problem worse is that if you write something like mouse_over_targets.borrow_mut().pop().unwrap(), you can access the contents without realizing you forgot to root it. Might not be worth worrying about?

nox · 2015-10-18T01:25:56Z

Unwrapping the result should make @Manishearth's lint complain, because it puts JS<T> on the stack.

eefriedman · 2015-10-18T01:31:23Z

Yes, the lint is probably good enough that it isn't a big deal, just a minor thing to try and watch for when reviewing DOM code.

Manishearth · 2015-10-18T09:14:50Z

We should perhaps disallow cloning JS<T>?

nox · 2015-10-18T11:34:12Z

@Manishearth I thought we had removed that with Copy, will look into it.

Manishearth · 2015-10-18T11:41:45Z

Isn't Copy worse?

nox · 2015-10-18T11:42:36Z

That's what I'm saying, I thought we had removed Clone at the same time we removed Copy.

Manishearth · 2015-10-18T11:43:44Z

Still there: https://github.com/servo/servo/pull/8060/files#diff-77e42f966279a10a63070b5c62117a56R143

nox · 2015-10-18T12:00:16Z

We can't remove Clone, Sink heavily rely on it. But I don't think it's a problem, for the same reason as @eefriedman's example isn't.

dzbarsky · 2015-10-18T18:57:39Z

components/script/dom/document.rs

@@ -549,7 +549,6 @@ impl Document {
    pub fn send_title_to_compositor(&self) {
        let window = self.window();
        // FIXME(https://github.com/rust-lang/rust/issues/23338)
-        let window = window.r();


let compositor = self.window().compositor() and remove the above line and FIXME?

Good catch.


        Do not root Document::window


        Return a reference in Document::window()


        Return a reference in BrowserContext::active_window()


        Do not root DOMImplementation::document


        Do not root DOMParser::window


        Do not root DOMStringMap::element


        Do not root DOMTokenList::element


        Do not root InputActivationState::checked_radio


        Do not root HTMLScriptElement::parser_document


        Do not root Location::window


        Do not root NamedNodeMap::owner


        Do not root NodeIterator::root_node


        Do not root ChildrenList::node


        Do not root Performance::timing


        Do not root Sink::document


        Do not root ServoHTMLParser::document


        Return a reference in ServoHTMLParser::window()


        Do not root XMLHttpRequest::upload

Ms2ger · 2015-10-19T08:07:42Z

@bors-servo r+
-S-awaiting-review +S-awaiting-merge

Reviewed 23 of 23 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from the review on Reviewable.io

bors-servo · 2015-10-19T08:07:42Z

📌 Commit 6dc42dd has been approved by Ms2ger

bors-servo · 2015-10-19T09:38:56Z

⌛ Testing commit 6dc42dd with merge 8ff3897...


        Auto merge of #8060 - nox:deref-js, r=Ms2ger

Implement Deref<Target=T> for JS<T> where T: Reflectable We can only borrow `JS<T>` from rooted things, so it's safe to deref it. The only types that provide mutable `JS<T>` things are `MutHeap<JS<T>>` and `MutNullableHeap<JS<T>>`, which don't actually expose that they contain `JS<T>` values.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8060)

bors-servo · 2015-10-19T10:00:37Z

⛄ The build was interrupted to prioritize another pull request.

bors-servo · 2015-10-19T10:00:38Z

⚡ Previous build results for gonk, linux-dev, mac-dev-ref-unit are reusable. Rebuilding only android, linux-rel, mac-rel-css, mac-rel-wpt...

bors-servo · 2015-10-19T10:32:34Z

💔 Test failed - mac-rel-wpt

nox · 2015-10-19T11:36:00Z

@bors-servo retry #7731

bors-servo · 2015-10-19T12:32:08Z

⌛ Testing commit 6dc42dd with merge 1a376aa...


        Auto merge of #8060 - nox:deref-js, r=Ms2ger

Implement Deref<Target=T> for JS<T> where T: Reflectable We can only borrow `JS<T>` from rooted things, so it's safe to deref it. The only types that provide mutable `JS<T>` things are `MutHeap<JS<T>>` and `MutNullableHeap<JS<T>>`, which don't actually expose that they contain `JS<T>` values.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8060)

bors-servo · 2015-10-19T13:26:16Z

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

nox added 3 commits Oct 16, 2015

Implement Deref<Target=T> for JS<T> where T: Reflectable

6c7f370

We can only borrow JS<T> from rooted things, so it's safe to deref it. The only types that provide mutable JS<T> things are MutHeap<JS<T>> and MutNullableHeap<JS<T>>, which don't actually expose that they contain JS<T> values.

Return a reference in BrowserContext::active_document()

1f31d5b

Return a reference in BrowserContext::frame_element()

264e943

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

dzbarsky reviewed Oct 18, 2015
View changes

nox added 13 commits Oct 19, 2015

Do not root Document::window

e889b09

Return a reference in Document::window()

409b5e3

Return a reference in BrowserContext::active_window()

6ab7f64

Do not root DOMImplementation::document

ce6aab6

Do not root DOMParser::window

ff0da2f

Do not root DOMStringMap::element

822e6f0

Do not root DOMTokenList::element

71dcabf

Do not root InputActivationState::checked_radio

e8f358d

Do not root HTMLScriptElement::parser_document

dee3aec

Do not root Location::window

5889a75

Do not root NamedNodeMap::owner

6849510

Do not root NodeIterator::root_node

b149e0c

Do not root ChildrenList::node

17cd420

nox added 5 commits Oct 17, 2015

Do not root Performance::timing

b0d1ccd

Do not root Sink::document

7c47a6b

Do not root ServoHTMLParser::document

da06341

Return a reference in ServoHTMLParser::window()

d28de59

Do not root XMLHttpRequest::upload

Loading status checks…

6dc42dd

nox force-pushed the nox:deref-js branch from 19f9f4b to 6dc42dd Oct 19, 2015

Ms2ger added S-awaiting-merge and removed S-awaiting-review labels Oct 19, 2015

nox assigned Ms2ger Oct 19, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 19, 2015

nox added S-awaiting-merge and removed S-tests-failed labels Oct 19, 2015

bors-servo merged commit 6dc42dd into servo:master Oct 19, 2015
2 checks passed

2 checks passed

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

homu Test successful
Details

This was referenced Oct 19, 2015

Improve spec-compliance of script loading and execution during document startup #7979

Merged

Update URL-related interfaces and their tests up to spec #8008

Merged

nox deleted the nox:deref-js branch Oct 19, 2015

Manishearth mentioned this pull request Oct 20, 2015

twis38 servo/blog.servo.org#36

Merged

servo / servo

Implement Deref<Target=T> for JS<T> where T: Reflectable #8060

Implement Deref<Target=T> for JS<T> where T: Reflectable #8060

nox commented Oct 17, 2015

highfive commented Oct 17, 2015

eefriedman commented Oct 18, 2015

nox commented Oct 18, 2015

eefriedman commented Oct 18, 2015

nox commented Oct 18, 2015

eefriedman commented Oct 18, 2015

Manishearth commented Oct 18, 2015

nox commented Oct 18, 2015

Manishearth commented Oct 18, 2015

nox commented Oct 18, 2015

Manishearth commented Oct 18, 2015

nox commented Oct 18, 2015

This comment has been minimized.

This comment has been minimized.

Ms2ger commented Oct 19, 2015

bors-servo commented Oct 19, 2015

bors-servo commented Oct 19, 2015

bors-servo commented Oct 19, 2015

bors-servo commented Oct 19, 2015

bors-servo commented Oct 19, 2015

nox commented Oct 19, 2015

bors-servo commented Oct 19, 2015

bors-servo commented Oct 19, 2015

servo / servo

Join GitHub today

Implement Deref<Target=T> for JS<T> where T: Reflectable #8060

Implement Deref<Target=T> for JS<T> where T: Reflectable #8060

Conversation

nox commented Oct 17, 2015

highfive commented Oct 17, 2015

eefriedman commented Oct 18, 2015

nox commented Oct 18, 2015

eefriedman commented Oct 18, 2015

nox commented Oct 18, 2015

eefriedman commented Oct 18, 2015

Manishearth commented Oct 18, 2015

nox commented Oct 18, 2015

Manishearth commented Oct 18, 2015

nox commented Oct 18, 2015

Manishearth commented Oct 18, 2015

nox commented Oct 18, 2015

This comment has been minimized.

dzbarsky Oct 18, 2015

This comment has been minimized.

nox Oct 19, 2015

Ms2ger commented Oct 19, 2015

bors-servo commented Oct 19, 2015

bors-servo commented Oct 19, 2015

bors-servo commented Oct 19, 2015

bors-servo commented Oct 19, 2015

bors-servo commented Oct 19, 2015

nox commented Oct 19, 2015

bors-servo commented Oct 19, 2015

bors-servo commented Oct 19, 2015