Skip to content
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 #9097: Don't panic when calling *VertexAttrib* with invalid indices #9102

Closed
wants to merge 1 commit into from

Conversation

@simartin
Copy link
Contributor

simartin commented Dec 30, 2015

Fixes #9097

Review on Reviewable

@emilio
Copy link
Member

emilio commented Dec 30, 2015

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.


components/canvas/webgl_paint_task.rs, line 60 [r1] (raw file):
Good work finding the root cause of the panic :)

I think this is not the best solution though, since this check is needed in the DOM side too, and should generate an error there, instead of silently returning.

Normally whenever the WebGL task panics due to a gl error, it means that that error should have been handled in the DOM side.

As a quick fix, we could make these calls synchronous, and return a WebGLResult, but long term it's probably more efficient to make a GLLimits or GLContextInfo structure, pass it on context creation, and refresh it when needed so we can do the checks before sending messages all over the place.

It'd be a good idea to add it to https://github.com/ecoal95/rust-offscreen-rendering-context/, so it can be re-used, over all in WebRender (don't forget to derive Serialize so it can be sent).

For now it could just have a single member (max_vertex_attribs), but we should be able to add more context info there for use in the DOM side.


Comments from the review on Reviewable.io

@simartin
Copy link
Contributor Author

simartin commented Dec 30, 2015

components/canvas/webgl_paint_task.rs, line 60 [r1] (raw file):
Thanks for the hint. I'll work in that direction...


Comments from the review on Reviewable.io

@simartin
Copy link
Contributor Author

simartin commented Jan 9, 2016

components/canvas/webgl_paint_task.rs, line 60 [r1] (raw file):
rust-offscreen-rendering-context PR submitted: servo/surfman#48


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Jan 10, 2016

The latest upstream changes (presumably #9201) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Feb 24, 2016

@ecoal95 What's the status here? Is this waiting on changes from @simartin or review from you?

@emilio
Copy link
Member

emilio commented Feb 24, 2016

It's waiting changes from @simartin, I initially forgot to update the labels when reviewing, I'll do it now :)

@jdm
Copy link
Member

jdm commented Mar 10, 2016

I filed #9958 to keep track of this work. Feel free to reopen this PR if you want to keep working on it!

@jdm jdm closed this Mar 10, 2016
@simartin simartin deleted the simartin:issue_9097 branch Mar 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.