Use Heap instead of UnsafeCell in DOM reflectors by jdm · Pull Request #15118 · servo/servo

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 Promise rooting is not sound #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.

… are used.

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

@bors-servo: try

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

⌛ Trying commit e5eaab3 with merge 469a3f4...

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

    /// 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: Properly cancel event when beforeunload handler returns null #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...

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

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 ghost Jan 19, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 19, 2017

highfive assigned Ms2ger and unassigned ghost Jan 19, 2017

jdm mentioned this pull request Jan 19, 2017

Allow windows to share browsing contexts. #15120

Merged

4 tasks

jdm force-pushed the reflector-barrier-crash branch from 842eff5 to 3fe9521 Compare January 20, 2017 16:29

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

e5eaab3

… are used.

jdm force-pushed the reflector-barrier-crash branch from 3fe9521 to e5eaab3 Compare January 20, 2017 16:32

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 23, 2017

KiChjang reviewed Jan 23, 2017

View reviewed changes

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 24, 2017

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

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. 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

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 25, 2017

Ms2ger deleted the reflector-barrier-crash branch February 3, 2017 14:33

Uh oh!

Conversation

jdm commented Jan 19, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Jan 19, 2017

Uh oh!

jdm commented Jan 19, 2017

Uh oh!

Ms2ger commented Jan 20, 2017

Uh oh!

jdm commented Jan 20, 2017

Uh oh!

jdm commented Jan 20, 2017

Uh oh!

bors-servo commented Jan 20, 2017

Uh oh!

bors-servo commented Jan 20, 2017

Uh oh!

Ms2ger commented Jan 23, 2017

Uh oh!

jdm commented Jan 23, 2017

Uh oh!

bors-servo commented Jan 23, 2017

Uh oh!

jonco3 commented Jan 23, 2017

Uh oh!

KiChjang Jan 23, 2017

Choose a reason for hiding this comment

Uh oh!

jdm Jan 23, 2017

Choose a reason for hiding this comment

Uh oh!

jdm commented Jan 23, 2017

Uh oh!

bors-servo commented Jan 23, 2017

Uh oh!

bors-servo commented Jan 23, 2017

Uh oh!

Ms2ger commented Jan 24, 2017

Uh oh!

Ms2ger commented Jan 24, 2017

Uh oh!

bors-servo commented Jan 24, 2017

Uh oh!

bors-servo commented Jan 24, 2017

Uh oh!

Ms2ger commented Jan 24, 2017

Uh oh!

jdm commented Jan 24, 2017

Uh oh!

mbrubeck commented Jan 24, 2017

Uh oh!

bors-servo commented Jan 24, 2017

Uh oh!

bors-servo commented Jan 24, 2017

Uh oh!

bors-servo commented Jan 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jdm commented Jan 19, 2017 •

edited by larsbergstrom

Loading