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 panic when transform is non-invertible #15334

Merged
merged 1 commit into from Feb 19, 2017
Merged

Fix the panic when transform is non-invertible #15334

merged 1 commit into from Feb 19, 2017

Conversation

@canova
Copy link
Member

canova commented Feb 2, 2017 •
edited

Fixes the panic when transform is non-invertible.
Counterpart of servo/webrender#823
r? @glennw

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #13266 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@highfive
Copy link

highfive commented Feb 2, 2017

warning Warning warning

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

canova commented Feb 2, 2017

I don't know exactly if I should write a test for it or not. It fixes a panic but it doesn't bring any new functionality.
@glennw
Copy link
Member

glennw commented Feb 2, 2017

There are existing tests that are expected CRASH due to this bug, so I would expect them to start passing and we'll have to update test expectations. You can test this locally by running:

./mach test-css --release --processes=4
@canova
Copy link
Member Author

canova commented Feb 2, 2017

@glennw I tested it but there is no passing test because it require other webrender PR to stop panicking entirely. I tried with both webrender and servo PR, there are some passing tests. But we need to wait for a wr update to mark them as passing.
@canova
Copy link
Member Author

canova commented Feb 15, 2017

@glennw I tested without this patch against most recent WR update. It looks like, it's showing (e.g. scale(0)) correctly without crashing. But when I try to click somewhere in servo, it still crashes without this PR. This PR prevents that crash but I couldn't find a test that checks after a click though.
@jdm
Copy link
Member

jdm commented Feb 15, 2017

It looks like it might be possible to trigger this from an automated test by using the Document.elementsFromPoint API, since that ends up doing a hit test on the display list.
@canova
Copy link
Member Author

canova commented Feb 16, 2017

I tried to trigger this with document.elementsFromPoint but it looks like this API is not working well. Page becomes all black and unresponsible when I use this.
@jdm
Copy link
Member

jdm commented Feb 16, 2017

Please file that as a separate issue with a testcase?
@canova
Copy link
Member Author

canova commented Feb 16, 2017

Filed one: #15592
@jdm
Copy link
Member

jdm commented Feb 16, 2017 •
edited

It should be possible to use elementsFromPoint from a setTimeout while #15592 is unfixed.
@canova canova force-pushed the canova:inverse branch from ea7bd31 to 164f4e3 Feb 16, 2017
@canova
Copy link
Member Author

canova commented Feb 16, 2017

Added a test for it. Thanks for the help!
@jdm
jdm reviewed Feb 16, 2017
View changes
tests/wpt/mozilla/tests/mozilla/non-invertible-transform.html Outdated
<body>
<div id="test"></div>
<script>
test(function() {

This comment has been minimized.

Sign in to view
@jdm

jdm Feb 16, 2017

Member

This needs to be an async test, or it will complete before the setTimeout executes.
@canova canova force-pushed the canova:inverse branch from 164f4e3 to 19b77a3 Feb 16, 2017
@canova
Copy link
Member Author

canova commented Feb 16, 2017

Updated code to use async_test.
@nox
Copy link
Member

nox commented Feb 19, 2017

I've checked that the test panics without the fix.

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2017

📌 Commit 19b77a3 has been approved by nox
@highfive highfive assigned nox and unassigned glennw Feb 19, 2017
@nox nox dismissed jdm’s stale review Feb 19, 2017

The test is now async.

@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2017

⌛ Testing commit 19b77a3 with merge 098397c...
bors-servo added a commit that referenced this pull request Feb 19, 2017
@bors-servo
Auto merge of #15334 - canaltinova:inverse, r=nox
Loading status checks…
098397c 
Fix the panic when transform is non-invertible

<!-- Please describe your changes on the following line: -->
Fixes the panic when transform is non-invertible.
Counterpart of servo/webrender#823
r? @glennw

---
<!-- 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
- [X] These changes fix #13266 (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

<!-- 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/15334)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2017

💔 Test failed - mac-dev-unit
@nox
Copy link
Member

nox commented Feb 19, 2017

@canaltinova The manifest is still incorrect.

diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json
index e5cfd66..cd55f26 100644
--- a/tests/wpt/mozilla/meta/MANIFEST.json
+++ b/tests/wpt/mozilla/meta/MANIFEST.json
@@ -25905,7 +25905,7 @@
    "testharness"
   ],
   "mozilla/non-invertible-transform.html": [
-   "c574749b1314922612fbd4ba46e1ef10a2c54074",
+   "6fa68fe08c9dd5d594e838da51617951193fee19",
    "testharness"
   ],
   "mozilla/out-of-order-stylesheet-loads-and-imports.html": [
@canova
Fix the panic when transform is non-invertible
Loading status checks…
f362f6f
@canova canova force-pushed the canova:inverse branch from 19b77a3 to f362f6f Feb 19, 2017
@canova
Copy link
Member Author

canova commented Feb 19, 2017

@nox Updated, thanks.
@nox
Copy link
Member

nox commented Feb 19, 2017

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2017

📌 Commit f362f6f has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2017

⌛ Testing commit f362f6f with merge d2ae3d8...
bors-servo added a commit that referenced this pull request Feb 19, 2017
@bors-servo
Auto merge of #15334 - canaltinova:inverse, r=nox
Loading status checks…
d2ae3d8 
Fix the panic when transform is non-invertible

<!-- Please describe your changes on the following line: -->
Fixes the panic when transform is non-invertible.
Counterpart of servo/webrender#823
r? @glennw

---
<!-- 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
- [X] These changes fix #13266 (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

<!-- 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/15334)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2017

☀️ 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
Approved by: nox
Pushing d2ae3d8 to master...
@bors-servo bors-servo merged commit f362f6f into servo:master Feb 19, 2017
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
@canova canova deleted the canova:inverse branch Feb 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@nox nox

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Unwrap called on fallible matrix inverse when translating point into stacking-context transform
6 participants
@canova @highfive @glennw @jdm @nox @bors-servo
You can’t perform that action at this time.