Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAvoid panics for empty or multibyte image usemap #16063
Conversation
highfive
commented
Mar 21, 2017
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon. |
|
cc @Ms2ger |
|
@bors-servo try |
Avoid panics for empty or multibyte image usemap <!-- Please describe your changes on the following line: --> Some check were added to make sure we can call `split_at` with no risk of panics (when value is empty or the first char is multibyte). --- <!-- 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 #15883 (github issue number if applicable). - [x] There are tests for these changes <!-- 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/16063) <!-- Reviewable:end -->
|
Sorry, nobody checked this (I think @Ms2ger is on PTO). |
|
|
|
Please instead do the same test changes as in #16128. |
|
@nox |
|
Indeed! Do add a test case for that too in the file modified in that other PR. |
|
Are such changes then upstreamed to w3c/wpt? |
|
Yes. |
|
Actually there is one already: https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/html/semantics/embedded-content/image-maps/image-map-processing-model/hash-name-reference-test-data.html#L118 does it even get executed? Can't see the ini for this file. |
|
So basically it should panic on https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/html/semantics/embedded-content/image-maps/image-map-processing-model/hash-name-reference-test-data.html#L118 on master, am I missing something? |
|
@methyl I think this test fails for reasons unrelated to the panic you fixed. |
|
Remove the expectation and run it to see why it actually fails. |
|
I think I know what's going on. The WPT test is just rendering the element, while the panic happens only when you click on it, so itself it won't uncover this bug. My test was covering exactly this case, do you think we don't need it? |
|
This looks good as written since the test is actually testing the changes we care about. |
|
|
Avoid panics for empty or multibyte image usemap <!-- Please describe your changes on the following line: --> Some check were added to make sure we can call `split_at` with no risk of panics (when value is empty or the first char is multibyte). --- <!-- 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 #15883 (github issue number if applicable). - [x] There are tests for these changes <!-- 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/16063) <!-- Reviewable:end -->
|
@jdm Thanks, I missed that somehow. |
|
|
methyl commentedMar 21, 2017
•
edited
Some check were added to make sure we can call
split_atwith no risk of panics (when value is empty or the first char is multibyte)../mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is