Skip to content
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

style: Don't assume siblings are alive in the rule tree when removing ourselves from the child list. #14167

Merged
merged 1 commit into from Nov 11, 2016

Conversation

@emilio
Copy link
Member

emilio commented Nov 10, 2016


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes OR

We can't assume all our siblings are alive because they may very well be in the
free list too.

This tempts to happen when the rule nodes are destroyed as part of the last GC,
the one that runs in the root destructor.

Also, properly put the next sibling back into the list when the rules are GCd.


This change is Reviewable

… ourselves from the child list.

We can't assume all our siblings are alive because they may very well be in the
free list too.

This tempts to happen when the rule nodes are destroyed as part of the last GC,
the one that runs in the root destructor.

Also, properly put the next sibling back into the list when the rules are GCd.
@highfive
Copy link

highfive commented Nov 10, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/rule_tree/mod.rs
@highfive
Copy link

highfive commented Nov 10, 2016

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@emilio
Copy link
Member Author

emilio commented Nov 10, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2016

Trying commit 65f239c with merge ced5aa3...

bors-servo added a commit that referenced this pull request Nov 10, 2016
style: Don't assume siblings are alive in the rule tree when removing ourselves from the child list.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

We can't assume all our siblings are alive because they may very well be in the
free list too.

This tempts to happen when the rule nodes are destroyed as part of the last GC,
the one that runs in the root destructor.

Also, properly put the next sibling back into the list when the rules are GCd.
@emilio
Copy link
Member Author

emilio commented Nov 10, 2016

@highfive highfive assigned SimonSapin and unassigned wafflespeanut Nov 10, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2016

💔 Test failed - mac-rel-wpt1

@jdm
Copy link
Member

jdm commented Nov 10, 2016

The failure was #11100.

@jdm jdm removed the S-tests-failed label Nov 10, 2016
@emilio
Copy link
Member Author

emilio commented Nov 11, 2016

Actually, simon is going to be away tomorrow. This can wait if needed, but blocks #8600 (or at least appears in the test failures), so... r? @Manishearth

@highfive highfive assigned Manishearth and unassigned SimonSapin Nov 11, 2016
@Manishearth
Copy link
Member

Manishearth commented Nov 11, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 11, 2016

📌 Commit 65f239c has been approved by Manishearth

@emilio
Copy link
Member Author

emilio commented Nov 11, 2016

@bors-servo retry

  • (the queue is empty but this is not on it, sigh)
@bors-servo
Copy link
Contributor

bors-servo commented Nov 11, 2016

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

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: #14175
@bors-servo
Copy link
Contributor

bors-servo commented Nov 11, 2016

📌 Commit 65f239c has been approved by Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Nov 11, 2016

Testing commit 65f239c with merge d49840e...

bors-servo added a commit that referenced this pull request Nov 11, 2016
style: Don't assume siblings are alive in the rule tree when removing ourselves from the child list.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

We can't assume all our siblings are alive because they may very well be in the
free list too.

This tempts to happen when the rule nodes are destroyed as part of the last GC,
the one that runs in the root destructor.

Also, properly put the next sibling back into the list when the rules are GCd.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14167)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 11, 2016

@bors-servo bors-servo merged commit 65f239c into servo:master Nov 11, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.