Skip to content

Comments

Make sure to correctly destroy old layers in collect_old_layers_for_pipeline.#7912

Merged
bors-servo merged 1 commit intoservo:masterfrom
eefriedman:compositor-layer
Oct 7, 2015
Merged

Make sure to correctly destroy old layers in collect_old_layers_for_pipeline.#7912
bors-servo merged 1 commit intoservo:masterfrom
eefriedman:compositor-layer

Conversation

@eefriedman
Copy link
Contributor

The previous code would skip calling clear_all_tiles(), which led to a
panic in rust-layers. ("You should have disposed of the pixmap properly
with destroy()! This pixmap will leak!")

Ran into this messing around with pdf.js; no minimized testcase.
Maybe related to #7895.

r? @pcwalton

Review on Reviewable

…ipeline.

The previous code would skip calling clear_all_tiles(), which led to a
panic in rust-layers. ("You should have disposed of the pixmap properly
with destroy()! This pixmap will leak!")

Ran into this messing around with pdf.js; no minimized testcase.
Maybe related to servo#7895.
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 7, 2015
@mrobinson
Copy link
Member

@bors-servo r+

This fixes reference tests for me locally, so I think we already have adequate test coverage.

@bors-servo
Copy link
Contributor

📌 Commit 0df71c7 has been approved by mrobinson

@highfive 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 Oct 7, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 0df71c7 with merge 217c7da...

bors-servo pushed a commit that referenced this pull request Oct 7, 2015
Make sure to correctly destroy old layers in collect_old_layers_for_pipeline.

The previous code would skip calling clear_all_tiles(), which led to a
panic in rust-layers. ("You should have disposed of the pixmap properly
with destroy()! This pixmap will leak!")

Ran into this messing around with pdf.js; no minimized testcase.
Maybe related to #7895.

r? @pcwalton

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

💔 Test failed - mac-dev-ref-unit

@highfive 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 Oct 7, 2015
@jdm
Copy link
Member

jdm commented Oct 7, 2015

@bors-servo
Copy link
Contributor

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

@bors-servo
Copy link
Contributor

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

@bors-servo bors-servo merged commit 0df71c7 into servo:master Oct 7, 2015
@SimonSapin SimonSapin removed the S-tests-failed The changes caused existing tests to fail. label Jan 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants