Use Heap instead of UnsafeCell in DOM reflectors #15118

jdm · 2017-01-19T18:49:21Z

The previous Reflector implementation did not use post barriers, so we could crash when storing nursery objects in a Reflector structure that were later moved out of the nursery.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15085
There are tests for these changes

This change is

highfive · 2017-01-19T18:49:24Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/promise.rs, components/script/dom/bindings/reflector.rs
@KiChjang: components/script/dom/promise.rs, components/script/dom/bindings/reflector.rs

jdm · 2017-01-19T18:49:58Z

r? @Ms2ger

Ms2ger · 2017-01-20T10:46:09Z

I noticed that trace_reflector calls CallUnbarrieredObjectTracer; would that need to be changed as well?

jdm · 2017-01-20T16:29:20Z

You are correct. I have updated the code to account for that.


        Use Heap in DOM object reflector implementation to ensure GC barriers…

… are used.

jdm · 2017-01-20T16:32:44Z

@bors-servo: try

bors-servo · 2017-01-20T16:32:47Z

⌛ Trying commit e5eaab3 with merge 469a3f4...


        Auto merge of #15118 - jdm:reflector-barrier-crash, r=<try>

Use Heap instead of UnsafeCell in DOM reflectors The previous `Reflector` implementation did not use post barriers, so we could crash when storing nursery objects in a `Reflector` structure that were later moved out of the nursery. - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15085 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15118)

bors-servo · 2017-01-20T17:42:23Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

Ms2ger · 2017-01-23T10:08:08Z

I don't see anything wrong with this, but would feel more comfortable if you had someone from the SM GC side have a look too. (Feel free to take this as r+ either way.)

jdm · 2017-01-23T15:32:43Z

10:31 <jonco> jdm: this means you're using the equivalent of JS::Heap<> to store the reflector object right?
10:31 <jdm> jonco: yes
10:31 <jonco> jdm: that sounds good

@bors-servo: r=Ms2ger

bors-servo · 2017-01-23T15:32:44Z

📌 Commit e5eaab3 has been approved by Ms2ger

jonco3 · 2017-01-23T15:36:32Z

Using Heap is the right thing to do here. I have minimal knowledge of rust, but this looks good to me.

KiChjang · 2017-01-23T22:32:56Z

components/script/dom/bindings/reflector.rs

    }

    /// Return a pointer to the memory location at which the JS reflector
    /// object is stored. Used to root the reflector, as
    /// required by the JSAPI rooting APIs.
-    pub fn rootable(&self) -> *mut *mut JSObject {
-        self.object.get()
+    pub fn rootable(&self) -> &Heap<*mut JSObject> {


Should this be &mut instead?

I argue that we can discuss that separately, since the changes here achieve the same effect as previously existed.

jdm · 2017-01-23T22:43:22Z

@bors-servo: r=Ms2ger

bors-servo · 2017-01-23T22:43:22Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: #15142

bors-servo · 2017-01-23T22:43:23Z

📌 Commit e5eaab3 has been approved by Ms2ger

Ms2ger · 2017-01-24T09:09:59Z

@bors-servo try-

Ms2ger · 2017-01-24T09:10:12Z

@bors-servo retry

bors-servo · 2017-01-24T11:52:46Z

⌛ Testing commit e5eaab3 with merge 2b91bb3...


        Auto merge of #15118 - jdm:reflector-barrier-crash, r=Ms2ger

Use Heap instead of UnsafeCell in DOM reflectors The previous `Reflector` implementation did not use post barriers, so we could crash when storing nursery objects in a `Reflector` structure that were later moved out of the nursery. - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15085 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15118)

bors-servo · 2017-01-24T12:20:57Z

💔 Test failed - windows-gnu-dev

Ms2ger · 2017-01-24T15:15:50Z

@bors-servo retry servo/saltfs#585

jdm · 2017-01-24T19:16:17Z

@bors-servo: r-

mbrubeck · 2017-01-24T22:05:29Z

@bors-servo r=Ms2ger

bors-servo · 2017-01-24T22:05:30Z

📌 Commit e5eaab3 has been approved by Ms2ger

bors-servo · 2017-01-24T22:11:52Z

⌛ Testing commit e5eaab3 with merge 023a9c5...


        Auto merge of #15118 - jdm:reflector-barrier-crash, r=Ms2ger

Use Heap instead of UnsafeCell in DOM reflectors The previous `Reflector` implementation did not use post barriers, so we could crash when storing nursery objects in a `Reflector` structure that were later moved out of the nursery. - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15085 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15118)

