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

Issue #20623: Check the input to WebGLRenderingContext's clear(). #20669

Merged
merged 1 commit into from May 20, 2018
Merged

Issue #20623: Check the input to WebGLRenderingContext's clear(). #20669

merged 1 commit into from May 20, 2018

Conversation

@simartin
Copy link
Contributor

simartin commented Apr 20, 2018 •
edited by SimonSapin

Validate the input to this function as per specifications.

  • ./mach build -d does not report any errors
  • ./mach build-geckolib does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #20623
  • There are tests for these changes

This change is Reviewable
@highfive
Copy link

highfive commented Apr 20, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webglrenderingcontext.rs
  • @fitzgen: components/script/dom/webglrenderingcontext.rs
  • @KiChjang: components/script/dom/webglrenderingcontext.rs
@nox
nox requested changes Apr 21, 2018
View changes
components/script/dom/webglrenderingcontext.rs Outdated
// - DEPTH_BUFFER_BIT
// - STENCIL_BUFFER_BIT
// - COLOR_BUFFER_BIT
match mask & !(constants::DEPTH_BUFFER_BIT | constants::STENCIL_BUFFER_BIT | constants::COLOR_BUFFER_BIT) {

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Apr 21, 2018

Member

Just use if.

if mask & !(constants::DEPTH_BUFFER_BIT | constants::STENCIL_BUFFER_BIT | constants::COLOR_BUFFER_BIT) != 0 {
    return self.webgl_error(InvalidValue);
}

This comment has been minimized.

Sign in to view
Copy link
@simartin

simartin Apr 21, 2018

Author Contributor

Yes indeed, thanks. PR amended accordingly.
components/script/dom/webglrenderingcontext.rs Outdated
// GL_INVALID_VALUE is generated if any bit other than the following is set in mask:
// - DEPTH_BUFFER_BIT
// - STENCIL_BUFFER_BIT
// - COLOR_BUFFER_BIT

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Apr 21, 2018

Member

Those comments just paraphrase the code. Please remove them.
tests/wpt/mozilla/meta/MANIFEST.json
@@ -39229,6 +39229,12 @@
{}
]
],
"mozilla/webgl/clear.html": [

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Apr 21, 2018

Member

@emilio We can't put tests there without causing issues for syncing with Khronos, right?

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio May 15, 2018

Member

Yeah, think so.

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm May 15, 2018

Member

No - we already have test files in that directory that are not part of the sync. It's fine to add additional ones.
@simartin
Issue #20623: Check the input to WebGLRenderingContext's clear().
Loading status checks…
f32ffeb
@simartin simartin force-pushed the simartin:issue_20623 branch from 05e04e7 to f32ffeb Apr 21, 2018
@nox
Copy link
Member

nox commented Apr 21, 2018

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Apr 21, 2018

⌛ Trying commit f32ffeb with merge 71054c1...
bors-servo added a commit that referenced this pull request Apr 21, 2018
@bors-servo
Auto merge of #20669 - simartin:issue_20623, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
71054c1 
Issue #20623: Check the input to WebGLRenderingContext's clear().

Validate the input to this function as per specifications.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach build-geckolib` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20623
- [X] There are tests for these changes

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

bors-servo commented Apr 21, 2018

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True
@simartin
Copy link
Contributor Author

simartin commented May 20, 2018

Is there anything outstanding for this PR to go in? Thanks.
@jdm
Copy link
Member

jdm commented May 20, 2018

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

bors-servo commented May 20, 2018

📌 Commit f32ffeb has been approved by jdm
@highfive highfive assigned jdm and unassigned mbrubeck May 20, 2018
@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2018

⌛ Testing commit f32ffeb with merge 0b57205...
bors-servo added a commit that referenced this pull request May 20, 2018
@bors-servo
Auto merge of #20669 - simartin:issue_20623, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
0b57205 
Issue #20623: Check the input to WebGLRenderingContext's clear().

Validate the input to this function as per specifications.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach build-geckolib` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20623
- [X] There are tests for these changes

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

bors-servo commented May 20, 2018

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 0b57205 to master...
@bors-servo bors-servo merged commit f32ffeb into servo:master May 20, 2018
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
@simartin simartin deleted the simartin:issue_20623 branch May 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

@emilio emilio

@nox nox

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Properly check the argument of WebGLRenderingContext.clear
7 participants
@simartin @highfive @nox @bors-servo @jdm @emilio @mbrubeck
You can’t perform that action at this time.