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

Ensure that coord system id is propagated for non-invertible nodes. #3331

Merged
merged 1 commit into from Nov 20, 2018

Conversation

Projects
None yet
4 participants
@gw3583
Collaborator

gw3583 commented Nov 20, 2018

When a spatial node is encountered that has an invalid matrix, we
need to ensure that the coord system ID is always propagated. This
avoids array indexing errors that can occur during calls to
get_relative_transform.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1507323.


This change is Reviewable

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 20, 2018

r? @kvark or anyone

@gw3583

This comment has been minimized.

@kvark

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gw3583)


webrender/src/spatial_node.rs, line 196 at r1 (raw file):

    pub fn mark_uninvertible(
        &mut self,
        state: &mut TransformUpdateState,

let's pass it as immutable &


webrender/src/spatial_node.rs, line 199 at r1 (raw file):

    ) {
        self.invertible = false;
        self.coordinate_system_id = state.current_coordinate_system_id;

actually, wouldn't it be simpler to just move this assignment straight into update() body (at the beginning)?
assigning some random given coord space ID doesn't exactly fit into "make_uninvertible" semantics, sorry to be pedantic about that :)

Ensure that coord system id is propagated for non-invertible nodes.
When a spatial node is encountered that has an invalid matrix, we
need to ensure that the coord system ID is always propagated. This
avoids array indexing errors that can occur during calls to
get_relative_transform.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1507323.

@gw3583 gw3583 force-pushed the gw3583:fix-cst-crash branch from 899a61b to cae7704 Nov 20, 2018

@gw3583

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @kvark)


webrender/src/spatial_node.rs, line 196 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

let's pass it as immutable &

Done


webrender/src/spatial_node.rs, line 199 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

actually, wouldn't it be simpler to just move this assignment straight into update() body (at the beginning)?
assigning some random given coord space ID doesn't exactly fit into "make_uninvertible" semantics, sorry to be pedantic about that :)

I think the semantics are clearer as is - because we generally assign a coord system id later during update_transform, so it seems better to only assign here if we are early exiting.

@emilio

This comment has been minimized.

Member

emilio commented Nov 20, 2018

(Is there any chance to land a crashtest of some sort for this?)

@kvark

kvark approved these changes Nov 20, 2018

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 20, 2018

@emilio Yea - it's a bit tricky in wrench, since it relies on animation. But we should be able to either do a wrench rawtest or a browser crashtest.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 20, 2018

Try run looks good.

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 20, 2018

📌 Commit cae7704 has been approved by kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 20, 2018

⌛️ Testing commit cae7704 with merge 229436b...

bors-servo added a commit that referenced this pull request Nov 20, 2018

Auto merge of #3331 - gw3583:fix-cst-crash, r=kvark
Ensure that coord system id is propagated for non-invertible nodes.

When a spatial node is encountered that has an invalid matrix, we
need to ensure that the coord system ID is always propagated. This
avoids array indexing errors that can occur during calls to
get_relative_transform.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1507323.

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

This comment has been minimized.

Contributor

bors-servo commented Nov 20, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 229436b to master...

@bors-servo bors-servo merged commit cae7704 into servo:master Nov 20, 2018

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
code-review/reviewable 1 file reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment