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 upAdd destructors to some WebGL objects, remove duplicated glutin dependency and try to enable the webgl reftests #8291
Conversation
…buffer This allows to cleanup resources earlier if they stop being used. Right now all resources were cleaned up when the context was destroyed, this is a slightly better approach. We ignore the possible failure of the send() call, since we don't keep track of these resources from the `WebGLRenderingContext` structure, so a texture could be destroyed after the context and give us problems.
I can't reproduce locally the failure that produced them, and since the builds that triggered them are deleted for some reason (like http://build.servo.org/builders/linux-dev/builds/419), it could be a good moment to troubleshoot it. If merged closes #7931
|
r? @jdm |
|
I don't know how the part of migrating tests to wpt/mozilla is going, probably should move them there instead of re-enabling them in test/ref? |
|
@jdm I added two commits moving the reftests to mozilla/wpt. The 6 tests are passing for me, tell me if you want me to remove them though :) |
|
These changes basically look fine to me. We just need to fix the |
|
@jdm That just loads the gl symbols calling the corresponding API (glx/cgl/egl...). It does the same that the line you mentioned, but without creating a gl context itself. I did that just in order to not depend on a glutin context to load the symbols. It's messy why we have a headless mode via feature and a headless mode via flag, and why they're handled differently, but... |
|
I updated the PR so it should pass tidy now :) |
|
@bors-servo: r+ |
|
|
Add destructors to some WebGL objects, remove duplicated glutin dependency and try to enable the webgl reftests The first commit allows to cleanup the gl resources of the webgl task earlier if they aren't being used. Right now all resources were cleaned up when the context was destroyed, so I think this is a slightly better approach. The second commit bumps rust-offscreen-rendering-context to remove the duplicated glutin dependency. The third one tries to reenable the webgl reftests. Since the errored builds are deleted, It's the only way I can try to troubleshoot it. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8291) <!-- Reviewable:end -->
|
|
|
|
Seems legit, for android servo omits it too (http://mxr.mozilla.org/servo/source/ports/glutin/window.rs#155)... Maybe we're statically linking OpenGL there? In that case I'll omit it on android too |
|
What's the linux buildbot environment like? |
|
Did you see the stack trace from the other issue? We do have the ability to disable tests on particular platforms, too. |
This new version shold prevent crashing on the linux buildbots.
|
Check this out, I think this might be the cause: servo/surfman@aae6e57 I find it really weird that it fails on May you do a try build? (the tests should still fail, but not segfaulting at least). I'm not sure why the X server reports that there's no display though. |
|
@bors-servo try |
|
|
Add destructors to some WebGL objects, remove duplicated glutin dependency and try to enable the webgl reftests The first commit allows to cleanup the gl resources of the webgl task earlier if they aren't being used. Right now all resources were cleaned up when the context was destroyed, so I think this is a slightly better approach. The second commit bumps rust-offscreen-rendering-context to remove the duplicated glutin dependency. The third one tries to reenable the webgl reftests. Since the errored builds are deleted, It's the only way I can try to troubleshoot it. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8291) <!-- Reviewable:end -->
|
|
|
The changes don't seem to help; still just crashes (see http://build.servo.org/builders/linux-rel/builds/164/steps/shell_1/logs/stdio). |
|
Still crashing on linux, now I have no damn idea about what's going on... The mac failure seems like an unrelated timeout, so I think we're fine disabling these tests on linux for now, if we can't do better debugging. How would I disable a reftest for a given platform? I guess it should be something like |
|
BTW @eefriedman thanks for your help! :) |
|
@jdm Done :P |
|
@bors-servo: r+ |
|
|
Add destructors to some WebGL objects, remove duplicated glutin dependency and try to enable the webgl reftests The first commit allows to cleanup the gl resources of the webgl task earlier if they aren't being used. Right now all resources were cleaned up when the context was destroyed, so I think this is a slightly better approach. The second commit bumps rust-offscreen-rendering-context to remove the duplicated glutin dependency. The third one tries to reenable the webgl reftests. Since the errored builds are deleted, It's the only way I can try to troubleshoot it. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8291) <!-- Reviewable:end -->
|
|
emilio commentedNov 1, 2015
The first commit allows to cleanup the gl resources of the webgl task earlier if they aren't being used.
Right now all resources were cleaned up when the context was destroyed, so I think this is
a slightly better approach.
The second commit bumps rust-offscreen-rendering-context to remove the duplicated glutin dependency.
The third one tries to reenable the webgl reftests.
Since the errored builds are deleted, It's the only way I can try to troubleshoot it.