Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the implementation of JSTraceable for RefCell. #8056

Merged
merged 1 commit into from Nov 3, 2015
Merged

Fix the implementation of JSTraceable for RefCell. #8056

merged 1 commit into from Nov 3, 2015

Conversation

@eefriedman
Copy link
Contributor

eefriedman commented Oct 17, 2015

The existing implementation could panic; make sure that doesn't
happen by requiring that the contents of a RefCell are trivially
traceable (i.e. the value don't contain any traceable objects).

I'm not sure whether the TriviallyJSTraceable trait is actually
worthwhile; maybe we should just never use RefCell in the DOM.

Review on Reviewable
@frewsxcv
Copy link
Member

frewsxcv commented Oct 17, 2015

Regression test?
@eefriedman
Copy link
Contributor Author

eefriedman commented Oct 17, 2015

This was found by inspection; I'm not sure it's actually possible to trigger this panic with Servo on master. The relevant sequence would be something like "call RefCell::borrow_mut(), then trigger a GC with the borrow active".

DOMRefCell sort of has the same issue, but the documentation for that explicitly acknowledges the problem, and it has a workaround.
@jdm
Copy link
Member

jdm commented Oct 17, 2015

We've attempted to keep RefCell out of script/dom/ in the past, but never got around to actually enforcing it via a lint.
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2015

☔ The latest upstream changes (presumably #8041) made this pull request unmergeable. Please resolve the merge conflicts.
@eefriedman eefriedman force-pushed the eefriedman:trace-refcell branch from 0247dc4 to 643127b Oct 23, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Oct 23, 2015

☔ The latest upstream changes (presumably #8091) made this pull request unmergeable. Please resolve the merge conflicts.
@eefriedman eefriedman force-pushed the eefriedman:trace-refcell branch from 643127b to 6486dd6 Oct 23, 2015
@eefriedman
Copy link
Contributor Author

eefriedman commented Oct 23, 2015

Rebased. (Should this be assigned to someone?)
@jdm jdm self-assigned this Oct 24, 2015
@jdm
Copy link
Member

jdm commented Nov 2, 2015

I would rather remove all uses of RefCell (and the implementation of JSTraceable for RefCell) than add another layer of annotations for "this type doesn't contain any GC-owned values". That's the purpose of no_jsmanaged_fields, which I believe is clearer.
@eefriedman
Remove JSTraceable implementation from RefCell.
Loading status checks…
df7fb8f 
The implementation wasn't really right, and we would rather just use
DOMRefCell anyway.
@eefriedman eefriedman force-pushed the eefriedman:trace-refcell branch from 6486dd6 to df7fb8f Nov 2, 2015
@eefriedman
Copy link
Contributor Author

eefriedman commented Nov 2, 2015

Okay; here's the version which just makes RefCell untraceable.
@jdm
Copy link
Member

jdm commented Nov 2, 2015

@bors-servo: r+
Thanks!

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io
@bors-servo
Copy link
Contributor

bors-servo commented Nov 2, 2015

📌 Commit df7fb8f has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

⌛ Testing commit df7fb8f with merge 4f51710...
bors-servo added a commit that referenced this pull request Nov 3, 2015
@bors-servo
Auto merge of #8056 - eefriedman:trace-refcell, r=jdm
Loading status checks…
4f51710 
Fix the implementation of JSTraceable for RefCell.

The existing implementation could panic; make sure that doesn't
happen by requiring that the contents of a RefCell are trivially
traceable (i.e. the value don't contain any traceable objects).

I'm not sure whether the TriviallyJSTraceable trait is actually
worthwhile; maybe we should just never use RefCell in the DOM.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8056)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

💔 Test failed - linux-rel
@eefriedman
Copy link
Contributor Author

eefriedman commented Nov 3, 2015

@bors-servo retry #7787
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

⚡ Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit df7fb8f into servo:master Nov 3, 2015
3 checks passed
3 checks passed
code-review/reviewable Review complete: all files reviewed, all discussions resolved
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Nov 3, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@eefriedman @frewsxcv @jdm @bors-servo @SimonSapin @highfive
You can’t perform that action at this time.