bors-servo · 2017-01-25T00:06:27Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned wafflespeanut Jan 19, 2017

highfive added the S-awaiting-review label Jan 19, 2017

highfive assigned Ms2ger and unassigned wafflespeanut Jan 19, 2017

jdm mentioned this pull request Jan 19, 2017

Allow windows to share browsing contexts. #15120

Merged

4 of 4 tasks complete

jdm force-pushed the jdm:reflector-barrier-crash branch from 842eff5 to 3fe9521 Jan 20, 2017

Use Heap in DOM object reflector implementation to ensure GC barriers…

Loading status checks…

e5eaab3

… are used.

jdm force-pushed the jdm:reflector-barrier-crash branch from 3fe9521 to e5eaab3 Jan 20, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 23, 2017

KiChjang reviewed Jan 23, 2017

View changes

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 24, 2017

jdm added the S-blocked-on-closed-tree label Jan 24, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Jan 24, 2017

mbrubeck removed the S-blocked-on-closed-tree label Jan 24, 2017

bors-servo merged commit e5eaab3 into servo:master Jan 25, 2017
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 Jan 25, 2017

Ms2ger deleted the jdm:reflector-barrier-crash branch Feb 3, 2017

servo / servo

Use Heap instead of UnsafeCell in DOM reflectors #15118

Use Heap instead of UnsafeCell in DOM reflectors #15118

jdm commented Jan 19, 2017 •

edited by larsbergstrom

highfive commented Jan 19, 2017

jdm commented Jan 19, 2017

Ms2ger commented Jan 20, 2017

jdm commented Jan 20, 2017

jdm commented Jan 20, 2017

bors-servo commented Jan 20, 2017

bors-servo commented Jan 20, 2017

Ms2ger commented Jan 23, 2017

jdm commented Jan 23, 2017

bors-servo commented Jan 23, 2017

jonco3 commented Jan 23, 2017

This comment has been minimized.

This comment has been minimized.

jdm commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

Ms2ger commented Jan 24, 2017

Ms2ger commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

Ms2ger commented Jan 24, 2017

jdm commented Jan 24, 2017

mbrubeck commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 25, 2017

servo / servo

Join GitHub today

Use Heap instead of UnsafeCell in DOM reflectors #15118

Use Heap instead of UnsafeCell in DOM reflectors #15118

Conversation

jdm commented Jan 19, 2017 • edited by larsbergstrom

highfive commented Jan 19, 2017

jdm commented Jan 19, 2017

Ms2ger commented Jan 20, 2017

jdm commented Jan 20, 2017

jdm commented Jan 20, 2017

bors-servo commented Jan 20, 2017

bors-servo commented Jan 20, 2017

Ms2ger commented Jan 23, 2017

jdm commented Jan 23, 2017

bors-servo commented Jan 23, 2017

jonco3 commented Jan 23, 2017

This comment has been minimized.

KiChjang Jan 23, 2017

This comment has been minimized.

jdm Jan 23, 2017

jdm commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

Ms2ger commented Jan 24, 2017

Ms2ger commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

Ms2ger commented Jan 24, 2017

jdm commented Jan 24, 2017

mbrubeck commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 25, 2017

jdm commented Jan 19, 2017 •

edited by larsbergstrom